lhotari opened a new pull request, #4829:
URL: https://github.com/apache/bookkeeper/pull/4829
Addresses #1440
### Motivation
`TableClientTest.testTableAPIServerSideRouting` has been flaky since 2018
(#1440), failing at `assertTrue(txnResult.isSuccess())`
(`TableClientTest.java:257`) with a clean client log. The CI logs attached to
#4802 captured the server-side counterpart of the same failure:
`IllegalArgumentException: Invalid unknonwn tag type: 4` thrown by
`TxnRequest.parseFrom` in the routing proxy interceptor, at routing key
`"txn-key"` — the exact key this test's txn uses.
Root cause chain:
1. Table service requests embed ByteBuf slices, and serializing a request
**drains** those slices (`writeTo` advances the stored slices' reader indexes
and is one-shot). A request instance therefore must not be reused across RPC
attempts. The simple client's put/get/delete/increment already build a fresh
request inside the retry supplier for exactly this reason — **txn was the
outlier, in both client paths**:
- `PByteBufSimpleTableImpl.TxnImpl.commit()` (server-side routing) passed
one pre-populated `TxnRequest` into `retryUtils.execute(...)`, so every retry
attempt re-serialized the same drained request.
- `TxnRequestProcessor.createRequest()` (client-side routing) returned
the same stored request on every attempt of `ListenableFutureRpcProcessor`'s
NOT_FOUND retry loop.
2. Retries in these paths are silent (`RetryUtils` retries most gRPC
statuses and logs nothing), and a first attempt can fail transiently by design
— e.g. a cold storage-container proxy channel makes
`StorageContainerStoreImpl.findChannel` return NOT_FOUND until the channel
future completes. The silent retry then sends a **corrupted** `TxnRequest`.
3. On the server, the corrupted request either fails to parse in
`RoutingHeaderProxyInterceptor` (which then forwards the already-drained stream
— see #4802) or reaches `RangeStoreServiceImpl.txn` as an empty request with
`rangeId=0`, which responds gRPC-OK with `code=BAD_REQUEST` and
`succeeded=false`.
4. `KvUtils.newKvTxnResult` only reads `isSucceeded()` and never checks the
response code, so the server error surfaces as `TxnResult.isSuccess() == false`
— indistinguishable from a legitimately failed compare, with no error logged
anywhere on the client.
The client-side routing variant of the test passes because
`TxnRequestProcessor.processResponse` fails loudly on non-SUCCESS codes and
only retries on guaranteed-not-applied NOT_FOUND — but it carries the same
latent request-reuse bug on those retries.
### Changes
- `PByteBufSimpleTableImpl.TxnImpl` and `PByteBufTableRangeImpl.TxnImpl`:
store the compare/success/failure ops and build a fresh `TxnRequest` for every
RPC attempt, mirroring the existing put/get/delete/increment pattern. Op
buffers stay retained until commit completes, as before.
- `TxnRequestProcessor`: accept a `Supplier<TxnRequest>` so each attempt
gets a freshly built request.
- `PByteBufSimpleTableImpl` txn response handling: surface non-SUCCESS
response codes as `InternalServerException` (matching
`TxnRequestProcessor.processResponse`) instead of conflating server errors with
a failed compare.
- Regression tests:
`TxnRequestProcessorTest#testRequestRebuiltForEachAttempt` and a new
`PByteBufSimpleTableImplTest`. Both fail against the previous implementation —
the retry test errors on the corrupted second request, and the conflation test
observes `isSuccess() == false` instead of an exception.
Verified with `mvn -pl stream/clients/java/kv test -DstreamTests`: 21/21
pass; checkstyle clean.
### Follow-ups (intentionally out of scope)
- #4802 fixes the proxy interceptor's drained-stream forwarding on parse
failure — complementary server-side hardening for the same incident class.
- The remaining client-side request processors (`PutRequestProcessor`,
`RangeRequestProcessor`, `DeleteRequestProcessor`, `IncrementRequestProcessor`)
also reuse a pre-built request across NOT_FOUND retries and should get the same
supplier treatment.
- The simple client's put/get/delete/increment result conversions
(`KvUtils.newPutResult` etc.) still ignore response codes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
