KAFKA-4357: Fix consumer group describe output when there is no active member (old consumer) #2075
Conversation
| @@ -83,12 +83,13 @@ object ConsumerGroupCommand extends Logging { | |||
| printAssignment(assignments, !opts.useOldConsumer) | |||
| case Some("Stable") => | |||
| printAssignment(assignments, !opts.useOldConsumer) | |||
| case None => | |||
There was a problem hiding this comment.
Minor: any reason to change the ordering here. It's better to have the case None as the last block in here.
There was a problem hiding this comment.
Thanks for your feedback. I changed the order so the last one in the list (Some(other)) is the one we should never reach. With None we may still have to do some processing. With that in mind, do you still think None should go to the bottom?
There was a problem hiding this comment.
@vahidhashemian can you add a comment over there. +1
There was a problem hiding this comment.
@harshach Sure, I assume you mean putting my response above in a code comment above. Thanks.
There was a problem hiding this comment.
It's a little odd to match on the state for old consumer at all since it's always None. Would it make sense to handle the old consumer case separately prior to matching?
There was a problem hiding this comment.
Sure, I'll try to do that (separate handling of describe for old and new consumer) later today. Thanks.
There was a problem hiding this comment.
@hachikuji I restructured that section of the code. It should address @harshach's concern too. Please let me know if you see issues with the updated code. Thanks.
|
@hachikuji This is the PR for the bug we discussed (under PR for KAFKA-3853) on Friday. Would appreciate your feedback, and opinion about the comment by @harshach. Thanks. |
vahidhashemian
force-pushed
the
KAFKA-4357
branch
from
b098ff4
to
8095bad
Compare
| // A valid state value is required for new consumer only, but this is still a valid case if old consumer is used | ||
| if (!opts.useOldConsumer) | ||
| throw new KafkaException("Expected a valid consumer group state, but none found.") | ||
| printAssignment(assignments, false) | ||
| case Some(other) => |
There was a problem hiding this comment.
If we should never reach here, maybe we can use case _?
There was a problem hiding this comment.
Actually, never mind. It's helpful to know the specific state.
…ctive member (old consumer)
vahidhashemian
force-pushed
the
KAFKA-4357
branch
from
8095bad
to
f76c5e0
Compare
|
LGTM. Not sure what's going on with jenkins, but I'll try to build locally. |
No description provided.