lianetm commented on code in PR #15579:
URL: https://github.com/apache/kafka/pull/15579#discussion_r1536155346


##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/MembershipManagerImpl.java:
##########
@@ -1010,49 +1015,55 @@ private void revokeAndAssign(LocalAssignment 
resolvedAssignment,
         // and assignment, executed sequentially).
         CompletableFuture<Void> reconciliationResult =
             revocationResult.thenCompose(__ -> {
-                boolean memberHasRejoined = memberEpochOnReconciliationStart 
!= memberEpoch;
-                if (state == MemberState.RECONCILING && !memberHasRejoined) {
+                if (!maybeAbortReconciliation()) {
                     // Apply assignment
                     return assignPartitions(assignedTopicIdPartitions, 
addedPartitions);
                 } else {
-                    log.debug("Revocation callback completed but the member 
already " +
-                        "transitioned out of the reconciling state for epoch 
{} into " +
-                        "{} state with epoch {}. Interrupting reconciliation 
as it's " +
-                        "not relevant anymore,", 
memberEpochOnReconciliationStart, state, memberEpoch);
-                    String reason = interruptedReconciliationErrorMessage();
                     CompletableFuture<Void> res = new CompletableFuture<>();
                     res.completeExceptionally(new KafkaException("Interrupting 
reconciliation" +

Review Comment:
   uhm good point, I did consider it but wasn't fully convinced at that time 
(mostly taking into account that this abort path is not only in the case of 
rejoin while reconciling, but also in the case of fatal failures while 
reconciling, for instance). But I do like you point of view about the 2 paths, 
seeing from the POV that even in the failure scenario, the logging/error 
handling should be done on that fatal transition path, and the reconciliation 
itself could only be responsible for aborting quietly. Changed, take a look and 
let me know your thoughts (note that the tradeoff is that it requires another 
check before sending the ack, to make sure that the reconciliation hasn't been 
already aborted) 



-- 
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: jira-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to