MINOR: Update inconsistent description for kafka-streams-application-reset and kafka-producer-perf-test#20445
Conversation
kafka-streams-application-reset.shkafka-producer-perf-test.sh(See --bootstrap-server) After: |
DL1231
left a comment
There was a problem hiding this comment.
Thanks for the patch, left a small comment.
| .type(String.class) | ||
| .metavar("BOOTSTRAP-SERVERS") | ||
| .metavar("BOOTSTRAP-SERVER") | ||
| .dest("bootstrapServers") |
There was a problem hiding this comment.
nit: bootstrapServer
ConfigPostProcessor.bootstrapServers -> bootstrapServer
There was a problem hiding this comment.
Done. Thanks for the review.
frankvicky
left a comment
There was a problem hiding this comment.
Please fix the build error.
|
Done. Thanks. |
m1a2st
left a comment
There was a problem hiding this comment.
Thanks for this patch, left a comment
| if (CommandLineUtils.isPrintVersionNeeded(this)) { | ||
| CommandLineUtils.printVersionAndExit(); | ||
| } | ||
| CommandLineUtils.checkRequiredArgs(parser, options, bootstrapServerOption); |
There was a problem hiding this comment.
Could you also add a test to verify that the bootstrap-server argument is required and that the command fails with a proper error message if it is missing?
|
I’ve added the test and made slight updates to the code and docs. Also updated the "After" screenshot above. Thanks for the review. |
| .ofType(String.class) | ||
| .describedAs("server to connect to"); | ||
| .describedAs("server to connect to") | ||
| .required(); |
There was a problem hiding this comment.
Is this breaking change included by the KIP?
There was a problem hiding this comment.
I'll revert the change that requires bootstrapServer, since the previous KIP-865 is ambiguous and highly dependent on the implementation. It's better not to introduce the breaking change here.
That is, this PR only updates the docs and text. Thanks.
There was a problem hiding this comment.
Updated. And also update the screenshot above.
There was a problem hiding this comment.
We should keep the word REQUIRED since the bootstrapServerOption is now completely required. That should be addressed in #16437 😄
There was a problem hiding this comment.
Sure. I've reverted the latest commit, thanks!
There was a problem hiding this comment.
Sorry, my last comment was incorrect. It has a default value, so the config is definitely optional
String bootstrapServerValue = "localhost:9092";
There was a problem hiding this comment.
No worries, I was lost too, haha.
Updated.
|
@frankvicky @DL1231 @m1a2st Sorry about that - I forgot to add you to reviewers. my bad :( |
…reset and kafka-producer-perf-test (apache#20445) The value placeholder for the --bootstrap-server argument in the usage output (e.g., when running with --help) has been corrected from the plural form (BOOTSTRAP-SERVERS) to the consistent singular form (BOOTSTRAP-SERVER). Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
…reset and kafka-producer-perf-test (apache#20445) The value placeholder for the --bootstrap-server argument in the usage output (e.g., when running with --help) has been corrected from the plural form (BOOTSTRAP-SERVERS) to the consistent singular form (BOOTSTRAP-SERVER). Reviewers: Chia-Ping Tsai <chia7712@gmail.com>
…reset and kafka-producer-perf-test (apache#20445) The value placeholder for the --bootstrap-server argument in the usage output (e.g., when running with --help) has been corrected from the plural form (BOOTSTRAP-SERVERS) to the consistent singular form (BOOTSTRAP-SERVER). Reviewers: Chia-Ping Tsai <chia7712@gmail.com>


The value placeholder for the --bootstrap-server argument in the usage
output (e.g., when running with --help) has been corrected from the
plural form (BOOTSTRAP-SERVERS) to the consistent singular form
(BOOTSTRAP-SERVER).
Reviewers: Chia-Ping Tsai chia7712@gmail.com