Hi Jorge, Thanks for the KIP. It would be a great to add feature to the reset tools. I made a pass over it and it looks good to me overall. I have a few comments:
1. For all the scenarios, do we allow users to specify more than one parameters? If not could you make that clear in the wiki, e.g. we would return with an error message saying that only one is allowed; if yes then what precedence order we are following? 2. Personally I feel that "--by-duration", "--to-offset" and "--shift-by" are a tad overkill, because 1) they assume there exist some committed offset for each of the topic, but that may not be always true, 2) offset / time shifting amount on different topics may not be a good fit universally, i.e. one could imagine the we want to reset all input topics to their offsets of a given time, but resetting all topics' offset to the same value or let all of them shifting the same amount of offsets are usually not applicable. For "--by-duration" it seems could be easily supported by the "to-date". For the general consumer group reset tool, since it could be set one per partition these parameters may be more useful. 3. As for the implementation details, when removing zookeeper config in `kafka-streams-application-reset`, we should consider return a meaning error message otherwise it would be "unrecognized config" blah. If you feel confident about the wiki after discussing about these points, please feel free to move on to start a voting thread. Note that we are about 3 weeks away from KIP deadline and 4 weeks away from feature deadline. Guozhang On Tue, Aug 22, 2017 at 1:45 PM, Matthias J. Sax <matth...@confluent.io> wrote: > Thanks for the update Jorge. > > I don't have any further comments. > > > -Matthias > > On 8/12/17 6:43 PM, Jorge Esteban Quilcate Otoya wrote: > > I have updated the KIP: > > > > - Change execution parameters, using `--dry-run` > > - Reference KAFKA-4327 > > - And advise about changes on `StreamResetter` > > > > Also includes that it will cover a change on `ConsumerGroupCommand` to > > align execution options. > > > > El dom., 16 jul. 2017 a las 5:37, Matthias J. Sax (< > matth...@confluent.io>) > > escribió: > > > >> Thanks a lot for the update! > >> > >> I like the KIP! > >> > >> One more question about `--dry-run` vs `--execute`: While I agree that > >> we should use the same flag for both tools, I am not sure which one is > >> the better one... My personal take is, that I like `--dry-run` better. > >> Not sure what others think. > >> > >> One more comment: with the removal of ZK, we can also tackle this JIRA: > >> https://issues.apache.org/jira/browse/KAFKA-4327 If we do so, I think > we > >> should mention it in the KIP. > >> > >> I am also not sure about backward compatibility issue for this case. > >> Actually, I don't expect people to call `StreamsResetter` from Java > >> code, but you can never know. So if we break this, we need to make sure > >> to cover it in the KIP and later on in the release notes. > >> > >> > >> -Matthias > >> > >> On 7/14/17 7:15 AM, Jorge Esteban Quilcate Otoya wrote: > >>> Hi, > >>> > >>> KIP is updated. > >>> Changes: > >>> 1. Approach discussed to keep both tools (streams application resetter > >> and > >>> consumer group reset offset). > >>> 2. Options has been aligned between both tools. > >>> 3. Zookeeper option from streams-application-resetted will be removed, > >> and > >>> replaced internally for Kafka AdminClient. > >>> > >>> Looking forward to your feedback. > >>> > >>> El jue., 29 jun. 2017 a las 15:04, Matthias J. Sax (< > >> matth...@confluent.io>) > >>> escribió: > >>> > >>>> Damian, > >>>> > >>>>> resets everything and clears up > >>>>>> the state stores. > >>>> > >>>> That's not correct. The reset tool does not touch local store. For > this, > >>>> we have `KafkaStreams#clenup` -- otherwise, you would need to run the > >>>> tool in every machine you run an application instance. > >>>> > >>>> With regard to state, the tool only deletes the underlying changelog > >>>> topics. > >>>> > >>>> Just to clarify. The idea of allowing to rest with different offset is > >>>> to clear all state but just use a different start offset (instead of > >>>> zero). This is for use case where your topic has a larger retention > time > >>>> than the amount of data you want to reprocess. But we always need to > >>>> start with an empty state. (Resetting with consistent state is > something > >>>> we might do at some point, but it's much hard and not part of this > KIP) > >>>> > >>>>> @matthias, could we remove the ZK dependency from the streams reset > >> tool > >>>>> now? > >>>> > >>>> I think so. The new AdminClient provide the feature we need AFAIK. I > >>>> guess we can picky back this into the KIP (we would need a KIP anyway > >>>> because we deprecate `--zookeeper` what is an public API change). > >>>> > >>>> > >>>> I just want to point out, that even without ZK dependency, I prefer to > >>>> wrap the consumer offset tool and keep two tools. > >>>> > >>>> > >>>> -Matthias > >>>> > >>>> > >>>> On 6/29/17 9:14 AM, Damian Guy wrote: > >>>>> Hi, > >>>>> > >>>>> Thanks for the KIP. What is not clear is how is this going to handle > >>>> state > >>>>> stores? Right now the streams reset tool, resets everything and > clears > >> up > >>>>> the state stores. What are we going to do if we reset to a particular > >>>>> offset? If we clear the state then we've lost any previously > aggregated > >>>>> values (which may or may not be what is expected). If we don't clear > >>>> them, > >>>>> then we will end up with incorrect aggregates. > >>>>> > >>>>> @matthias, could we remove the ZK dependency from the streams reset > >> tool > >>>>> now? > >>>>> > >>>>> Thanks, > >>>>> Damian > >>>>> > >>>>> On Thu, 29 Jun 2017 at 15:22 Jorge Esteban Quilcate Otoya < > >>>>> quilcate.jo...@gmail.com> wrote: > >>>>> > >>>>>> You're right, I was not considering Zookeeper dependency. > >>>>>> > >>>>>> I'm starting to like the idea to wrap `reset-offset` from > >>>>>> `streams-application-reset`. > >>>>>> > >>>>>> I will wait a bit for more feedback and work on a draft with this > >>>> changes. > >>>>>> > >>>>>> > >>>>>> El mié., 28 jun. 2017 a las 0:20, Matthias J. Sax (< > >>>> matth...@confluent.io > >>>>>>> ) > >>>>>> escribió: > >>>>>> > >>>>>>> I agree, that we should not duplicate functionality. > >>>>>>> > >>>>>>> However, I am worried, that a non-streams users using the offset > >> reset > >>>>>>> tool might delete topics unintentionally (even if the changes are > >>>> pretty > >>>>>>> small). Also, currently the stream reset tool required Zookeeper > >> while > >>>>>>> the offset reset tool does not -- I don't think we should add this > >>>>>>> dependency to the offset reset tool. > >>>>>>> > >>>>>>> Thus, it think it might be better to keep both tools, but > internally > >>>>>>> rewrite the streams reset entry class, to reuse as much as possible > >>>> from > >>>>>>> the offset reset tool. Ie. the streams tool would be a wrapper > around > >>>>>>> the offset tool and add some functionality it needs that is Streams > >>>>>>> specific. > >>>>>>> > >>>>>>> I also think, that keeping separate tools for consumers and streams > >> is > >>>> a > >>>>>>> good thing. We might want to add new features that don't apply to > >> plain > >>>>>>> consumers -- note, a Streams applications is more than just a > client. > >>>>>>> > >>>>>>> WDYT? > >>>>>>> > >>>>>>> Would be good to get some feedback from others, too. > >>>>>>> > >>>>>>> > >>>>>>> -Matthias > >>>>>>> > >>>>>>> > >>>>>>> On 6/27/17 9:05 AM, Jorge Esteban Quilcate Otoya wrote: > >>>>>>>> Thanks for the feedback Matthias! > >>>>>>>> > >>>>>>>> The main idea is to use only 1 tool to reset offsets and don't > >>>>>> replicate > >>>>>>>> functionality between tools. > >>>>>>>> Reset Offset (KIP-122) tool not only reset but support execute the > >>>>>> reset > >>>>>>>> but also export, import from files, etc. > >>>>>>>> If we extend the current tool (kafka-streams-application- > reset.sh) > >> we > >>>>>>> will > >>>>>>>> have to duplicate all this functionality also. > >>>>>>>> Maybe another option is to move the current implementation into > >>>>>>>> `kafka-consumer-group` and add a new command > >> `--reset-offset-streams` > >>>>>>> with > >>>>>>>> the current implementation functionality and add `--reset-offset` > >>>>>> options > >>>>>>>> for input topics. Does this make sense? > >>>>>>>> > >>>>>>>> > >>>>>>>> El lun., 26 jun. 2017 a las 23:32, Matthias J. Sax (< > >>>>>>> matth...@confluent.io>) > >>>>>>>> escribió: > >>>>>>>> > >>>>>>>>> Jorge, > >>>>>>>>> > >>>>>>>>> thanks a lot for this KIP. Allowing the reset streams > applications > >>>>>> with > >>>>>>>>> arbitrary start offset is something we got multiple requests > >> already. > >>>>>>>>> > >>>>>>>>> Couple of clarification question: > >>>>>>>>> > >>>>>>>>> - why do you want to deprecate the current tool instead of > >> extending > >>>>>>>>> the current tool with the stuff the offset reset tool can do (ie, > >> use > >>>>>>>>> the offset reset tool internally) > >>>>>>>>> > >>>>>>>>> - you suggest to extend the offset reset tool to replace the > >> stream > >>>>>>>>> reset tool: how would the reset tool know if it is resetting a > >>>> streams > >>>>>>>>> applications or a regular consumer group? > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> -Matthias > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> On 6/26/17 1:28 PM, Jorge Esteban Quilcate Otoya wrote: > >>>>>>>>>> Hi all, > >>>>>>>>>> > >>>>>>>>>> I'd like to start the discussion to add reset offset tooling for > >>>>>> Stream > >>>>>>>>>> applications. > >>>>>>>>>> The KIP can be found here: > >>>>>>>>>> > >>>>>>>>> > >>>>>>> > >>>>>> > >>>> > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP- > 171+-+Extend+Consumer+Group+Reset+Offset+for+Stream+Application > >>>>>>>>>> > >>>>>>>>>> Thanks, > >>>>>>>>>> Jorge. > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>>> > >>>>>> > >>>>> > >>>> > >>>> > >>> > >> > >> > > > > -- -- Guozhang
