KAFKA-7141: ConsumerGroupCommand should describe group assignment eve… #5356
Conversation
…n with no offsets committed. https://issues.apache.org/jira/browse/KAFKA-7141 Currently, if a consumer group never commits offsets, ConsumerGroupCommand cannot describe it at all even if the member assignment is valid. Instead, the tool should be able to describe the group information showing empty current_offset and LAG.
…n-commit-offset groups
|
@vahidhashemian Please review this patch. Thanks. |
|
retest it please. |
There was a problem hiding this comment.
@huxihx thanks for making this improvement. I just had a minor comment inline. Otherwise, LGTM.
| abstract class AbstractConsumerRunnable(broker: String, groupId: String) extends Runnable { | ||
| val props = new Properties | ||
| abstract class AbstractConsumerRunnable(broker: String, groupId: String, customPropsOpt: Option[Properties] = None) extends Runnable { | ||
| val props = if (customPropsOpt.isDefined) new Properties(customPropsOpt.get) else new Properties |
There was a problem hiding this comment.
Wouldn't it be better to move this line after configure(props) below so we also provide a way to override the default configs? In other words, this line would remain unchanged, and after configure(props) we add
if (customPropsOpt.isDefined)
props.putAll(customPropsOpt.get)
|
@vahidhashemian Thanks for the comments, please review again. |
|
LGTM, thanks for addressing my comment. |
|
@hachikuji Could you take some time to review this patch? Thanks. |
|
With Vahid's previous KIP, I think the intention is that the default behavior of |
|
I think this PR fixes the situation where consumers have joined the group but have not committed any offset yet. Current implementation does not report any rows until they commit offsets, which I think does not provide a good user experience. It seems to me that this PR still follows the definition of
Please advise if I'm missing something. |
|
Ok, that makes sense. |
| val props = new Properties | ||
| configure(props) | ||
| if (customPropsOpt.isDefined) |
|
@hachikuji Thanks for the comments. Please review again. |
|
retest it please. |
|
retest this please |
|
Thanks @huxihx. I will merge in the morning after the build finishes. |
|
retest this please |
1 similar comment
|
retest this please |
|
FAILURE |
|
@huxihx The build failure on jdk10 seems deterministic. Can you look into it? |
|
@hachikuji Seems it hit a known Scala bug opened by Ijuma (scala/bug#10418). I will take care of it. |
|
retest this please |
|
@huxihx Thanks for fixing the issue. Merged to trunk. |
…n with no offsets committed.
https://issues.apache.org/jira/browse/KAFKA-7141
Currently, if a consumer group never commits offsets, ConsumerGroupCommand cannot describe it at all even if the member assignment is valid. Instead, the tool should be able to describe the group information showing empty current_offset and LAG.
More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)