Repository navigation
feat(bigtable): plumb per-operation timeouts through CbtTestProxy - #14997
alexoneill wants to merge 1 commit into
Conversation
Add an optional `google.protobuf.Duration per_operation_timeout` field to each operation request message in `test_proxy.proto` (`ReadRowRequest`, `ReadRowsRequest`, `MutateRowRequest`, `MutateRowsRequest`, `CheckAndMutateRowRequest`, `SampleRowKeysRequest`, `ReadModifyWriteRowRequest`, and `ExecuteQueryRequest`) so callers can override `CreateClientRequest.per_operation_timeout` (in both directions) and the client library default timeout on a per-call basis. - Pass `callContext` into `retryAlgorithm.createFirstAttempt(callContext)` in `MutateRowsAttemptCallable` so per-call `RetrySettings` overrides on `bulkMutationCallable().call(batch, callContext)` apply to entry-level retry decisions as well as the parent `RetryingFuture`. - Add `hasPositiveTimeout` and `createCallContextWithTimeout` in `CbtTestProxy`, updating `updateTimeout` to also raise `initialRpcTimeoutDuration` and `maxRpcTimeoutDuration` when they were previously clamped equal to `oldTotalTimeout`. - Plumb per-operation timeout overrides through `mutateRow`, `bulkMutateRows`, `readRow`, `readRows`, `sampleRowKeys`, `checkAndMutateRow`, `readModifyWriteRow`, and `executeQuery`. - Add unit tests in `CbtTestProxyTest` and `MutateRowsAttemptCallableTest` covering shorter and longer per-operation timeout overrides.
There was a problem hiding this comment.
Code Review
This pull request introduces per-operation timeout overrides for various Bigtable operations in the test proxy, updating CbtTestProxy to apply these timeouts and extending test_proxy.proto with per_operation_timeout fields. It also includes comprehensive tests verifying that these overrides correctly supersede client-level timeouts, and updates MutateRowsAttemptCallable to pass the call context on the first attempt. Feedback on the changes highlights a potential NullPointerException in CbtTestProxy.java if previousContext is null when calling detach, which should be guarded to prevent masking other exceptions.
| if (cancellableContext != null) { | ||
| cancellableContext.detach(previousContext); | ||
| cancellableContext.close(); | ||
| } |
There was a problem hiding this comment.
If cancellableContext is initialized but previousContext remains null (for example, if an exception occurs or the thread is interrupted before attach() completes), calling cancellableContext.detach(previousContext) will throw a NullPointerException because Context.detach does not accept null. This NPE would mask the original exception.
To prevent this, guard the detach call by checking if previousContext is not null.
if (previousContext != null) {
cancellableContext.detach(previousContext);
}
if (cancellableContext != null) {
cancellableContext.close();
}
Add an optional
google.protobuf.Duration per_operation_timeoutfield to each operation request message intest_proxy.proto(ReadRowRequest,ReadRowsRequest,MutateRowRequest,MutateRowsRequest,CheckAndMutateRowRequest,SampleRowKeysRequest,ReadModifyWriteRowRequest, andExecuteQueryRequest) so callers can overrideCreateClientRequest.per_operation_timeout(in both directions) and the client library default timeout on a per-call basis.callContextintoretryAlgorithm.createFirstAttempt(callContext)inMutateRowsAttemptCallableso per-callRetrySettingsoverrides onbulkMutationCallable().call(batch, callContext)apply to entry-level retry decisions as well as the parentRetryingFuture.hasPositiveTimeoutandcreateCallContextWithTimeoutinCbtTestProxy, updatingupdateTimeoutto also raiseinitialRpcTimeoutDurationandmaxRpcTimeoutDurationwhen they were previously clamped equal tooldTotalTimeout.mutateRow,bulkMutateRows,readRow,readRows,sampleRowKeys,checkAndMutateRow,readModifyWriteRow, andexecuteQuery.CbtTestProxyTestandMutateRowsAttemptCallableTestcovering shorter and longer per-operation timeout overrides.