Bigtable: 21. Refactor Batching - Move retries behind batching #3026
Conversation
googlebot
added
the
cla: yes
igorbernstein2
force-pushed
the
21-improve-batching-retries
branch
5 times, most recently
from
6f4270d to
7227e77
Compare
|
Rebased. This is ready for review. @garrettjonesgoogle & @vam-google can you take a look when you have a moment? |
igorbernstein2
changed the title
|
I added it a few lines below your comment: https://github.com/GoogleCloudPlatform/google-cloud-java/pull/3026/files#diff-8bd7e4ecc548faa3568dadc99cab3f40R194 Thanks again for reviewing |
igorbernstein2
force-pushed
the
21-improve-batching-retries
branch
from
0329e2b to
e84f474
Compare
igorbernstein2
force-pushed
the
21-improve-batching-retries
branch
from
e84f474 to
c1ea2c1
Compare
|
I just did an interactive rebase to merge kevin's feedback back into the original commits (so that it can still be reviewed a commit at a time). @garrettjonesgoogle PTAL when you have a moment |
| * } catch(ExecutionException executionError) { | ||
| * MutateRowsException e = (MutateRowsException)executionError.getCause(); | ||
| * | ||
| * for(FailedMutation m : e.getFailedMutations() { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| * Send the current request and the parent {@link RetryingFuture} with this attempt's future. | ||
| * | ||
| * <p>On RPC completion this method will preprocess all errors (both RPC level and entry level) | ||
| * and wrap them in a {@link MutateRowsException}. Please note that the results of RPC are only |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| } | ||
|
|
||
| /** | ||
| * Handle RPC level failure by generating a {@link FailedMutation} for each expected entry. The |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| permanentFailures.add(failedMutation); | ||
| } else { | ||
| // Schedule the mutation entry for the next RPC by adding it to the request builder and | ||
| // recording it's original index |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| } | ||
|
|
||
| currentRequest = builder.build(); | ||
| originalIndexes = newOriginalIndexes; |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
Thanks for reviewing, please merge when ready |
This is the first step to enable batch flushing. Currently, the outermost callable for bulk mutations is a RetryingCallable. Although this composes nicely with automatic batching, it makes it very difficult to reason about manual flushing of batches. For example: what happens when multiple entries fail after a flush? should batcher retry each entry individually? should it spool them? etc. By moving the retries after batching, flushing becomes easier: on flush, the batcher sends all of its batches to the retry logic and waits for all batches to either succeed or run out of attempts.
The implementation uses the retrying infrastructure directly, bypassing Callable.retrying & RetryingCallable. This is necessary because I need to track state of completed entries. This state is maintained in the new MutateRowsAttemptCallable class, which tracks incomplete mutation entries. The class also transforms the results of the RPC to be a bit more ergonomic:
At some point in the future I would like to try to generalize
MutateRowsAttemptCallableto simply wrap something like aBatchingDescriptor. This would allow the GAPIC generated batching configuration to be used for implementing partial retries on batches.The PR is broken up into 3 commits:
Callable, which is equivalent of gax's AttemptCallable. It contains all of the logic for composing a request.Callablein aUnaryCallablewhich uses gax'sRetryingExecutorto manage retry attemptsIt would be easiest to review this a commit at a time.