Sounds good, thank you! Moreover, I do feel it would be great to integrate some of the motivation stuff into the option description, but we could discuss more in the PR.
On Sat, Jul 18, 2020 at 3:24 AM Joel Wee <joel....@outlook.com> wrote: > I agree Boyang, we should be able to specify both `—internal-topics` and > `—dry-run` at the same time and this should display the topics that will be > deleted. What I was trying to communicate in the description is that if you > want to see all the topics that are valid as input into the option, then > first do a dry-run without the `—internal-topics` option. I’ve rephrased it > slightly which hopefully makes it 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 (do a dry-run > without this > option to view these topics). > > > - Joel > > On 11 Jul 2020, at 8:41 AM, Boyang Chen <reluctanthero...@gmail.com > <mailto:reluctanthero...@gmail.com>> wrote: > > Thanks for the update! > > On Fri, Jul 3, 2020 at 3:18 PM Joel Wee <joel....@outlook.com<mailto: > 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<mailto: > 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<mailto: > 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 > <mailto: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<mailto: > 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<mailto: > 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><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> > <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 > > > > > > > >
