feat(bigtable): route point read rows to shim#13542
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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, |
There was a problem hiding this comment.
I don't think this review is right. readRows callable is converted to readRow callable with Transforming callable.
Switch annotations and imports to org.junit.jupiter so the test fits the project's existing JUnit 5 conventions.
igorbernstein2
left a comment
igorbernstein2
left a comment
There was a problem hiding this comment.
lgtm, but please think about the effects of the settings change
2 participants
No description provided.