You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/09/13 17:25:37 UTC

[GitHub] [kafka] nizhikov opened a new pull request, #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

nizhikov opened a new pull request, #12632:
URL: https://github.com/apache/kafka/pull/12632

   Implementation of [KIP-865](https://cwiki.apache.org/confluence/display/KAFKA/KIP-865%3A+Support+--bootstrap-server+in+kafka-streams-application-reset)
   
   Support of `--bootstrap-server` option for `kafka-streams-application-reset` implemented
   
   ### Committer Checklist (excluded from commit message)
   - [ ] Verify design and implementation 
   - [ ] Verify test coverage and CI build status
   - [ ] Verify documentation (including upgrade notes)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] nizhikov commented on a diff in pull request #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

Posted by GitBox <gi...@apache.org>.
nizhikov commented on code in PR #12632:
URL: https://github.com/apache/kafka/pull/12632#discussion_r970460345


##########
docs/streams/developer-guide/app-reset-tool.html:
##########
@@ -84,9 +84,11 @@ <h2>Step 1: Run the application reset tool<a class="headerlink" href="#step-1-ru
 ---------------------                 -----------
 * --application-id &lt;String: id&gt;       The Kafka Streams application ID
                                         (application.id).
---bootstrap-servers &lt;String: urls&gt;    Comma-separated list of broker urls with
-                                        format: HOST1:PORT1,HOST2:PORT2
-                                        (default: localhost:9092)
+--bootstrap-server [String: Server(s)  The server(s) to use for bootstrapping.

Review Comment:
   Fixed. Thanks.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] nizhikov commented on a diff in pull request #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

Posted by GitBox <gi...@apache.org>.
nizhikov commented on code in PR #12632:
URL: https://github.com/apache/kafka/pull/12632#discussion_r973021340


##########
core/src/main/scala/kafka/tools/StreamsResetter.java:
##########
@@ -213,11 +217,16 @@ private void parseArguments(final String[] args) {
             .ofType(String.class)
             .describedAs("id")
             .required();
-        bootstrapServerOption = optionParser.accepts("bootstrap-servers", "Comma-separated list of broker urls with format: HOST1:PORT1,HOST2:PORT2")
-            .withRequiredArg()
+        bootstrapServersOption = optionParser.accepts("bootstrap-servers", "DEPRECATED: Comma-separated list of broker urls with format: HOST1:PORT1,HOST2:PORT2")
+            .withOptionalArg()

Review Comment:
   Logic changed to:
   
   ```
   
               String bootstrapServerValue = BOOTSTRAP_SERVER_DEFAULT;
   
               if (options.has(bootstrapServerOption))
                   bootstrapServerValue = options.valueOf(bootstrapServerOption);
               else if (options.has(bootstrapServersOption))
                   bootstrapServerValue = options.valueOf(bootstrapServersOption);
   
               properties.put(ConsumerConfig.BOOTSTRAP_SERVERS_CONFIG, bootstrapServerValue);
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] C0urante merged pull request #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

Posted by GitBox <gi...@apache.org>.
C0urante merged PR #12632:
URL: https://github.com/apache/kafka/pull/12632


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] nizhikov commented on pull request #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

Posted by GitBox <gi...@apache.org>.
nizhikov commented on PR #12632:
URL: https://github.com/apache/kafka/pull/12632#issuecomment-1249359020

   Looks like system tests OK.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] nizhikov commented on pull request #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

Posted by GitBox <gi...@apache.org>.
nizhikov commented on PR #12632:
URL: https://github.com/apache/kafka/pull/12632#issuecomment-1249358441

   ```
   docker exec ducker01 bash -c "cd /opt/kafka-dev && ducktape --cluster-file /opt/kafka-dev/tests/docker/build/cluster.json  ./tests/kafkatest/tests/streams/streams_optimized_test.py "
   /usr/local/lib/python3.9/dist-packages/paramiko/transport.py:236: CryptographyDeprecationWarning: Blowfish has been deprecated
     "class": algorithms.Blowfish,
   [INFO:2022-09-16 06:19:04,228]: starting test run with session id 2022-09-16--001...
   [INFO:2022-09-16 06:19:04,228]: running 1 tests...
   [INFO:2022-09-16 06:19:04,228]: Triggering test 1 of 1...
   [INFO:2022-09-16 06:19:04,236]: RunnerClient: Loading test {'directory': '/opt/kafka-dev/tests/kafkatest/tests/streams', 'file_name': 'streams_optimized_test.py', 'cls_name': 'StreamsOptimizedTest', 'method_name': 'test_upgrade_optimized_topology', 'injected_args': {'metadata_quorum': 'REMOTE_KRAFT'}}
   [INFO:2022-09-16 06:19:04,239]: RunnerClient: kafkatest.tests.streams.streams_optimized_test.StreamsOptimizedTest.test_upgrade_optimized_topology.metadata_quorum=REMOTE_KRAFT: on run 1/1
   [INFO:2022-09-16 06:19:04,241]: RunnerClient: kafkatest.tests.streams.streams_optimized_test.StreamsOptimizedTest.test_upgrade_optimized_topology.metadata_quorum=REMOTE_KRAFT: Setting up...
   [INFO:2022-09-16 06:19:04,241]: RunnerClient: kafkatest.tests.streams.streams_optimized_test.StreamsOptimizedTest.test_upgrade_optimized_topology.metadata_quorum=REMOTE_KRAFT: Running...
   [INFO:2022-09-16 06:20:18,437]: RunnerClient: kafkatest.tests.streams.streams_optimized_test.StreamsOptimizedTest.test_upgrade_optimized_topology.metadata_quorum=REMOTE_KRAFT: Tearing down...
   [INFO:2022-09-16 06:20:24,849]: RunnerClient: kafkatest.tests.streams.streams_optimized_test.StreamsOptimizedTest.test_upgrade_optimized_topology.metadata_quorum=REMOTE_KRAFT: PASS
   [INFO:2022-09-16 06:20:24,849]: RunnerClient: kafkatest.tests.streams.streams_optimized_test.StreamsOptimizedTest.test_upgrade_optimized_topology.metadata_quorum=REMOTE_KRAFT: Data: None
   ================================================================================
   SESSION REPORT (ALL TESTS)
   ducktape version: 0.11.1
   session_id:       2022-09-16--001
   run time:         1 minute 20.629 seconds
   tests run:        1
   passed:           1
   flaky:            0
   failed:           0
   ignored:          0
   ================================================================================
   test_id:    kafkatest.tests.streams.streams_optimized_test.StreamsOptimizedTest.test_upgrade_optimized_topology.metadata_quorum=REMOTE_KRAFT
   status:     PASS
   run time:   1 minute 20.610 seconds
   --------------------------------------------------------------------------------
   nizhikov@kafka-test:~/kafka$ git status
   On branch KAFKA-12878
   nothing to commit, working tree clean
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] C0urante commented on a diff in pull request #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12632:
URL: https://github.com/apache/kafka/pull/12632#discussion_r974169402


##########
core/src/main/scala/kafka/tools/StreamsResetter.java:
##########
@@ -213,11 +217,16 @@ private void parseArguments(final String[] args) {
             .ofType(String.class)
             .describedAs("id")
             .required();
-        bootstrapServerOption = optionParser.accepts("bootstrap-servers", "Comma-separated list of broker urls with format: HOST1:PORT1,HOST2:PORT2")
-            .withRequiredArg()
+        bootstrapServersOption = optionParser.accepts("bootstrap-servers", "DEPRECATED: Comma-separated list of broker urls with format: HOST1:PORT1,HOST2:PORT2")
+            .withOptionalArg()

Review Comment:
   I think we still need to revert the change from `withRequiredArg` to `withOptionalArg`. If something has a required argument, the flag (e.g., `--bootstrap-server`) has to be followed by an argument; if it has an optional argument, you can provide just the flag without any argument.
   
   It's fine if we make providing the flags themselves optional (which I believe your if/else logic does), but we shouldn't make providing arguments for those flags optional; if someone provides `--bootstrap-server` or `--bootstrap-servers`, it should always be followed by the actual bootstrap server list.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] nizhikov commented on a diff in pull request #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

Posted by GitBox <gi...@apache.org>.
nizhikov commented on code in PR #12632:
URL: https://github.com/apache/kafka/pull/12632#discussion_r974176089


##########
core/src/main/scala/kafka/tools/StreamsResetter.java:
##########
@@ -213,11 +217,16 @@ private void parseArguments(final String[] args) {
             .ofType(String.class)
             .describedAs("id")
             .required();
-        bootstrapServerOption = optionParser.accepts("bootstrap-servers", "Comma-separated list of broker urls with format: HOST1:PORT1,HOST2:PORT2")
-            .withRequiredArg()
+        bootstrapServersOption = optionParser.accepts("bootstrap-servers", "DEPRECATED: Comma-separated list of broker urls with format: HOST1:PORT1,HOST2:PORT2")
+            .withOptionalArg()

Review Comment:
   Thank. I changed code following your suggestion.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] C0urante commented on a diff in pull request #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12632:
URL: https://github.com/apache/kafka/pull/12632#discussion_r972121384


##########
core/src/main/scala/kafka/tools/StreamsResetter.java:
##########
@@ -213,11 +217,16 @@ private void parseArguments(final String[] args) {
             .ofType(String.class)
             .describedAs("id")
             .required();
-        bootstrapServerOption = optionParser.accepts("bootstrap-servers", "Comma-separated list of broker urls with format: HOST1:PORT1,HOST2:PORT2")
-            .withRequiredArg()
+        bootstrapServersOption = optionParser.accepts("bootstrap-servers", "DEPRECATED: Comma-separated list of broker urls with format: HOST1:PORT1,HOST2:PORT2")
+            .withOptionalArg()

Review Comment:
   I don't think we can apply this change, even though having a required argument effectively cancels out the default of `localhost:9092`, since it's not covered in the KIP.



##########
docs/streams/developer-guide/app-reset-tool.html:
##########
@@ -84,9 +84,14 @@ <h2>Step 1: Run the application reset tool<a class="headerlink" href="#step-1-ru
 ---------------------                 -----------
 * --application-id &lt;String: id&gt;       The Kafka Streams application ID
                                         (application.id).
---bootstrap-servers &lt;String: urls&gt;    Comma-separated list of broker urls with
-                                        format: HOST1:PORT1,HOST2:PORT2
-                                        (default: localhost:9092)
+--bootstrap-server &lt;String: server to  REQUIRED unless --bootstrap-servers
+                            connect to&gt;                            (deprecated) is specified. The server
+                                         (s) to connect to. The broker list
+                                         string in the form HOST1:PORT1,HOST2:
+                                         PORT2.
+--bootstrap-servers [String: urls]     DEPRECATED: Comma-separated list of

Review Comment:
   One more:
   ```suggestion
   --bootstrap-servers &lt;String: urls&gt;     DEPRECATED: Comma-separated list of
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] C0urante commented on pull request #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

Posted by GitBox <gi...@apache.org>.
C0urante commented on PR #12632:
URL: https://github.com/apache/kafka/pull/12632#issuecomment-1245985132

   Should we also update the system test [here](https://github.com/apache/kafka/blob/7b49f175b964c468d5ca7ac9bd70dfd72e6a2e4e/tests/kafkatest/services/streams.py#L547)?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] nizhikov commented on pull request #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

Posted by GitBox <gi...@apache.org>.
nizhikov commented on PR #12632:
URL: https://github.com/apache/kafka/pull/12632#issuecomment-1251293104

   @C0urante tests finished. It seems for me that failures unrelated but can you double check?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] nizhikov commented on pull request #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

Posted by GitBox <gi...@apache.org>.
nizhikov commented on PR #12632:
URL: https://github.com/apache/kafka/pull/12632#issuecomment-1246427376

   @C0urante Thanks for the review. Fixed all your comments.
   
   > Should we also update the system test
   
   I doubt it.
   
   a. We have compatibility tests (which not use streams resetter at the moment). see - [streams_broker_compatibility_test.py](https://github.com/apache/kafka/blob/trunk/tests/kafkatest/tests/streams/streams_broker_compatibility_test.py#L82).
   So it's possible then in future we will try to use `kafka-streams-application-reset.sh` for previously released Kafka versions which knows nothing about new parameter.
   
   b. At the moment I don't have test environment to check changes in system tests :).


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] nizhikov commented on a diff in pull request #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

Posted by GitBox <gi...@apache.org>.
nizhikov commented on code in PR #12632:
URL: https://github.com/apache/kafka/pull/12632#discussion_r970457373


##########
core/src/main/scala/kafka/tools/StreamsResetter.java:
##########
@@ -213,8 +217,12 @@ private void parseArguments(final String[] args) {
             .ofType(String.class)
             .describedAs("id")
             .required();
-        bootstrapServerOption = optionParser.accepts("bootstrap-servers", "Comma-separated list of broker urls with format: HOST1:PORT1,HOST2:PORT2")
-            .withRequiredArg()
+        bootstrapServerOption = optionParser.accepts("bootstrap-server", "The server(s) to use for bootstrapping.")

Review Comment:
   Thanks for a hint. Apply code you suggested.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] nizhikov commented on pull request #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

Posted by GitBox <gi...@apache.org>.
nizhikov commented on PR #12632:
URL: https://github.com/apache/kafka/pull/12632#issuecomment-1251318220

   @C0urante Thank you very much)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] C0urante commented on a diff in pull request #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

Posted by GitBox <gi...@apache.org>.
C0urante commented on code in PR #12632:
URL: https://github.com/apache/kafka/pull/12632#discussion_r970105520


##########
core/src/main/scala/kafka/tools/StreamsResetter.java:
##########
@@ -213,8 +217,12 @@ private void parseArguments(final String[] args) {
             .ofType(String.class)
             .describedAs("id")
             .required();
-        bootstrapServerOption = optionParser.accepts("bootstrap-servers", "Comma-separated list of broker urls with format: HOST1:PORT1,HOST2:PORT2")
-            .withRequiredArg()
+        bootstrapServerOption = optionParser.accepts("bootstrap-server", "The server(s) to use for bootstrapping.")

Review Comment:
   Should we use a description that's similar to the one in the console producer, which also had a KIP to deprecate a different property in favor of `--bootstrap-server`?
   
   Thinking we can copy and tweak from here: https://github.com/apache/kafka/blob/7b49f175b964c468d5ca7ac9bd70dfd72e6a2e4e/core/src/main/scala/kafka/tools/ConsoleProducer.scala#L135



##########
docs/streams/developer-guide/app-reset-tool.html:
##########
@@ -84,9 +84,11 @@ <h2>Step 1: Run the application reset tool<a class="headerlink" href="#step-1-ru
 ---------------------                 -----------
 * --application-id &lt;String: id&gt;       The Kafka Streams application ID
                                         (application.id).
---bootstrap-servers &lt;String: urls&gt;    Comma-separated list of broker urls with
-                                        format: HOST1:PORT1,HOST2:PORT2
-                                        (default: localhost:9092)
+--bootstrap-server [String: Server(s)  The server(s) to use for bootstrapping.

Review Comment:
   Can we keep the formatting consistent with the other options here, using `<...>` instead of `[...]`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] nizhikov commented on pull request #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

Posted by GitBox <gi...@apache.org>.
nizhikov commented on PR #12632:
URL: https://github.com/apache/kafka/pull/12632#issuecomment-1249229645

   @C0urante Thanks for the review. Fixed all your comments. Please, take a look one more time.
   
   > So we shouldn't have to worry about running these tests with a version of the CLI tools that doesn't recognize --bootstrap-server.
   
   Option name changed. I will try to setup ducktape tests environment and run the test.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] nizhikov commented on pull request #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

Posted by GitBox <gi...@apache.org>.
nizhikov commented on PR #12632:
URL: https://github.com/apache/kafka/pull/12632#issuecomment-1247109394

   I've checked tests failure.
   Looks like they are not related to the changes.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [kafka] C0urante commented on pull request #12632: KAFKA-12878 Support --bootstrap-server in kafka-streams-application-reset tool

Posted by GitBox <gi...@apache.org>.
C0urante commented on PR #12632:
URL: https://github.com/apache/kafka/pull/12632#issuecomment-1251316903

   Yep, looks good. Merged.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org