lianetm commented on code in PR #16686:
URL: https://github.com/apache/kafka/pull/16686#discussion_r1834529057
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractMembershipManager.java:
##########
@@ -549,7 +576,7 @@ public CompletableFuture<Void> leaveGroup() {
CompletableFuture<Void> leaveResult = new CompletableFuture<>();
leaveGroupInProgress = Optional.of(leaveResult);
- CompletableFuture<Void> callbackResult = signalMemberLeavingGroup();
+ CompletableFuture<Void> callbackResult =
signalMemberLeavingGroup(isClosing);
Review Comment:
I wonder if we could avoid propagating the isClosing/runCallbacks to this
func that is really about "invokeCallbacks" (we're effectively calling an
"invokeCallbacks" func, passing a param "runCallback" that can be false). This
seems confusing and could lead to misleading logs I would say (ie. we would end
up with the log line "Member completed callbacks..." when it really didn't do
that here).
So one idea that comes to mind is to shortcircuit before the
signalMemberLeavingGroup:
```
if (isClosing/runCallbacks) {
CompletableFuture<Void> callbackResult =
signalMemberLeavingGroup();
callbackResult.whenComplete((result, error) -> {
// log callback completion
...
clearAssignmentAndSendLeave(); // encapsulate what we do
after callbacks
});
} else {
clearAssignmentAndSendLeave();
}
```
What do you think?
--
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
