-
Notifications
You must be signed in to change notification settings - Fork 13.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-4733: Improve Streams Reset Tool console output #2503
Conversation
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR Gwen!
.withRequiredArg() | ||
.ofType(String.class) | ||
.withValuesSeparatedBy(',') | ||
.describedAs("list"); | ||
intermediateTopicsOption = optionParser.accepts("intermediate-topics", "Comma-separated list of intermediate user topics") | ||
intermediateTopicsOption = optionParser.accepts("intermediate-topics", "Comma-separated list of intermediate user topics (topics created using through() method). For these topics, the tool will skip to the end.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
topics created using through() method
-> topics used in/by through() method
"*** Important! You will get wrong output if you don't clean up the statestore after running the reset tool!\n\n" | ||
); | ||
parser.printHelpOn(System.err); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: remove empty line ;)
@@ -271,6 +271,18 @@ private boolean isInternalTopic(final String topicName) { | |||
&& (topicName.endsWith("-changelog") || topicName.endsWith("-repartition")); | |||
} | |||
|
|||
private void printHelp(OptionParser parser) throws IOException { | |||
System.err.println("The Application Reset Tool allows you to quickly reset an application in order to reprocess its data from scratch.\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please format this differently. No line should be longer than 120 chars and please start a new line after each sentence (otherwise reviewing is quite cumbersome). Thx
@@ -271,6 +271,18 @@ private boolean isInternalTopic(final String topicName) { | |||
&& (topicName.endsWith("-changelog") || topicName.endsWith("-repartition")); | |||
} | |||
|
|||
private void printHelp(OptionParser parser) throws IOException { | |||
System.err.println("The Application Reset Tool allows you to quickly reset an application in order to reprocess its data from scratch.\n" + | |||
"* This tool will reset offsets of input topics to 0 and it skips to the end of intermediate topics (topics created with the through() method).\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
topics created with the through() method
as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove will
: This tool reset[s] offsets
same below
System.err.println("The Application Reset Tool allows you to quickly reset an application in order to reprocess its data from scratch.\n" + | ||
"* This tool will reset offsets of input topics to 0 and it skips to the end of intermediate topics (topics created with the through() method).\n" + | ||
"* This tool will deletes the internal topics that were created automatically (topics ending in -changelog and -repartition).\n You do not need to specify internal topics the tool finds them automatically.\n" + | ||
"* This tool will not delete output topics (if you want to delete them, you need to do it yourself through bin/kafka-topics.sh command)\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
at the end missing
"* This tool will reset offsets of input topics to 0 and it skips to the end of intermediate topics (topics created with the through() method).\n" + | ||
"* This tool will deletes the internal topics that were created automatically (topics ending in -changelog and -repartition).\n You do not need to specify internal topics the tool finds them automatically.\n" + | ||
"* This tool will not delete output topics (if you want to delete them, you need to do it yourself through bin/kafka-topics.sh command)\n" + | ||
"* This tool will does not clean up the local state on the stream application instances (the persisted stores used to cache aggregation results).\n You need to call KafkaStreams#cleanUp() in your application or manually delete them from the directory specified by state.dir configuration (usually /tmp/kafka-streams/<application.id>\n\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: will does not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually
-> default
(I hope that people usually change the default to avoid unwanted state recreating if temp
gets wiped out :)
And: closing )
missing at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"state.dir"
"* This tool will deletes the internal topics that were created automatically (topics ending in -changelog and -repartition).\n You do not need to specify internal topics the tool finds them automatically.\n" + | ||
"* This tool will not delete output topics (if you want to delete them, you need to do it yourself through bin/kafka-topics.sh command)\n" + | ||
"* This tool will does not clean up the local state on the stream application instances (the persisted stores used to cache aggregation results).\n You need to call KafkaStreams#cleanUp() in your application or manually delete them from the directory specified by state.dir configuration (usually /tmp/kafka-streams/<application.id>\n\n" + | ||
"*** Important! You will get wrong output if you don't clean up the statestore after running the reset tool!\n\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
statestore
-> local stores
(to use same terminology as above)
private void printHelp(OptionParser parser) throws IOException { | ||
System.err.println("The Application Reset Tool allows you to quickly reset an application in order to reprocess its data from scratch.\n" + | ||
"* This tool will reset offsets of input topics to 0 and it skips to the end of intermediate topics (topics created with the through() method).\n" + | ||
"* This tool will deletes the internal topics that were created automatically (topics ending in -changelog and -repartition).\n You do not need to specify internal topics the tool finds them automatically.\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
automatically by Kafka Streams
(maybe remove "automatically"?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe: topics starting with "<application.id>-"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[because] the tool
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
\cc any of @miguno @enothereska @dguy for second review
@@ -138,12 +138,12 @@ private void parseArguments(final String[] args) throws IOException { | |||
.ofType(String.class) | |||
.defaultsTo("localhost:2181") | |||
.describedAs("url"); | |||
inputTopicsOption = optionParser.accepts("input-topics", "Comma-separated list of user input topics") | |||
inputTopicsOption = optionParser.accepts("input-topics", "Comma-separated list of user input topics. For these topics, the tool will reset the offset to 0.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think this is not always true -- it will reset to the earliest available offset, which may not necessarily be 0
, right? (same comment applies to the other mention of offset 0
in the CLI help output below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix this, but note that I copied this explanation out of Confluent's docs, so we may need to fix those too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. @mjsax could you take care of updating the Confluent docs accordingly?
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
@guozhangwang Call for final review and merging. |
I feel like I'll need to start paying the review debt for @guozhangwang soon... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and merged to trunk.
Added general explanation of the tool and what it does. Also added few details to the arguments.