feat(bigtable): route point read rows to shim#13542
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces MaybePointReadCallable to route single-point ReadRows queries through a unary point-read callable, allowing them to benefit from session-shim optimizations. It also adds helper methods in Query to identify single-point queries and updates EnhancedBigtableStub to integrate this new routing logic. The reviewer feedback highlights several critical improvements: addressing potential synchronous exceptions and cancellation handling in MaybePointReadCallable to comply with the ResponseObserver contract, and properly propagating custom retryableCodes when routing ReadRows calls to point reads.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces MaybePointReadCallable to optimize ReadRows calls by routing single-row point queries through a unary point-read callable, allowing them to benefit from session-shim optimizations. It also adds helper methods in Query to identify single-point queries and extracts a reusable createPointReadCallable helper in EnhancedBigtableStub. A critical bug was identified in EnhancedBigtableStub where the locally created readRowsCallable is ignored in favor of the class field readRowCallable, bypassing the custom retry settings and row adapter.
| @@ -302,16 +327,20 @@ public <RowT> UnaryCallable<Query, RowT> createReadRowCallable(RowAdapter<RowT> | |||
| BigtableUnaryOperationCallable<Query, RowT> classic = | |||
| new BigtableUnaryOperationCallable<>( | |||
| readRowCallable, | |||
There was a problem hiding this comment.
There is a bug here where the locally created readRowsCallable (which is configured with the correct retrySettings, retryableCodes, and rowAdapter) is ignored. Instead, the class field readRowCallable is passed to BigtableUnaryOperationCallable. This causes the custom retry settings and the generic rowAdapter to be bypassed. Please update this to use readRowsCallable.
| readRowCallable, | |
| readRowsCallable, |
No description provided.