Skip to content

[java-bigtable] Mutation's MAX_MUTATIONS and MAX_BYTE_SIZE do not count mutations wrapped from protos #14020

Description

@laughingman7743

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

Summary

Mutation.MAX_MUTATIONS and Mutation.MAX_BYTE_SIZE are enforced in addMutation, against
counters that only addMutation maintains. Three factories add mutations without going through it,
so mutations wrapped from existing protos do not count towards either limit. For the mutation count
this is caught later by a separate check; for the byte size there is no check anywhere else, so the
200 MB guard simply does not apply to those mutations.

Detail

The counters and the guard:

// 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);
}

Three factories add to the same list directly, leaving both counters at zero:

// Mutation.java:85-91, 98-105, 115-121
public static Mutation fromProtoUnsafe(List<com.google.bigtable.v2.Mutation> protos) {
  Mutation mutation = new Mutation(true);
  mutation.mutations.addAll(protos);      // numMutations and byteSize untouched
  return mutation;
}
// ... the Iterable overload and the package-private fromProto(List) are the same shape

They are reachable publicly through Mutation.fromProtoUnsafe(...) (@BetaApi),
RowMutationEntry.createFromMutationUnsafe(...) (@BetaApi) and
RowMutation.fromProto(MutateRowRequest), which is the one in-tree caller of the package-private
fromProto.

Reproduction

Measured, not inferred — a probe on 2.80.0, five mutations wrapped from protos and then
MAX_MUTATIONS more added through setCell:

MAX_MUTATIONS=100000
seeded=5 addedViaSetCell=100000 threw=null
actual mutations in list = 100005
toProto() rejected: IllegalArgumentException: Too many mutations, got 100005, limit is 100000

So the guard in addMutation never fires, and the row grows past the limit unchecked.

What is and is not caught downstream

I want to be precise about the impact, because the two limits differ:

  • Mutation count is backstopped, so it does surface — but late, from a different place and as a
    different exception type. RowMutationEntry.toProto() (RowMutationEntry.java:205-209) and
    BulkMutation.add (BulkMutation.java:103-106) both re-check against getMutations().size(),
    which is the real count. The user gets an IllegalArgumentException at send time rather than an
    IllegalStateException from the setCell that actually crossed the line.
  • Byte size is not backstopped at all. MAX_BYTE_SIZE appears only in addMutation. A
    Mutation seeded from protos can exceed 200 MB — with or without further setCell calls — and
    nothing client-side objects; the request is built and sent, and the server rejects it. That is
    the half of this that actually loses the guard rather than deferring it.

Suggested fix

Count wrapped protos in the three factories, so the counters describe the whole row:

private void countTowardsLimits(com.google.bigtable.v2.Mutation mutation) {
  numMutations++;
  byteSize += mutation.getSerializedSize();
}

called from addMutation and from each fromProto factory. Deliberately not a checkState in
the factories themselves: that would make wrapping an over-limit proto throw where it currently
does not, which is a bigger behaviour change than restoring the guard needs.

This does change behaviour: a caller that wraps protos and then adds more mutations will now hit
IllegalStateException at the setCell that crosses a limit, where today it either fails later
(count) or not at all (bytes). I think that is the intent of the limits — fail fast client-side
rather than send a request that cannot succeed — but it is your call, and I would understand
wanting it behind a release note.

I have this implemented with tests that fail without it, and will open a PR referencing this issue.

Related

Two other findings in the same area are filed separately and are independent of this one: the
duplicate proto construction in MutateRowsBatchingDescriptor.createResource(), and the absence of
a serialized-size accessor on RowMutationEntry.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions