Hello PoAn, just of couple more minor comments: LM4. Regarding the new “protocol” field added to MemberDescription. Should we consider reusing the existing GroupProtocol enum instead of String? (It’s the one we use from the consumer side to refer to the protocol in use, just missing Share I notice).
LM5. Regarding the change to the output for kafka-consumer-groups, the command shown does not include the —state option, but the output shows state info (state, #members, epochs). I would guess that we want to modify the output only when we describe a group with the —state —verbose option, is my understanding right? If my understanding is right we’re just missing adding the —state in the example, and the KIP does not introduce any changes to the —describe —verbose option. (If not, it would mean a bigger change to the output of —describe —verbose which I expect is not the intention?) Thanks! Lianet On Wed, Nov 20, 2024 at 2:12 AM PoAn Yang <yangp...@gmail.com> wrote: > Hi Chia-Ping, > > Thanks for the review and suggestions. > > CI0: Thanks for the reminder. Update validVersions in > ConsumerGroupDescribeRequest to 0-1. > > CI1: Yes, I use `ConsumerGroupMember#useClassicProtocol` to check whether > a member in consumer group uses “classic” or “consumer” protocol. > > CI2: Yes, a member in share group always uses “share” protocol. > > CI3: Add a table to show meaning of “classic”, “consumer”, and “share” > protocol. > > BTW, the vote thread is in > https://lists.apache.org/thread/rb25tf75tzf4c7jqqldlo5jh9w8chsq6. > > Thanks, > PoAn > > > On Nov 20, 2024, at 11:46 AM, Chia-Ping Tsai <chia7...@apache.org> > wrote: > > > > hi PoAn > > > > CI0: We have to bump the version of ConsumerGroupDescribeRequest as > well, so server can distinguish the new and old behavior. > > > > CI1: the type of new filed is string, so I guess you plan to use > `ConsumerGroupMember#useClassicProtocol` [0] flag to return either > "classic" or "consumer", right? > > > > CI2: `MemberDescription` is used by `ShareGroupDescription` too, so the > new filed protocol in shared group is always "shared", right? > > > > CI3: Could you consider adding a table to show the value of the protocol > field in each case? Andrew has a beautiful table in KIP-1043 that lists all > possible protocol names. > > > > [0] > https://github.com/apache/kafka/blob/441a6d0b790f4a17b454caeea7588a6b90fbd9db/group-coordinator/src/main/java/org/apache/kafka/coordinator/group/modern/consumer/ConsumerGroupMember.java#L454 > > > > > Best, > > Chia-Ping > > > > On 2024/11/19 15:45:16 PoAn Yang wrote: > >> Hi David, > >> > >> Thanks for the reminder. > >> > >> DJ4: Add related content to the KIP. > >> Bump validVersions in ConsumerGroupDescribeResponse to “0-1”. > >> Add a new field “Protocol” to ConsumerGroupDescribeResponse. > >> > >> Thanks, > >> PoAn > >> > >>> On Nov 19, 2024, at 3:57 PM, David Jacot <dja...@confluent.io.INVALID> > wrote: > >>> > >>> Hi PoAn, > >>> > >>> DJ4: Could you please also include the required changes to the > >>> ConsumerGroupDescribed API in the public interface section? We > basically > >>> need to bump the version, add the new field, etc. > >>> > >>> Thanks, > >>> David > >>> > >>> On Mon, Nov 18, 2024 at 5:40 AM PoAn Yang <yangp...@gmail.com> wrote: > >>> > >>>> Hi David, > >>>> > >>>> DJ4: Add a new field protocol to MemberDescription, > >>>> so the command line tool can show protocol information when users > describe > >>>> members. > >>>> > >>>> If there is no further suggestion, I will start a vote thread today. > >>>> > >>>> Thanks, > >>>> PoAn > >>>> > >>>>> On Nov 15, 2024, at 11:50 PM, David Jacot > <dja...@confluent.io.INVALID> > >>>> wrote: > >>>>> > >>>>> Hi, > >>>>> > >>>>> DJ2: Using "-" sounds good to me. > >>>>> > >>>>> DJ3: That seems reasonable to me. > >>>>> > >>>>> DJ4: Why not add it right now? I don't want to change the output of > the > >>>>> tool too many times. > >>>>> > >>>>> Best, > >>>>> David > >>>>> > >>>>> On Fri, Nov 15, 2024 at 3:23 PM PoAn Yang <yangp...@gmail.com> > wrote: > >>>>> > >>>>>> Hi David / Andrew, > >>>>>> > >>>>>> Thanks for review. Thanks Andrew for picking up > kafka-share-groups.sh > >>>>>> implementation. > >>>>>> I will handle kafka-consumer-groups.sh. > >>>>>> > >>>>>> DJ3: After discussing with @Chia-Ping Tsai, we think that using new > >>>> format > >>>>>> is more clear. > >>>>>> > >>>>>> The new format will be like > >>>>>> > >>>> > <topic-name1>:<partition-id1>,<partition-id2>;<topic-name2>:<partition-id1>,<partition-id2> > >>>>>> > >>>>>> Using colon(:) to concat topic name and partition IDs. > >>>>>> Using comma(,) to concat partition IDs within same topic name. > >>>>>> Using semicolon(;) to concat topic strings. > >>>>>> > >>>>>> AS3: Fix it with kafka-share-groups.sh. Thanks. > >>>>>> > >>>>>> If there is no further suggestion, I will start a vote thread next > >>>> Monday. > >>>>>> > >>>>>> Thanks, > >>>>>> PoAn > >>>>>> > >>>>>>> On Nov 14, 2024, at 9:05 PM, Andrew Schofield < > >>>>>> andrew_schofield_j...@outlook.com> wrote: > >>>>>>> > >>>>>>> Hi PoAn, > >>>>>>> DJ2: I was just going to comment that "-" would be a more > appropriate > >>>>>> missing value, but > >>>>>>> you got there first. > >>>>>>> > >>>>>>> AS3: The examples for kafka-share-groups.sh include > >>>>>> kafka-consumer-groups.sh in the > >>>>>>> command line. > >>>>>>> > >>>>>>> If this is accepted in time, I'm happy to pick up the > implementation of > >>>>>> the share groups > >>>>>>> part of this if it helps. > >>>>>>> > >>>>>>> Thanks, > >>>>>>> Andrew > >>>>>>> > >>>>>>> ________________________________________ > >>>>>>> From: Frank Yang <yangp...@gmail.com> > >>>>>>> Sent: 14 November 2024 10:48 > >>>>>>> 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 David, > >>>>>>> > >>>>>>> Thanks for the review and suggestion! I would like to get this in > AK > >>>> 4.0 > >>>>>> as well. I will do my best. > >>>>>>> > >>>>>>> DJ1: Update KIP to put GROUP-EPOCH and TARGET-ASSIGNMENT-EPOCH > before > >>>>>> #MEMBERS. > >>>>>>> > >>>>>>> DJ2: I prefer to follow current missing column value “-“. > (reference < > >>>>>> > >>>> > https://github.com/apache/kafka/blob/18199028a672fd973ac37bf26316994babc2a6da/tools/src/main/java/org/apache/kafka/tools/consumer/group/ConsumerGroupCommand.java#L92 > >>>>>>> ) > >>>>>>> > >>>>>>> DJ3: Update KIP to use CURRENT-EPOCH CURRENT-ASSIGNMENT > TARGET-EPOCH > >>>>>> TARGET-ASSIGNMENT. > >>>>>>> Remove GROUP-EPOCH. > >>>>>>> > >>>>>>> For assignment value, it follows current output (reference < > >>>>>> > >>>> > https://github.com/apache/kafka/blob/18199028a672fd973ac37bf26316994babc2a6da/tools/src/main/java/org/apache/kafka/tools/consumer/group/ConsumerGroupCommand.java#L413-L418 > >>>>> ). > >>>>>> I think the form of `topicid-partitionid` is more clear. > >>>>>>> If we would like to use this form, I will update both output in > >>>>>> kafka-consumer-groups.sh and kafka-share-groups.sh. > >>>>>>> > >>>>>>> DJ4: It looks like DescribeGroupsResponseData only has protocol > type at > >>>>>> group level. > >>>>>>> Both DescribeGroupsResponseData and > ConsumerGroupDescribeResponseData > >>>>>> don’t have protocol at member level. > >>>>>>> Could we use a followup to add it? > >>>>>>> > >>>>>>> DJ5: Update KIP to put LEADER-EPOCH before CURRENT-OFFSET. > >>>>>>> > >>>>>>> Thanks, > >>>>>>> PoAn > >>>>>>> > >>>>>>>> On Nov 14, 2024, at 3:43 PM, David Jacot > <dja...@confluent.io.INVALID > >>>>> > >>>>>> wrote: > >>>>>>>> > >>>>>>>> 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 > >>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>> > >>>>>> > >>>> > >>>> > >> > >> > >
