artemlivshits commented on code in PR #17849:
URL: https://github.com/apache/kafka/pull/17849#discussion_r1848996598
##########
clients/src/main/java/org/apache/kafka/clients/producer/internals/TransactionManager.java:
##########
@@ -1164,23 +1164,42 @@ private TransactionalRequestResult
handleCachedTransactionRequestResult(
return result;
}
+ /**
+ * Determines if an epoch bump can be triggered manually based on the api
versions.
+ *
+ * <ol>
+ * <li><b>Client-Triggered Epoch Bump</b>:
+ * If the coordinator supports epoch bumping
(initProducerIdVersion.maxVersion() >= 3),
+ * client-triggered epoch bumping is allowed, returns true.
+ * <code>epochBumpTriggerRequired</code> must be set to true in
this case.</li>
+ *
+ * <li><b>No Epoch Bump Allowed</b>:
+ * If the coordinator does not support epoch bumping, returns
false.</li>
+ *
+ * <li><b>Server-Triggered Only</b>:
+ * When TransactionV2 is enabled, epoch bumping is handled
automatically
+ * by the server in EndTxn, so manual epoch bumping is not
required, returns false.</li>
+ * </ol>
+ *
+ * @return true if a client-triggered epoch bump is allowed, otherwise
false.
+ */
// package-private for testing
- boolean canBumpEpoch() {
- if (!isTransactional()) {
- return true;
+ boolean canTriggerEpochBump() {
+ if (!coordinatorSupportsBumpingEpoch) {
+ return false;
}
-
- return coordinatorSupportsBumpingEpoch;
+ // If coordinator supports epoch bumping, client can trigger the bump
manually if TransactionsV2 is disabled.
+ return !isTransactionV2Enabled;
Review Comment:
I think that encapsulating `isTransactionV2Enabled` could lead to subtle
bugs (like I pointed in
https://github.com/apache/kafka/pull/17849/files#r1848951501). The problem is
that canTriggerEpochBump (or whatever name we come up with) really indicates
whether the broker supports the proper version of `InitProducerId` that can be
used by the client to explicitly trigger the epoch. If the broker doesn't
support it, then we cannot use abortable error.
If we have transaction v2, then we still can use abortable error, but don't
need to use explicit epoch bump. This distinction is lost: we have a Boolean
to express 3 cases:
1. No explicit epoch bump + fatal error.
2. Explicit epoch bump + abortable error.
3. No explicit epoch bump + abortable error.
--
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]
