ableegoldman commented on a change in pull request #9627:
URL: https://github.com/apache/kafka/pull/9627#discussion_r618821079
##########
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/distributed/WorkerGroupMember.java
##########
@@ -202,7 +202,7 @@ public void requestRejoin() {
}
public void maybeLeaveGroup(String leaveReason) {
- coordinator.maybeLeaveGroup(leaveReason);
+ coordinator.maybeLeaveGroup(leaveReason, true);
Review comment:
The only invocation of `WorkerGroupMember#maybeLeaveGroup` in fact
already does log a warning as to why instead of relying on `maybeLeaveGroup` to
do so. Imo we should do something similar for the "consumer poll timeout has
expired" case
##########
File path:
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java
##########
@@ -1023,9 +1023,14 @@ protected void close(Timer timer) {
}
/**
+ * Leaving the group. This method also sends LeaveGroupRequest and log
{@code leaveReason} if this is dynamic members
+ * or unknown coordinator or state is not UNJOINED or this generation has
a valid member id.
Review comment:
I think it may be more useful to describe the cases where it will _not_
send a LeaveGroup and describe what this actually means (also it should have
been 'and' not 'or' in the original):
```suggestion
* Sends LeaveGroupRequest and logs the {@code leaveReason}, unless this
member is using
* static membership or is already not part of the group (ie does not
have a valid member id,
* is in the UNJOINED state, or the coordinator is unknown).
```
##########
File path:
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java
##########
@@ -1386,7 +1400,7 @@ public void run() {
"the poll loop is spending
too much time processing messages. " +
"You can address this
either by increasing max.poll.interval.ms or by reducing " +
"the maximum size of
batches returned in poll() with max.poll.records.";
- maybeLeaveGroup(leaveReason);
+ maybeLeaveGroup(leaveReason, true);
Review comment:
I think it would be simpler to just log the current `leaveReason` right
here at the warn level, and then pass in a more brief description to
`maybeLeaveGroup` rather than add a flag to that method just for this one case
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
