Hi Guozhang, Removing the script-extention from the KIP is good to me. +1 from me, too.
Thank you. Luke On Thu, Jul 7, 2022 at 7:39 AM Guozhang Wang <wangg...@gmail.com> wrote: > Hello folks, > > I tried to implement the script-extension along with the unit test coverage > on DescribeConsumerGroupTest, but it turns out more complicated than I > anticipated due to the fact that we need to make sure the `require-stable` > flag is only effective for describing consumers. This also makes me feeling > that maybe my original thought of just adding that option for "--describe" > may not be comprehensive and we may need to think through under which > action the additional option should be allowed. So I would remove this part > of the KIP and continue the voting thread. > > I've updated the KIP addressing the renaming suggestion. And I will close > this thread as accepted with three binding votes (John, David, Bruno) if I > don't hear from you for more suggestions. > > Thanks, > Guozhang > > > > On Mon, Jul 4, 2022 at 1:57 AM Bruno Cadonna <cado...@apache.org> wrote: > > > Hi, > > > > I would prefer to not include the script-extension into the KIP if you > > you cannot commit to its implementation. I think partially implemented > > KIPs make release management harder. If we can avoid implementing KIPs > > partially, we should do it. > > > > I am +1 either way. I just wanted to bring this up. > > > > Best, > > Bruno > > > > On 04.07.22 04:37, Luke Chen wrote: > > > Hi Guozhang, > > > > > >> We can add it into this proposal though I could not commit to > > implementing > > > it myself with all the `DescribeConsumerGroupTest` coverage after it's > > > accepted, instead I could add a JIRA ticket under this KIP for others > > who's > > > interested to chime in. What do you think? > > > > > > Sounds good to me. > > > > > > Thank you. > > > Luke > > > > > > On Mon, Jul 4, 2022 at 3:44 AM Guozhang Wang <wangg...@gmail.com> > wrote: > > > > > >> Thanks for folks for your input ! > > >> > > >> 1) I'm happy to change the setter names to be consistent with the > > >> topicPartition ones. I used a different name for getter from setter > as I > > >> remember seeing some other options differentiating function names for > > >> getter and setters, while some other options seem to be more on just > > >> keeping the names the same. After getting your feedback I think it's > > better > > >> to do the same for both getter / setters. > > >> > > >> 2) For the kafka-consumer-group.sh tool, I looked at > > >> ConsumerGroupCommand#describeGroups, and I think it's appropriate to > add > > >> it. I'm planning to add it in the shell tool as: > > >> > > >> ``` > > >> "Require brokers to hold on returning unstable offsets (due to pending > > >> transactions) but retry until timeout for stably committed offsets" > > >> "Example: --bootstrap-server localhost:9092 --describe --group group1 > > >> --offsets --require-stable" > > >> ``` > > >> > > >> We can add it into this proposal though I could not commit to > > implementing > > >> it myself with all the `DescribeConsumerGroupTest` coverage after it's > > >> accepted, instead I could add a JIRA ticket under this KIP for others > > who's > > >> interested to chime in. What do you think? > > >> > > >> > > >> Guozhang > > >> > > >> > > >> On Fri, Jul 1, 2022 at 1:18 AM David Jacot > <dja...@confluent.io.invalid > > > > > >> wrote: > > >> > > >>> Hi Guozhang, > > >>> > > >>> Thanks for the KIP! > > >>> > > >>> I agree with Luke. `requireStable` seems more consistent. > > >>> > > >>> Regarding the kafka-consumer-group command line tool, I wonder if > > >>> there is real value in doing it. We don't necessarily have to add all > > >>> the options to it but we could if it is proven to be useful. Anyway, > I > > >>> would leave it for a future KIP. > > >>> > > >>> +1 (binding) > > >>> > > >>> Best, > > >>> David > > >>> > > >>> On Fri, Jul 1, 2022 at 9:47 AM Bruno Cadonna <cado...@apache.org> > > wrote: > > >>>> > > >>>> Hi Guozhang, > > >>>> > > >>>> thank you for the KIP! > > >>>> > > >>>> I do not have strong feelings about the naming of the getter, but I > > >> tend > > >>>> to agree with Luke. > > >>>> > > >>>> Regarding, the adaptation of the kafka-consumer-group.sh script, I > am > > >>>> fine if we leave that for a future KIP. > > >>>> > > >>>> +1 (binding) > > >>>> > > >>>> Best, > > >>>> Bruno > > >>>> > > >>>> On 01.07.22 06:05, Luke Chen wrote: > > >>>>> Hi Guozhang, > > >>>>> > > >>>>> Thanks for the KIP. > > >>>>> Some comments: > > >>>>> 1. I have the same question as Ziming, should we also add an option > > >> in > > >>>>> kafka-consumer-groups.sh in this KIP? > > >>>>> Or you'd like to keep the current scope, and other people can > create > > >> a > > >>>>> follow-up KIP to address the kafka-consumer-groups.sh script? > > >>>>> 2. The setter method name: `shouldRequireStable` might need to > rename > > >>> to > > >>>>> `requireStable` to be consistent with above `topicPartitions` > > >>> getter/setter > > >>>>> > > >>>>> Thank you. > > >>>>> Luke > > >>>>> > > >>>>> On Fri, Jul 1, 2022 at 11:17 AM John Roesler <vvcep...@apache.org> > > >>> wrote: > > >>>>> > > >>>>>> Thanks for the KIP, Guozhang! > > >>>>>> > > >>>>>> I’m +1 (binding) > > >>>>>> > > >>>>>> -John > > >>>>>> > > >>>>>> On Thu, Jun 30, 2022, at 21:17, deng ziming wrote: > > >>>>>>> Thanks for this KIP, > > >>>>>>> we have a kafka-consumer-groups.sh shell which is based on the > API > > >>> you > > >>>>>>> proposed to change, is it worth update it as well? > > >>>>>>> > > >>>>>>> -- > > >>>>>>> Best, > > >>>>>>> Ziming > > >>>>>>> > > >>>>>>>> On Jul 1, 2022, at 9:04 AM, Guozhang Wang <wangg...@gmail.com> > > >>> wrote: > > >>>>>>>> > > >>>>>>>> Hello folks, > > >>>>>>>> > > >>>>>>>> I'd like to call out for a vote for the following KIP to expose > > >> the > > >>>>>>>> requireStable flag inside admin client's options as well: > > >>>>>>>> > > >>>>>>>> > > >>>>>> > > >>> > > >> > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-851%3A+Add+requireStable+flag+into+ListConsumerGroupOffsetsOptions > > >>>>>>>> > > >>>>>>>> Any feedback as well as your votes are welcome. > > >>>>>>>> > > >>>>>>>> -- Guozhang > > >>>>>> > > >>>>> > > >>> > > >> > > >> > > >> -- > > >> -- Guozhang > > >> > > > > > > > > -- > -- Guozhang >
