Thanks for the update! On Fri, Jul 3, 2020 at 3:18 PM Joel Wee <joel....@outlook.com> wrote:
> Thanks all for voting! > > I am closing the vote as accepted with three binding +1 votes (Boyang, > Guozhang, John). > > Thanks John for the suggestion. I think that makes sense. I have updated > the KIP to say that only topics flagged as internal by the tool can be > deleted, and have also rephrased the option description to make this > clearer: > > > --internal-topics <String: list> Comma-separated list of internal > topics > > to delete. Must be a subset of > the > > internal topics marked for > deletion by > > the default behaviour. To view > these > > topics, do a dry-run without > this > > option. > > Hmm, could you explain why we couldn't run with both --internal-topics and --dry-run? To me we could either display the exact set of internal topics specified to be deleted, or reject the same way as we are doing an actual reset. > > Thanks Guozhang for the suggestion. The updated option description should > address your first point. By the “topology description” script are you > referring to bin/kafka-topics.sh? It currently has the option to display > all topics (including internal topics). Were you thinking about adding > something to view just internal topics? > > Thanks Bruno for the suggestion. I will close this vote for now, and we > can continue the discussion on another thread. (P.S. apologies for > misspelling your name previously) > > - Joel > > > On 1 Jul 2020, at 3:04 AM, Boyang Chen <reluctanthero...@gmail.com> > wrote: > > > > Hey Bruno, > > > > I agree adding a prompt would be a nice precaution, but it is not > backward > > compatible as you suggested and could make the automation harder to > > achieve. > > > > If you want, we may consider starting a separate ticket to discuss > whether > > adding a prompt to let users be aware of the topics that are about to > > delete. However, this is also inverting the assumptions we made about > > `--dry-run` mode, which would become useless to me once we are adding a > > prompt asking users whether they want to remove these topics completely, > or > > do nothing at all. > > > > Back to KIP-623, I think this discussion could be held in orthogonal, > which > > applies to more general considerations of reducing human errors, etc. > > > > Boyang > > > > On Tue, Jun 30, 2020 at 12:55 AM Bruno Cadonna <br...@confluent.io> > wrote: > > > >> Hi, > >> > >> I have already brought this up in the discussion thread. > >> > >> Should we not run a dry-run in any case to avoid inadvertently > >> deleting topics of other applications? > >> > >> I know it is a backward incompatible change if users use it in > >> scripts, but I think it is still worth discussing it. I would to hear > >> what committers think about it. > >> > >> Best, > >> Bruno > >> > >> On Tue, Jun 30, 2020 at 5:33 AM Boyang Chen <reluctanthero...@gmail.com > > > >> wrote: > >>> > >>> Thanks John for the great suggestion. +1 for enforcing the prefix check > >> for > >>> the `--internal-topics` list. > >>> > >>> On Mon, Jun 29, 2020 at 5:11 PM John Roesler <vvcep...@apache.org> > >> wrote: > >>> > >>>> Oh, I meant to say, if that’s the case, then I’m +1 (binding) also :) > >>>> > >>>> Thanks again, > >>>> John > >>>> > >>>> On Mon, Jun 29, 2020, at 19:09, John Roesler wrote: > >>>>> Thanks for the KIP, Joel! > >>>>> > >>>>> It seems like a good pattern to document would be to first run with > >>>>> —dry-run and without —internal-topics to list all potential topics, > >> and > >>>>> then to use —internal-topics if needed to limit the internal topics > >> to > >>>>> delete. > >>>>> > >>>>> Just to make sure, would we have a sanity check to forbid including > >>>>> arbitrary topics? I.e., it seems like —internal-topics should require > >>>>> all the topics to be prefixed with the app id. Is that right? > >>>>> > >>>>> Thanks, > >>>>> John > >>>>> > >>>>> On Mon, Jun 29, 2020, at 18:25, Guozhang Wang wrote: > >>>>>> Thanks Joel, the KIP lgtm. > >>>>>> > >>>>>> A minor suggestion is to explain where users can get the list of > >>>> internal > >>>>>> topics of a given application, and maybe also add it as part of the > >>>> helper > >>>>>> scripts, for example via topology description. > >>>>>> > >>>>>> Overall, I'm +1 as well (binding). > >>>>>> > >>>>>> Guozhang > >>>>>> > >>>>>> > >>>>>> On Sat, Jun 27, 2020 at 4:33 AM Joel Wee <joel....@outlook.com> > >> wrote: > >>>>>> > >>>>>>> Thanks Boyang, I think what you’ve said makes sense. I’ve made > >> the > >>>>>>> motivation clearer now: > >>>>>>> > >>>>>>> > >>>>>>> Users may want to specify which internal topics should be > >> deleted. At > >>>>>>> present, the streams reset tool deletes all topics that start > >> with "< > >>>>>>> application.id<http://application.id>>-" and there are no > >> options to > >>>>>>> control it. > >>>>>>> > >>>>>>> The `--internal-topics` option is especially useful when there > >> are > >>>> prefix > >>>>>>> conflicts between applications, e.g. "app" and "app-v2". In this > >>>> case, if > >>>>>>> we want to reset "app", the reset tool’s default behaviour will > >>>> delete both > >>>>>>> the internal topics of "app" and "app-v2" (since both are > >> prefixed by > >>>>>>> "app-"). With the `--internal-topics` option, we can provide > >>>> internal topic > >>>>>>> names for "app" and delete the internal topics for "app" without > >>>> deleting > >>>>>>> the internal topics for "app-v2". > >>>>>>> > >>>>>>> Best > >>>>>>> > >>>>>>> Joel > >>>>>>> > >>>>>>> On 27 Jun 2020, at 2:07 AM, Boyang Chen < > >> reluctanthero...@gmail.com > >>>>>>> <mailto:reluctanthero...@gmail.com>> wrote: > >>>>>>> > >>>>>>> Thanks for driving the proposal Joel, I have a minor > >> suggestion: we > >>>> should > >>>>>>> be more clear about why we introduce this flag, so it would be > >>>> better to > >>>>>>> also state clearly in the document for the default behavior as > >> well, > >>>> such > >>>>>>> like: > >>>>>>> > >>>>>>> Comma-separated list of internal topics to be deleted. By > >> default, > >>>>>>> Streams reset tool will delete all topics prefixed by the > >>>>>>> application.id<http://application.id>. > >>>>>>> > >>>>>>> This flag is useful when you need to keep certain topics intact > >> due > >>>> to > >>>>>>> the prefix conflict with another application (such like "app" vs > >>>>>>> "app-v2"). > >>>>>>> > >>>>>>> With provided internal topic names for "app", the reset tool will > >>>> only > >>>>>>> delete internal topics associated with "app", instead of both > >> "app" > >>>>>>> and "app-v2". > >>>>>>> > >>>>>>> > >>>>>>> Other than that, +1 from me (binding). > >>>>>>> > >>>>>>> On Wed, Jun 24, 2020 at 1:19 PM Joel Wee <joel....@outlook.com > >>>> <mailto: > >>>>>>> joel....@outlook.com>> wrote: > >>>>>>> > >>>>>>> Apologies. Changing the subject. > >>>>>>> > >>>>>>> On 24 Jun 2020, at 9:14 PM, Joel Wee <joel....@outlook.com > >> <mailto: > >>>>>>> joel....@outlook.com><mailto: > >>>>>>> joel....@outlook.com<mailto:joel....@outlook.com>>> wrote: > >>>>>>> > >>>>>>> Hi all > >>>>>>> > >>>>>>> I would like to start a vote for KIP-623, which adds the option > >>>>>>> --internal-topics to the streams-application-reset-tool: > >>>>>>> > >>>> > >> > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=158862177 > >>>>>>> . > >>>>>>> > >>>>>>> Please let me know what you think. > >>>>>>> > >>>>>>> Best > >>>>>>> > >>>>>>> Joel > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>> > >>>>>> -- > >>>>>> -- Guozhang > >>>>>> > >>>>> > >>>> > >> > >
