BulkMutation can fail if a batch contains > 100,000 mutations

[steveniemitz]

I ran into this using Dataflow (using bigtable-client-core 0.9.7.1), but I think it happens in the latest 1.0.0-pre3 release as well.

When bulk loading some very-highly dimensional data, I ran into an issue where a batch from BulkMutation would be rejected by the Bigtable instance with the error Status{code=INVALID_ARGUMENT, description=Too many mutations: 238015 provided; limit 100000, cause=null},

Looking through BulkMutation, it seems that although size and # of rowkeys is checked, there's nothing to prevent a large number of mutations.

It seems like the logic in add() needs to be inverted somewhat, where it would check if adding the current item to a batch would cause it to be > 100,000 mutations, and send the existing batch if so.

[sduskis]

That's a server side limitation. The default settings are to have 25 mutations per request. Did you change the defaults?

[steveniemitz]

I didn't change the defaults. Most of my rows are small-ish, ~20 mutations, but some can end up being very large (I've seen some > 2000). I suspect sometimes I just get unlucky and end up with a batch with a few very large ones.

edit: also, a correction, the default is 25 row keys, not mutations.

[steveniemitz]

Following up, I added some logic in BulkMutation to do a couple things (which solved my problem)

1. Keep track of the number of actual mutations in a batch and respond appropriately in isFull
2. Run the flush() logic BEFORE adding the next mutation. This basically involved renaming isFull() to wouldBeFull(MutateRowsRequest.Entry) and calling it before enqueuing an entry.

Of these, I'm pretty sure the second was the most important. The key point is that if I had an "almost full" batch, I'd still be able to add the next large mutation set, even if doing so would put it over the maximum. Checking before adding allows the batch to reset and accept the new large mutation in the next round. The first check is a good "just in case" check though to catch other edge cases.

Additionally, I wonder if its worth validating the number of mutations in a single MutateRowRequest before even attempting to enqueue it if we know it'll never succeed.

[sduskis]

I didn't know that check existed. I'll update the client.

[steveniemitz]

I can submit a pull request if you want, the change is pretty trivial though.

Also interestingly enough a unit test actually hits this case right now, its just never checked for.

[sduskis]

A pull request would be awesome. An integration test would be great as well.

[steveniemitz]

cool I'll send one in a few minutes!

[alugowski]

This fix doesn't solve the case where a single mutation request is too large by itself.

[sduskis]

@alugowski, That's true. I don't think that clients ought to solve the > 100K cell row limitations as a general rule. Row writes are generally transactional, and writing more than 100K cells to a single row requires breaking up the row into smaller mutations thereby breaking transactionality.

Would you mind describing your situation and why you're writing 100K cells to a single row?

[alugowski]

@sduskis Good point about transaction guarantees.

We are hitting this as part of a Dataflow ingestion job. Our data is generally <100 cells per row spread over a handful of columns, but for some datapoints can grow into the tens of thousands. The data for all cells in a row is available all at once, so the most natural way to write processElement is to construct a Put and fill it with values for that row.

You can see that a large outlier can cause >100k cell writes. If that happens then this is a non-trivial error to spot because the exception is raised some time after the troublesome Put, and from a different thread. It also kills the Dataflow job. If I wasn't knee deep in the client code, and have experience from building this stuff in a previous life, I would have been unable to find the cause.

Our application can be split into multiple Puts, so we'll do that.

I can see an argument that our problem is actually user error, but it's an error that's on the most naive implementation path. I think the client shouldn't make such errors hard to spot and fix. If one were to follow the examples they'd hit this problem.

In light of your point, it seems the appropriate thing is to not allow Put instances that are impossible to store. I opened #1919 with that in mind.

[alugowski]

Realistic example: Imagine setting up the BigQueryBigtableTransfer example as a service. Many datasets have many columns in a CF. If someone were to run a query with more than 100k columns in a single row then you'd hit the case I'm describing.

https://github.com/GoogleCloudPlatform/cloud-bigtable-examples/blob/master/java/dataflow-connector-examples/src/main/java/com/google/cloud/bigtable/dataflow/example/BigQueryBigtableTransfer.java