[alexoneill]

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.
• Store Optional<Duration> perOperationTimeout() on CbtClient while keeping client.settings() at the unmodified library defaults, and resolve the effective timeout per operation via resolveTimeout (request-level timeout supersedes client.perOperationTimeout()).
• 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 across all operations.

[gemini-code-assist]

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.

[gemini-code-assist]

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

[alexoneill]

Done — guarded cancellableContext.detach(previousContext) with if (previousContext != null).

[mutianf]

Hmm, use MoreExecutores.directExecutor() instead of null?

[alexoneill]

Done — added a ScheduledExecutorService deadlineScheduler on CbtTestProxy (shut down in close()) and passed it to withDeadlineAfter (which requires a ScheduledExecutorService), along with a CancellationListener to interrupt the caller on deadline expiration.

[mutianf]

Instead of mutating BigtableDataSettings in createClient (which overwrites the library defaults and requires the equals(oldTotalTimeout) heuristic to unclamp rpcTimeout / maxRpcTimeout), could we store Optional<Duration> perOperationTimeout on CbtClient and leave client.settings() at the library defaults (similar to the C++ test proxy)?

Then every operation can resolve the effective timeout (request override -> client override -> empty) and only clamp down from the untouched library defaults:

  @AutoValue
  abstract static class CbtClient {
    static CbtClient create(BigtableDataSettings settings, BigtableDataClient dataClient) {
      return create(settings, dataClient, Optional.empty());
    }

    static CbtClient create(
        BigtableDataSettings settings,
        BigtableDataClient dataClient,
        Optional<Duration> perOperationTimeout) {
      return new AutoValue_CbtTestProxy_CbtClient(settings, dataClient, perOperationTimeout);
    }

    abstract BigtableDataSettings settings();

    abstract BigtableDataClient dataClient();

    abstract Optional<Duration> perOperationTimeout();
  }

  private static Optional<Duration> resolveTimeout(
      @Nullable CbtClient client,
      boolean hasRequestTimeout,
      com.google.protobuf.Duration requestTimeout) {
    if (hasRequestTimeout && Durations.compare(requestTimeout, Durations.ZERO) > 0) {
      return Optional.of(
          Duration.ofSeconds(requestTimeout.getSeconds(), requestTimeout.getNanos()));
    }
    return client != null ? client.perOperationTimeout() : Optional.empty();
  }

  private static ApiCallContext createCallContextWithTimeout(
      RetrySettings defaultRetrySettings, Duration timeout) {
    RetrySettings.Builder builder = defaultRetrySettings.toBuilder();
    Duration initialRpcTimeout = builder.getInitialRpcTimeoutDuration();
    if (initialRpcTimeout != null && initialRpcTimeout.compareTo(timeout) > 0) {
      builder.setInitialRpcTimeoutDuration(timeout);
    }
    Duration maxRpcTimeout = builder.getMaxRpcTimeoutDuration();
    if (maxRpcTimeout != null && maxRpcTimeout.compareTo(timeout) > 0) {
      builder.setMaxRpcTimeoutDuration(timeout);
    }
    builder.setTotalTimeoutDuration(timeout);
    return GrpcCallContext.createDefault().withRetrySettings(builder.build());
  }

And in each operation handler:

    Optional<Duration> timeout =
        resolveTimeout(client, request.hasPerOperationTimeout(), request.getPerOperationTimeout());
    if (timeout.isPresent()) {
      ApiCallContext callContext =
          createCallContextWithTimeout(
              client.settings().getStubSettings().mutateRowSettings().getRetrySettings(),
              timeout.get());
      client.dataClient().mutateRowCallable().call(mutation, callContext);
    } else {
      client.dataClient().mutateRow(mutation);
    }

[alexoneill]

Done — stored Optional<Duration> perOperationTimeout() on CbtClient while keeping client.settings() at the unmodified library defaults, and added resolveTimeout + simplified createCallContextWithTimeout.