Hi PoAn, Thanks for the KIP! I have a few minor comments/suggestions:
DJ1: In the output of `--describe --verbose`, I would suggest putting `GROUP-EPOCH` and `TARGET-ASSIGNMENT-EPOCH` before `#MEMBERS`. DJ2: Continuing on the above, I assume that we will print out N/A for the fields not supported by classic groups. Is this correct? DJ3: In the output of `--describe --members --verbose`, I wonder if we should use the following order and terms: CURRENT-EPOCH CURRENT-ASSIGNMENT TARGET-EPOCH TARGET-ASSIGNMENT . I would remove the GROUP-EPOCH because it is already in the group description. The value `(0)` used for the assignment is incorrect. Here I suppose that we will print out the list of partitions in the form of `topicid-partitionid`. DJ4: Continuing on the above, I wonder if we should also add the protocol used `classic` or `consumer`. For context, it is possible to have `classic` members and `consumer` members in a `consumer` group during an online upgrade from the classic protocol to the consumer protocol. Having this information may be handy for administrators. What do you think? DJ5: In the output of `--describe --offsets --verbose`, I would suggest putting `LEADER-EPOCH` closer to `CURRENT-OFFSET`. It would be great if we could get this one in AK 4.0 as it changes the output of the command. Thanks, DJ On Fri, Nov 1, 2024 at 7:40 AM Frank Yang <yangp...@gmail.com> wrote: > Hi Sean / Andrew / Lianet, > > Thanks for all your review and suggestions. > > AS1, LM1, LM4: Change to add KIP-848 information when users give —verbose > option. > —describe —verbose: shows group epoch and target assignment epoch. > —describe —members —verbose: shows above information, member epoch, and > target assignment. > > AS2: Change to use MEMBER-EPOCH to align with KIP-848 definition. > > LM2: For classic group, it doesn’t have epoch, so I use Optional fields in > ConsumerGroupDescription. > For share group, it relies on KIP-848. It must have epoch, so I use int > fields in ShareGroupDescription. > > LM3: Remove —state change. We can get group level information by —describe > —verbose. > > SQ1: Add LEADER-EPOCH when users give —describe —offsets —verbose. > > Thanks. > PoAn > > > On Nov 1, 2024, at 5:08 AM, Lianet M. <liane...@gmail.com> wrote: > > > > Hello Frank, thanks for the KIP! A few comments: > > > > LM1. I strongly agree with Andrew's suggestion of moving this into a > > --verbose option. I expect someone would only attempt to make sense of > the > > epochs while debugging an issue, not while trying to view the group or > > member's info. (Also member-epoch makes more sense to me, even more if we > > end up agreeing in a --verbose). Related to both issues, my take is that > > whoever is close to the protocol will expect member-epoch, whoever is not > > will probably won't even need to see the epochs at all. > > > > LM2. Why are the epochs defined as Optional (don't we expect to always > have > > them? with 0 initially, defined on the broker side for the group ones, > and > > client side for the member) > > > > LM3. Why is the KIP including the –state option in the proposed changes? > ( > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=327977411#KIP1099:Extendkafkaconsumergroupscommandlinetooltosupportnewconsumergroup---state > > ). > > I get that the output would change in that example, but it’s not because > of > > any change to the –state option itself. It's because of the change > proposed > > to the –described (required with the --state), and the changes to > > --describe are already explained above (seems confusing, got me looking > for > > a change to the state filter which seemed unrelated). > > > > LM4. Group epoch and target assignment epoch are conceptually at the > group > > level. vs member epoch that lands at a member level. So wonder if we > should > > show them accordingly (ex. using the --verbose option) > > –describe –verbose => shows group epoch and target assignment epoch > > –describe –members –verbose => shows member epoch (along > > with group epoch and target assignment epoch) > > > > Thanks! > > > > Lianet > > > > On Thu, Oct 31, 2024 at 7:30 AM Andrew Schofield < > > andrew_schofield_j...@outlook.com> wrote: > > > >> Hi PoAn, > >> Thanks for the KIP. I have a few comments. > >> > >> AS1: It seems to me that these new additions are most useful to people > >> trying to understand > >> the progress of rebalancing in quite some detail. The information is > >> really not understandable > >> for most users who do not have deep knowledge of KIP-848/932. > >> > >> As a result, I suggest for kafka-share-groups.sh that you add a > --members > >> --verbose option > >> and only include the new information in the output for that option, > rather > >> than changing the > >> non-verbose --members output. > >> > >> I also make a similar suggestion for kafka-consumer-groups.sh --members > >> and only add the > >> new information for the --verbose output. > >> > >> AS2: I strongly suggest that you use MEMBER-EPOCH instead of > >> CONSUMER-EPOCH. > >> I think it's more confusing not following the terminology of the KIPs. > For > >> one thing, > >> we already have "member" in the admin client such as MemberDescription. > >> It's not a > >> new term. > >> > >> > >> Thanks, > >> Andrew > >> ________________________________________ > >> From: PoAn Yang <pay...@apache.org> > >> Sent: 25 October 2024 13:55 > >> To: dev@kafka.apache.org <dev@kafka.apache.org> > >> Subject: Re: [DISCUSS] KIP-1099: Extend kafka-consumer-groups command > line > >> tool to support new consumer group > >> > >> Hi Lucas, > >> > >> Thanks for the review! > >> > >> 1) Yes, I add related change for kafka-share-groups.sh to the KIP. Could > >> you take a look? Thanks for the suggestion. > >> > >> 2) We use CONSUMER-ID as member ID. If we use MEMBER-EPOCH here, users > may > >> confuse what is different between CONSUMER and MEMBER. > >> > >> Thanks, > >> PoAn > >> > >> On 2024/10/23 13:28:17 Lucas Brutschy wrote: > >>> Hi Frank, > >>> > >>> thanks for the KIP! > >>> > >>> 1) For consistency, should we do the same for > >>> kafka-share-groups.sh, ShareGroupDescription, etc. ? Even if we do not > >>> implement it right now if the share group implementation may still be > >>> incomplete, it may make sense to include it in the KIP. > >>> > >>> 2) Why call it CONSUMER-EPOCH, not MEMBER-EPOCH? That would seem more > >>> consistent. > >>> > >>> Cheers, > >>> Lucas > >>> > >>> On Wed, Oct 23, 2024 at 2:41 PM Frank Yang <yangp...@gmail.com> wrote: > >>> > >>>> Hi all, > >>>> > >>>> I would like to kick off the discussion of KIP-1099. This KIP enhances > >> the > >>>> kafka-consumer-groups tools to include state which is introduced by > >> KIP-848. > >>>> > >>>> KIP-1099: Extend kafka-consumer-groups command line tool to support > new > >>>> consumer group - Apache Kafka - Apache Software Foundation > >>>> < > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1099%3A+Extend+kafka-consumer-groups+command+line+tool+to+support+new+consumer+group > >>> > >>>> cwiki.apache.org > >>>> < > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1099%3A+Extend+kafka-consumer-groups+command+line+tool+to+support+new+consumer+group > >>> > >>>> [image: favicon.ico] > >>>> < > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1099%3A+Extend+kafka-consumer-groups+command+line+tool+to+support+new+consumer+group > >>> > >>>> < > >> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1099%3A+Extend+kafka-consumer-groups+command+line+tool+to+support+new+consumer+group > >>> > >>>> > >>>> Thank you, > >>>> PoAn > >>>> > >>> > >> > >
