Skip to content

Commit afa3296

Browse files
laughingman7743laughingman7743
committed
fix(bigtable): count mutations wrapped from protos towards the row limits
Mutation.MAX_MUTATIONS and MAX_BYTE_SIZE are enforced in addMutation, against counters only addMutation maintains. fromProtoUnsafe(List), fromProtoUnsafe(Iterable) and fromProto(List) add to the mutation list directly and leave both counters at zero, so mutations wrapped from existing protos count towards neither limit. The mutation count is backstopped by RowMutationEntry.toProto() and BulkMutation.add, which re-check the real list size, so it surfaces late and as a different exception type. The byte size is not backstopped anywhere, so a Mutation seeded from protos can exceed 200 MB with nothing client-side objecting. Count wrapped protos in the three factories, so the counters describe the whole row. Deliberately not a checkState in the factories themselves: that would make wrapping an already-over-limit proto throw where it currently does not.
1 parent 9337a93 commit afa3296

2 files changed

Lines changed: 117 additions & 4 deletions

File tree

  • java-bigtable/google-cloud-bigtable/src

‎java-bigtable/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/models/Mutation.java‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ public static Mutation createUnsafe() {
8585
@BetaApi
8686
public static Mutation fromProtoUnsafe(List<com.google.bigtable.v2.Mutation> protos) {
8787
Mutation mutation = new Mutation(true);
88-
mutation.mutations.addAll(protos);
88+
mutation.addAllFromProto(protos);
8989
return mutation;
9090
}
9191

@@ -98,7 +98,7 @@ public static Mutation fromProtoUnsafe(List<com.google.bigtable.v2.Mutation> pro
9898
@BetaApi
9999
public static Mutation fromProtoUnsafe(Iterable<com.google.bigtable.v2.Mutation> protos) {
100100
Mutation mutation = new Mutation(true);
101-
mutation.mutations.addAll(protos);
101+
mutation.addAllFromProto(protos);
102102
return mutation;
103103
}
104104

@@ -114,7 +114,7 @@ public static Mutation fromProtoUnsafe(Iterable<com.google.bigtable.v2.Mutation>
114114
*/
115115
static Mutation fromProto(List<com.google.bigtable.v2.Mutation> protos) {
116116
Mutation mutation = new Mutation(false);
117-
mutation.mutations.addAll(protos);
117+
mutation.addAllFromProto(protos);
118118
return mutation;
119119
}
120120

@@ -333,10 +333,22 @@ private void addMutation(com.google.bigtable.v2.Mutation mutation) {
333333
byteSize + mutation.getSerializedSize() <= MAX_BYTE_SIZE,
334334
"Byte size of mutations is too large");
335335

336+
countTowardsLimits(mutation);
337+
338+
mutations.add(mutation);
339+
}
340+
341+
private void countTowardsLimits(com.google.bigtable.v2.Mutation mutation) {
336342
numMutations++;
337343
byteSize += mutation.getSerializedSize();
344+
}
338345

339-
mutations.add(mutation);
346+
private void addAllFromProto(Iterable<com.google.bigtable.v2.Mutation> protos) {
347+
// One traversal: protos may be a non-repeatable Iterable.
348+
for (com.google.bigtable.v2.Mutation proto : protos) {
349+
mutations.add(proto);
350+
countTowardsLimits(proto);
351+
}
340352
}
341353

342354
private static ByteString wrapByteString(String str) {

‎java-bigtable/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/data/v2/models/MutationTest.java‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,15 @@
2323
import com.google.bigtable.v2.Mutation.DeleteFromRow;
2424
import com.google.bigtable.v2.Mutation.MergeToCell;
2525
import com.google.cloud.bigtable.data.v2.models.Range.TimestampRange;
26+
import com.google.common.collect.ImmutableList;
2627
import com.google.common.primitives.Longs;
2728
import com.google.protobuf.ByteString;
2829
import java.io.ByteArrayInputStream;
2930
import java.io.ByteArrayOutputStream;
3031
import java.io.IOException;
3132
import java.io.ObjectInputStream;
3233
import java.io.ObjectOutputStream;
34+
import java.util.Iterator;
3335
import java.util.List;
3436
import org.junit.Before;
3537
import org.junit.Test;
@@ -261,6 +263,105 @@ public void tooLargeRequest() {
261263
assertThat(actualError).isInstanceOf(IllegalStateException.class);
262264
}
263265

266+
@Test
267+
public void tooManyMutationsCountsWrappedProtosTest() {
268+
Mutation mutation =
269+
Mutation.fromProtoUnsafe(
270+
ImmutableList.of(
271+
com.google.bigtable.v2.Mutation.newBuilder()
272+
.setDeleteFromRow(com.google.bigtable.v2.Mutation.DeleteFromRow.newBuilder())
273+
.build()));
274+
275+
for (int i = 0; i < Mutation.MAX_MUTATIONS - 1; i++) {
276+
mutation.setCell("f", "", "");
277+
}
278+
279+
Exception actualError = null;
280+
try {
281+
mutation.setCell("f", "", "");
282+
} catch (Exception e) {
283+
actualError = e;
284+
}
285+
286+
assertThat(actualError).isInstanceOf(IllegalStateException.class);
287+
assertThat(mutation.getMutations()).hasSize(Mutation.MAX_MUTATIONS);
288+
}
289+
290+
@Test
291+
public void tooLargeRequestCountsWrappedProtosTest() {
292+
Mutation mutation =
293+
Mutation.fromProtoUnsafe(
294+
ImmutableList.of(
295+
com.google.bigtable.v2.Mutation.newBuilder()
296+
.setSetCell(
297+
com.google.bigtable.v2.Mutation.SetCell.newBuilder()
298+
.setFamilyName("f")
299+
.setValue(ByteString.copyFrom(new byte[Mutation.MAX_BYTE_SIZE / 2])))
300+
.build(),
301+
com.google.bigtable.v2.Mutation.newBuilder()
302+
.setSetCell(
303+
com.google.bigtable.v2.Mutation.SetCell.newBuilder()
304+
.setFamilyName("f")
305+
.setValue(ByteString.copyFrom(new byte[Mutation.MAX_BYTE_SIZE / 2])))
306+
.build()));
307+
308+
Exception actualError = null;
309+
try {
310+
mutation.setCell("f", "", "");
311+
} catch (Exception e) {
312+
actualError = e;
313+
}
314+
315+
assertThat(actualError).isInstanceOf(IllegalStateException.class);
316+
}
317+
318+
@Test
319+
public void fromProtoUnsafeTraversesTheIterableOnceTest() {
320+
Iterable<com.google.bigtable.v2.Mutation> oneShot =
321+
oneShot(
322+
com.google.bigtable.v2.Mutation.newBuilder()
323+
.setDeleteFromRow(com.google.bigtable.v2.Mutation.DeleteFromRow.newBuilder())
324+
.build(),
325+
com.google.bigtable.v2.Mutation.newBuilder()
326+
.setDeleteFromRow(com.google.bigtable.v2.Mutation.DeleteFromRow.newBuilder())
327+
.build());
328+
329+
Mutation mutation = Mutation.fromProtoUnsafe(oneShot);
330+
331+
assertThat(mutation.getMutations()).hasSize(2);
332+
333+
for (int i = 0; i < Mutation.MAX_MUTATIONS - 2; i++) {
334+
mutation.setCell("f", "", "");
335+
}
336+
337+
Exception actualError = null;
338+
try {
339+
mutation.setCell("f", "", "");
340+
} catch (Exception e) {
341+
actualError = e;
342+
}
343+
344+
assertThat(actualError).isInstanceOf(IllegalStateException.class);
345+
}
346+
347+
/** An Iterable that refuses a second traversal. */
348+
private static Iterable<com.google.bigtable.v2.Mutation> oneShot(
349+
com.google.bigtable.v2.Mutation... protos) {
350+
List<com.google.bigtable.v2.Mutation> source = ImmutableList.copyOf(protos);
351+
return new Iterable<com.google.bigtable.v2.Mutation>() {
352+
private boolean consumed;
353+
354+
@Override
355+
public Iterator<com.google.bigtable.v2.Mutation> iterator() {
356+
if (consumed) {
357+
throw new IllegalStateException("Iterable traversed more than once");
358+
}
359+
consumed = true;
360+
return source.iterator();
361+
}
362+
};
363+
}
364+
264365
@Test
265366
public void testWithLongValue() {
266367
Mutation mutation =

0 commit comments

Comments
 (0)