googleapis / googleapis/google-cloud-java

[java-bigtable] RowMutationEntry exposes no serialized size, so bulk-mutation callers must rebuild the proto

Đang mở
#14,018 0 bình luận 0 reaction 1 người được giao Được @mutianf nhận Xem trên GitHub
Ngôn ngữ chính
Java
Star
2.1k
Fork
1.2k
Merge trung bình
1 ngày 23 giờ
Pull request đã merge (30 ngày)
154

Mô tả

Version: `google-cloud-bigtable` 2.80.0. `RowMutationEntry.java` and `Mutation.java` are
byte-identical at `main` today, so the line numbers below apply there too.

### Summary

`RowMutationEntry` exposes no accessor for its serialized size, its row key or its mutation count. A
caller that needs any of those — to bound its own in-flight memory, to report a bytes-written
metric, or to put a rejected mutation on a dead-letter queue — has to call the `@InternalApi`
`toProto()` and build a whole `MutateRowsRequest.Entry` to read one number.

### Detail

The whole public instance surface of `RowMutationEntry` is the mutation builders plus:

```java
// RowMutationEntry.java:203-204
@InternalApi
public MutateRowsRequest.Entry toProto() {
```

`Mutation` already accumulates a size on every cell added, though not the framed size a caller
would need, and it is private with no accessor:

```java
// Mutation.java:329-340
private void addMutation(com.google.bigtable.v2.Mutation mutation) {
Preconditions.checkState(numMutations + 1 <= MAX_MUTATIONS, "Too many mutations per row");
Preconditions.checkState(
byteSize + mutation.getSerializedSize() <= MAX_BYTE_SIZE,
"Byte size of mutations is too large");

numMutations++;
byteSize += mutation.getSerializedSize();

mutations.add(mutation);
}
```

The client's own batching path needs exactly this number and gets it the expensive way —
`MutateRowsBatchingDescriptor.countBytes()` calls `entry.toProto().getSerializedSize()`.

### Why a caller needs it

Bulk mutations are unbounded in size: a single row mutation may legitimately be megabytes
(`Mutation.MAX_BYTE_SIZE` is 200 MB), so an application that bounds its own in-flight work by
counting entries bounds nothing useful. Bounding by bytes requires each entry's size, and
`toProto()` is the only route to it — an `@InternalApi` method, which callers are asked not to
depend on, for a value the object could know.

The same gap makes the row key unreachable. Reporting which mutation was rejected, or writing it to
a dead-letter destination, means building the proto purely to call `getRowKey()`.

### Cost of the workaround

Measured on OpenJDK 21.0.5 (aarch64, `-Xmx4g`, default collector) against `google-cloud-bigtable`
2.80.0; five JVM forks, 4 s warmup and 7 timed iterations of >=2 s each, allocation sampled over
the same loop:

| Entry shape | serialized size | `toProto().getSerializedSize()` | allocated |
|---|---|---|---|
| 1 cell, 64 B value | 88 B | 33.0 ns | 176 B |
| 8 cells, 128 B values | 1191 B | 154.1 ns | 336 B |
| 64 cells, 128 B values | 9533 B | 912.0 ns | 1232 B |

Small next to an RPC, and that is not really the argument — the argument is that it is an
`@InternalApi` dependency taken to read a value the object holds, on every record of a streaming
pipeline.

### Proposed API

```java
public ByteString getRowKey();
public int getMutationCount();
public long getSerializedSize(); // == toProto().getSerializedSize(), without building it
```

I have this implemented and tested, and will open a PR referencing this issue — happy to reshape it
if you would rather have different names, a subset, or an explicitly approximate size. Two things
that came out of implementing it, in case they affect what you want:

- **`Mutation.byteSize` is not the number**, so this cannot just be exposed. It sums each child's
own serialized size and omits the row-key field and the per-mutation tag and length prefixes. My
PR accumulates the framed size alongside it instead.
- **Three factories bypass `addMutation`** — `fromProtoUnsafe(List)`, `fromProtoUnsafe(Iterable)`
and `fromProto(List)` all do `mutation.mutations.addAll(protos)` directly, so `numMutations` and
`byteSize` stay at zero for entries built through `createFromMutationUnsafe`. That is pre-existing
and I have not changed it (the `MAX_MUTATIONS` and `MAX_BYTE_SIZE` preconditions therefore do not
account for those mutations today), but any accessor added here has to be maintained on those
paths or it would be silently wrong for them.

### Related

The client's own duplicate proto construction in `MutateRowsBatchingDescriptor.createResource()` is
filed separately, and is fixable without any API change.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.