feat: introduce Bigtable Admin V2 clients for Selective GAPIC#13173
feat: introduce Bigtable Admin V2 clients for Selective GAPIC#13173jinseopkim0 wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces modern V2 Admin clients for Bigtable, specifically BigtableInstanceAdminClientV2 and BigtableTableAdminClientV2, which provide enhanced convenience methods for consistency polling and table restoration optimization. The changes also include modifications to the base admin clients and OwlBot configuration to allow for custom lifecycle management of background executors. Review feedback identifies several critical issues in the new Table Admin client: a blocking call within an asynchronous method that should utilize ApiFutures.transformAsync, potential resource management conflicts when shutting down shared executors, and a logic error in the awaitTermination implementation that fails to account for elapsed time when calling the superclass method.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces modern Cloud Bigtable Admin clients, BigtableInstanceAdminClientV2 and BigtableTableAdminClientV2, providing enhanced convenience methods for consistency polling and table optimization. Key changes include the addition of AwaitConsistencyCallableV2 for automated polling and modifications to the base admin clients to allow overriding the close() method for better resource management. Review feedback identifies a performance issue where a blocking .get() call is used in an asynchronous method and suggests using a more accurate RPC method descriptor for operation parsing.
| /** | ||
| * Awaits the completion of the "Optimize Restored Table" operation. | ||
| * | ||
| * <p>This method blocks until the restore operation is complete, extracts the optimization token, | ||
| * and returns an ApiFuture for the optimization phase. | ||
| * | ||
| * @param restoreFuture The future returned by restoreTableAsync(). | ||
| * @return An ApiFuture that tracks the optimization progress. | ||
| */ | ||
| public ApiFuture<Empty> awaitOptimizeRestoredTable(ApiFuture<RestoredTableResult> restoreFuture) { | ||
| // 1. Block and wait for the restore operation to complete | ||
| RestoredTableResult result; | ||
| try { | ||
| result = restoreFuture.get(); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException("Restore operation failed", e); | ||
| } | ||
|
|
||
| // 2. Extract the operation token from the result | ||
| // (RestoredTableResult already wraps the OptimizeRestoredTableOperationToken) | ||
| OptimizeRestoredTableOperationToken token = result.getOptimizeRestoredTableOperationToken(); | ||
|
|
||
| if (token == null || Strings.isNullOrEmpty(token.getOperationName())) { | ||
| // If there is no optimization operation, return immediate success. | ||
| return ApiFutures.immediateFuture(Empty.getDefaultInstance()); | ||
| } | ||
|
|
||
| // 3. Return the future for the optimization operation | ||
| return getOptimizeRestoredTableCallable().resumeFutureCall(token.getOperationName()); | ||
| } |
There was a problem hiding this comment.
The method awaitOptimizeRestoredTable(ApiFuture<RestoredTableResult> restoreFuture) returns an ApiFuture but performs a blocking .get() call on the input future. This defeats the purpose of returning a future and can lead to thread starvation if called on an executor thread. It should use ApiFutures.transformAsync to chain the operations asynchronously. Additionally, the Javadoc should be updated to reflect that the method is non-blocking.
/**
* Awaits the completion of the "Optimize Restored Table" operation asynchronously.
*
* <p>This method chains the optimization phase to the completion of the restore operation.
*
* @param restoreFuture The future returned by restoreTableAsync().
* @return An ApiFuture that tracks the optimization progress.
*/
public ApiFuture<Empty> awaitOptimizeRestoredTable(ApiFuture<RestoredTableResult> restoreFuture) {
return ApiFutures.transformAsync(
restoreFuture,
new com.google.api.core.ApiAsyncFunction<RestoredTableResult, Empty>() {
@Override
public ApiFuture<Empty> apply(RestoredTableResult result) {
// Extract the operation token from the result
OptimizeRestoredTableOperationToken token =
result.getOptimizeRestoredTableOperationToken();
if (token == null || Strings.isNullOrEmpty(token.getOperationName())) {
// If there is no optimization operation, return immediate success.
return ApiFutures.immediateFuture(Empty.getDefaultInstance());
}
// Return the future for the optimization operation
return getOptimizeRestoredTableCallable().resumeFutureCall(token.getOperationName());
}
},
com.google.common.util.concurrent.MoreExecutors.directExecutor());
}References
- Update stale Javadoc comments to reflect new method behavior for improved clarity, rather than removing them.
| MethodDescriptor<Void, Operation> fakeDescriptor = | ||
| (MethodDescriptor<Void, Operation>) | ||
| (MethodDescriptor<?, ?>) | ||
| com.google.bigtable.admin.v2.BigtableTableAdminGrpc.getUpdateTableMethod(); | ||
|
|
There was a problem hiding this comment.
Using getUpdateTableMethod() as a proxy for an operation-returning RPC is misleading and potentially dangerous because UpdateTable returns a Table object, not a google.longrunning.Operation. Although it's labeled as "unused", it's better to use an RPC that actually returns an Operation, such as getRestoreTableMethod(), to ensure type compatibility with the Operation response parsing logic.
| MethodDescriptor<Void, Operation> fakeDescriptor = | |
| (MethodDescriptor<Void, Operation>) | |
| (MethodDescriptor<?, ?>) | |
| com.google.bigtable.admin.v2.BigtableTableAdminGrpc.getUpdateTableMethod(); | |
| @SuppressWarnings("unchecked") | |
| MethodDescriptor<Void, Operation> fakeDescriptor = | |
| (MethodDescriptor<Void, Operation>) | |
| (MethodDescriptor<?, ?>) | |
| com.google.bigtable.admin.v2.BigtableTableAdminGrpc.getRestoreTableMethod(); |
This PR introduces the modern
BigtableTableAdminClientV2andBigtableInstanceAdminClientV2clients injava-bigtableas handwritten entry points for the Selective GAPIC hierarchy.It is a migration of the original PR from the split repository (googleapis/java-bigtable#2889) into the
google-cloud-javamonorepo.Key Changes
1. Core V2 Clients & Callables
BigtableInstanceAdminClientV2: Recreates the standardized admin entry point extendingBaseBigtableInstanceAdminClient.BigtableTableAdminClientV2: Recreates the administrative client extendingBaseBigtableTableAdminClient. It relocates manual wrappers for Critical User Journeys (CUJs) like chained Long-Running Operations (awaitOptimizeRestoredTable) and consistency token polling (waitForConsistency).AwaitConsistencyCallableV2: Implements a completely decoupled, lightweight utility callable for polling consistency tokens, ensuring the V2 administrative client stack has no compile-time dependency on the data client package.2. Code Generation & POM Rules
owlbot.py: Added a post-processing rule to remove thefinalmodifier from theclose()method inBaseBigtable*AdminClientclasses to allow handwritten subclass overrides.pom.xml: Promoted the scope ofgrpc-google-cloud-bigtable-admin-v2fromtestto compile scope to support V2 admin gRPC descriptors and classes in production code.