Hi all, Thanks for the feedback.
I have updated the "Compatibility, Deprecation, and Migration Plan" section to document this to support the rollback. I probably should have handled this change, as small as it looks, as a new KIP to avoid this issue. I like Colin's idea about asking for confirmation, although I'm not sure if another tool has already this behavior and could create more confusion (e.g. why this command ask for confirmation and others don't). Maybe we will require a more broad looks at the CLI tools to agree on this? Jorge. El jue., 22 feb. 2018 a las 21:09, Guozhang Wang (<wangg...@gmail.com>) escribió: > Yup, agreed. > > On Thu, Feb 22, 2018 at 11:46 AM, Ismael Juma <ism...@juma.me.uk> wrote: > > > Hi Guozhang, > > > > To clarify my comment: any change with a backwards compatibility impact > > should be mentioned in the "Compatibility, Deprecation, and Migration > Plan" > > section (in addition to the deprecation period and only happening in a > > major release as you said). > > > > Ismael > > > > On Thu, Feb 22, 2018 at 11:10 AM, Guozhang Wang <wangg...@gmail.com> > > wrote: > > > > > Just to clarify, the KIP itself has mentioned about the change so the > PR > > > was not un-intentional: > > > > > > " > > > > > > 3. Keep execution parameters uniform between both tools: It will > execute > > by > > > default, and have a `dry-run` parameter just show the results. This > will > > > involve change current `ConsumerGroupCommand` to change execution > > options. > > > > > > " > > > > > > We were agreed that the proposed change is better than the current > > status, > > > since may people not using "--execute" on consumer reset tool were > > actually > > > surprised that nothing gets executed. What we were concerning as a > > > hind-sight is that instead of doing such change in a minor release like > > > 1.1, we should consider only doing that in the next major release as it > > > breaks compatibility. In the past when we are going to remove / replace > > > certain option we would first add a going-to-be-deprecated warning in > the > > > previous releases until it was finally removed. So Jason's suggestion > is > > to > > > do the same: we are not reverting this change forever, but trying to > > delay > > > it after 1.1. > > > > > > > > > Guozhang > > > > > > > > > On Thu, Feb 22, 2018 at 10:56 AM, Colin McCabe <cmcc...@apache.org> > > wrote: > > > > > > > Perhaps, if the user doesn't pass the --execute flag, the tool should > > > > print a prompt like "would you like to perform this reset?" and wait > > for > > > a > > > > Y / N (or yes or no) input from the command-line. Then, if the > > --execute > > > > flag is passed, we skip this. That seems 99% compatible, and also > > > > accomplishes the goal of making the tool less confusing. > > > > > > > > best, > > > > Colin > > > > > > > > > > > > On Thu, Feb 22, 2018, at 10:23, Ismael Juma wrote: > > > > > Yes, let's revert the incompatible changes. There was no mention of > > > > > compatibility impact on the KIP and we should ensure that is the > case > > > for > > > > > 1.1.0. > > > > > > > > > > Ismael > > > > > > > > > > On Thu, Feb 22, 2018 at 9:55 AM, Jason Gustafson < > ja...@confluent.io > > > > > > > wrote: > > > > > > > > > > > I know it's a been a while since this vote passed, but I think we > > > need > > > > to > > > > > > reconsider the incompatible changes to the consumer reset tool. > > > > > > Specifically, we have removed the --execute option without > > > deprecating > > > > it > > > > > > first, and we have changed the default behavior to execute rather > > > than > > > > do a > > > > > > dry run. The latter in particular seems dangerous since users who > > > were > > > > > > previously using the default behavior to view offsets will now > > > suddenly > > > > > > find the offsets already committed. As far as I can tell, this > > change > > > > was > > > > > > done mostly for cosmetic reasons. Without a compelling reason, I > > > think > > > > we > > > > > > should err on the side of maintaining compatibility. At a > minimum, > > if > > > > we > > > > > > really want to break compatibility, we should wait for the next > > major > > > > > > release. > > > > > > > > > > > > Note that I have submitted a patch to revert this change here: > > > > > > https://github.com/apache/kafka/pull/4611. > > > > > > > > > > > > Thoughts? > > > > > > > > > > > > Thanks, > > > > > > Jason > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Nov 14, 2017 at 3:26 AM, Jorge Esteban Quilcate Otoya < > > > > > > quilcate.jo...@gmail.com> wrote: > > > > > > > > > > > > > Thanks to everyone for your feedback. > > > > > > > > > > > > > > KIP has been accepted and discussion is moved to PR. > > > > > > > > > > > > > > Cheers, > > > > > > > Jorge. > > > > > > > > > > > > > > El lun., 6 nov. 2017 a las 17:31, Rajini Sivaram (< > > > > > > rajinisiva...@gmail.com > > > > > > > >) > > > > > > > escribió: > > > > > > > > > > > > > > > +1 (binding) > > > > > > > > Thanks for the KIP, Jorge. > > > > > > > > > > > > > > > > Regards, > > > > > > > > > > > > > > > > Rajini > > > > > > > > > > > > > > > > On Tue, Oct 31, 2017 at 9:58 AM, Damian Guy < > > > damian....@gmail.com> > > > > > > > wrote: > > > > > > > > > > > > > > > > > Thanks for the KIP - +1 (binding) > > > > > > > > > > > > > > > > > > On Mon, 23 Oct 2017 at 18:39 Guozhang Wang < > > wangg...@gmail.com > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Thanks Jorge for driving this KIP! +1 (binding). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Guozhang > > > > > > > > > > > > > > > > > > > > On Mon, Oct 16, 2017 at 2:11 PM, Bill Bejeck < > > > > bbej...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > Bill > > > > > > > > > > > > > > > > > > > > > > On Fri, Oct 13, 2017 at 6:36 PM, Ted Yu < > > > yuzhih...@gmail.com > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Oct 13, 2017 at 3:32 PM, Matthias J. Sax < > > > > > > > > > > matth...@confluent.io> > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > +1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 9/11/17 3:04 PM, Jorge Esteban Quilcate Otoya > > wrote: > > > > > > > > > > > > > > Hi All, > > > > > > > > > > > > > > > > > > > > > > > > > > > > It seems that there is no further concern with > the > > > > KIP-171. > > > > > > > > > > > > > > At this point we would like to start the voting > > > > process. > > > > > > > > > > > > > > > > > > > > > > > > > > > > The KIP can be found here: > > > > > > > > > > > > > > https://cwiki.apache.org/ > > > confluence/display/KAFKA/KIP- > > > > > > > > > > > > > > 171+-+Extend+Consumer+Group+Reset+Offset+for+Stream+ > > > > > > > Application > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > -- Guozhang > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > -- Guozhang > > > > > > > > > -- > -- Guozhang >
