artemlivshits commented on PR #21735:
URL: https://github.com/apache/kafka/pull/21735#issuecomment-6203441321
Thanks for the detailed review, @junrao. I merged trunk and pushed changes
for all the comments. Here's a summary; the PR description is also updated to
explain the dual-identity model.
**Merge with trunk** (69f9f51ccc and follow-ups)
- `lastProducerEpoch` is now persisted (KAFKA-20357), so your failover
question on `lastProducerEpoch` is addressed on trunk.
- KAFKA-20785 (`PRODUCER_FENCED` for CompleteAbort + Commit retry) is ported
into the new TV2 EndTxn logic.
- The Scala `TransactionsTest` / `TransactionMetadataTest` were rewritten in
Java on trunk, so the 2PC integration tests now live in a separate
`TransactionsWith2PCTest`. That also means the unstable API / feature versions
are no longer enabled for the other transaction tests.
**Dual identity.** After InitProducerId(keepPreparedTxn=true), the
client-facing identity (nextProducerId / nextProducerEpoch) is the identity of
the current state. producerId / producerEpoch are frozen and used only for the
markers. Fixes that apply this consistently:
- dca23702cf: the epoch fence (InitProducerId(keepPreparedTxn=false) and
transaction timeout) and the InitProducerId expected-identity check now use the
client-facing identity. Previously the fence failed after more than one
keepPrepared call or a client producer id rotation, and left the transactional
id stuck in PREPARE_EPOCH_FENCE.
- 132326404a: AddPartitions on a kept transaction returns INVALID_TXN_STATE
for any identity, including the frozen one (your comment on the
hasNextProducerEpoch ordering).
- 1353d43726: EndTxn with TV1 on a kept transaction returns
INVALID_TXN_STATE. Previously an old client holding the frozen identity could
complete it.
- c699e0964b: keepPreparedTxn=true is rejected (INVALID_TXN_STATE) if the
client's 2PC setting doesn't match the ongoing transaction. The kept
transaction's timeout, which determines whether it's 2PC, carries over to the
producer's subsequent transactions.
- 5060ebcdac (EndTxn):
- The retry conditions only apply in COMPLETE_* states, which makes the
producer id check strict elsewhere (your stricter-check comment).
- retryOnOverflow requires a valid producer id (your `prevProducerId !=
-1` comment).
- The PREPARE_* case throws IllegalStateException.
- The fence condition is rewritten as you suggested, and the comments are
clarified.
- For a kept transaction, prevProducerId is now the client-facing producer
id. This fixes a retry after the producer id was rotated both by
keepPreparedTxn and on overflow.
**Naming, comments and checks** (c32a84de1e)
- `isProducerEpochExhausted` → `isClientFacingEpochExhausted`.
- ONGOING state check in `prepareIncrementProducerEpochForOngoing`.
- `validProducerEpoch`: javadoc, indentation, `transitTxnState` rename, and
the redundant check removed.
- `setClientTransactionVersion` moved into the `logVersion >= 1` block.
Writing it at version 0 actually threw UnsupportedVersionException.
- keepPreparedTxn=true/false wording, and the comments on the post-append
check. That function is TV2-only: the second epoch condition covers an abort at
Short.MaxValue, which doesn't bump the epoch.
**Tests** (c66ae4bc2e, b7a3127477, 580b5d613b)
- Addressed all your test comments:
- the captor and explicit epoch;
- the epoch boundary values (32766);
- the redundant and tautological asserts, replaced with a real transaction
log round trip and live state checks;
- TV_1 coverage;
- keepPreparedTxn without an ongoing transaction is now a positive test
(the old test passed only because 2PC wasn't enabled in the mock);
- `inLock`, the exactly-one rotation assert, and the describe-epoch
assertion (it now checks that the client-facing epoch survives a coordinator
restart and the frozen epoch is unchanged).
- Added tests for each fix above.
- Restored the uninitialized epoch in the KAFKA-14880 regression test, which
an earlier merge had changed.
- Sped up the 2PC crash and recovery integration tests: 3 tests at about 16s
each, previously one test at about 90s that needed an extended timeout.
For the comments where I suggested not making a change (prevProducerId in
`retryOnEpochBump` and in the current-epoch check, `transactionStarted` reset,
and making the 2PC config internal), my replies are in the corresponding
threads.
--
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]
