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 2021/07/09 09:55:55 UTC

[GitHub] [kafka] OmniaGM opened a new pull request #11007: KAFKA-10589 Rename kafka-replica-verification CLI command line arguments for KIP-629

OmniaGM opened a new pull request #11007:
URL: https://github.com/apache/kafka/pull/11007


   This pr make `--topic-white-list` as deprecated and introduce `--topic-include-list` for `kafka-replica-verification`
   Note 
   There's no test or any other usage for `--topic-white-list` in other places in the repo.
   
   ### 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] OmniaGM commented on pull request #11007: KAFKA-10589 Rename kafka-replica-verification CLI command line arguments for KIP-629

Posted by GitBox <gi...@apache.org>.
OmniaGM commented on pull request #11007:
URL: https://github.com/apache/kafka/pull/11007#issuecomment-879026189


   @gwenshap (or any other committer) can you please have a look into this? 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] xvrl commented on a change in pull request #11007: KAFKA-10589 Rename kafka-replica-verification CLI command line arguments for KIP-629

Posted by GitBox <gi...@apache.org>.
xvrl commented on a change in pull request #11007:
URL: https://github.com/apache/kafka/pull/11007#discussion_r667359994



##########
File path: core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
##########
@@ -143,11 +152,11 @@ object ReplicaVerificationTool extends Logging {
     }
 
     val filteredTopicMetadata = topicsMetadata.filter { topicMetaData =>
-      topicWhiteListFiler.isTopicAllowed(topicMetaData.name, excludeInternalTopics = false)
+      topicIncludeFiler.isTopicAllowed(topicMetaData.name, excludeInternalTopics = false)

Review comment:
       looks like there was a typo here before `s/Filter/Filter/`
   ```suggestion
         topicsIncludeFilter.isTopicAllowed(topicMetaData.name, excludeInternalTopics = false)
   ```




-- 
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] xvrl commented on a change in pull request #11007: KAFKA-10589 Rename kafka-replica-verification CLI command line arguments for KIP-629

Posted by GitBox <gi...@apache.org>.
xvrl commented on a change in pull request #11007:
URL: https://github.com/apache/kafka/pull/11007#discussion_r667359994



##########
File path: core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
##########
@@ -143,11 +152,11 @@ object ReplicaVerificationTool extends Logging {
     }
 
     val filteredTopicMetadata = topicsMetadata.filter { topicMetaData =>
-      topicWhiteListFiler.isTopicAllowed(topicMetaData.name, excludeInternalTopics = false)
+      topicIncludeFiler.isTopicAllowed(topicMetaData.name, excludeInternalTopics = false)

Review comment:
       looks like there was a typo here before
   ```suggestion
         topicIncludeFilter.isTopicAllowed(topicMetaData.name, excludeInternalTopics = false)
   ```




-- 
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] OmniaGM commented on pull request #11007: KAFKA-10589 Rename kafka-replica-verification CLI command line arguments for KIP-629

Posted by GitBox <gi...@apache.org>.
OmniaGM commented on pull request #11007:
URL: https://github.com/apache/kafka/pull/11007#issuecomment-877638062


   Hi, @xvrl can you please have a look into this? 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] OmniaGM commented on pull request #11007: KAFKA-10589 Rename kafka-replica-verification CLI command line arguments for KIP-629

Posted by GitBox <gi...@apache.org>.
OmniaGM commented on pull request #11007:
URL: https://github.com/apache/kafka/pull/11007#issuecomment-878101172


   @kkonstantine can you approve this, please?


-- 
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] dongjinleekr commented on a change in pull request #11007: KAFKA-10589 Rename kafka-replica-verification CLI command line arguments for KIP-629

Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #11007:
URL: https://github.com/apache/kafka/pull/11007#discussion_r668731566



##########
File path: core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
##########
@@ -89,11 +89,16 @@ object ReplicaVerificationTool extends Logging {
                          .describedAs("ms")
                          .ofType(classOf[java.lang.Integer])
                          .defaultsTo(1000)
-    val topicWhiteListOpt = parser.accepts("topic-white-list", "White list of topics to verify replica consistency. Defaults to all topics.")
+    val topicWhiteListOpt = parser.accepts("topic-white-list", "DEPRECATED use --topics-include instead; ignored if --topics-include specified. List of topics to verify replica consistency. Defaults to all topics.")
                          .withRequiredArg
                          .describedAs("Java regex (String)")
                          .ofType(classOf[String])
                          .defaultsTo(".*")
+    val topicsIncludeOpt = parser.accepts("topics-include", "List of topics to verify replica consistency. Defaults to all topics.")

Review comment:
       `topics-include` - I love this consistency with other cli tools! :+1:
   
   However, how about `Defaults to all topics.` → `Defaults to '.*' (all topics)`? I think this way feels much more straightforward since it is a regex.

##########
File path: core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
##########
@@ -118,8 +123,12 @@ object ReplicaVerificationTool extends Logging {
     }
     CommandLineUtils.checkRequiredArgs(parser, options, brokerListOpt)
 
-    val regex = options.valueOf(topicWhiteListOpt)
-    val topicWhiteListFiler = new IncludeList(regex)
+    val regex = if (options.has(topicsIncludeOpt))
+      options.valueOf(topicsIncludeOpt)
+    else
+      options.valueOf(topicWhiteListOpt)
+
+    val topicsIncludeFilter = new IncludeList(regex)

Review comment:
       Great, fixing the typo: `topicWhiteListFiler` → `topicsIncludeFilter` :+1: 




-- 
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] xvrl commented on a change in pull request #11007: KAFKA-10589 Rename kafka-replica-verification CLI command line arguments for KIP-629

Posted by GitBox <gi...@apache.org>.
xvrl commented on a change in pull request #11007:
URL: https://github.com/apache/kafka/pull/11007#discussion_r667359922



##########
File path: core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
##########
@@ -89,11 +89,16 @@ object ReplicaVerificationTool extends Logging {
                          .describedAs("ms")
                          .ofType(classOf[java.lang.Integer])
                          .defaultsTo(1000)
-    val topicWhiteListOpt = parser.accepts("topic-white-list", "White list of topics to verify replica consistency. Defaults to all topics.")
+    val topicWhiteListOpt = parser.accepts("topic-white-list", "DEPRECATED use --topics-include instead; List of topics to verify replica consistency. Defaults to all topics.")
                          .withRequiredArg
                          .describedAs("Java regex (String)")
                          .ofType(classOf[String])
                          .defaultsTo(".*")
+    val topicIncludeOpt = parser.accepts("topics-include", "List of topics to verify replica consistency. Defaults to all topics.")

Review comment:
       nit, keep the variable name consistent with the argument
   ```suggestion
       val topicsIncludeOpt = parser.accepts("topics-include", "List of topics to verify replica consistency. Defaults to all topics.")
   ```




-- 
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] dajac merged pull request #11007: KAFKA-10589 Rename kafka-replica-verification CLI command line arguments for KIP-629

Posted by GitBox <gi...@apache.org>.
dajac merged pull request #11007:
URL: https://github.com/apache/kafka/pull/11007


   


-- 
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] dajac commented on pull request #11007: KAFKA-10589 Rename kafka-replica-verification CLI command line arguments for KIP-629

Posted by GitBox <gi...@apache.org>.
dajac commented on pull request #11007:
URL: https://github.com/apache/kafka/pull/11007#issuecomment-879641980


   Merged to trunk and 3.0. cc @kkonstantine 


-- 
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] xvrl commented on a change in pull request #11007: KAFKA-10589 Rename kafka-replica-verification CLI command line arguments for KIP-629

Posted by GitBox <gi...@apache.org>.
xvrl commented on a change in pull request #11007:
URL: https://github.com/apache/kafka/pull/11007#discussion_r667359922



##########
File path: core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
##########
@@ -89,11 +89,16 @@ object ReplicaVerificationTool extends Logging {
                          .describedAs("ms")
                          .ofType(classOf[java.lang.Integer])
                          .defaultsTo(1000)
-    val topicWhiteListOpt = parser.accepts("topic-white-list", "White list of topics to verify replica consistency. Defaults to all topics.")
+    val topicWhiteListOpt = parser.accepts("topic-white-list", "DEPRECATED use --topics-include instead; List of topics to verify replica consistency. Defaults to all topics.")
                          .withRequiredArg
                          .describedAs("Java regex (String)")
                          .ofType(classOf[String])
                          .defaultsTo(".*")
+    val topicIncludeOpt = parser.accepts("topics-include", "List of topics to verify replica consistency. Defaults to all topics.")

Review comment:
       nit, keep the variable name consistent with the argument and
   
   ```suggestion
       val topicsIncludeOpt = parser.accepts("topics-include", "List of topics to verify replica consistency. Defaults to all topics.")
   ```




-- 
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] xvrl commented on a change in pull request #11007: KAFKA-10589 Rename kafka-replica-verification CLI command line arguments for KIP-629

Posted by GitBox <gi...@apache.org>.
xvrl commented on a change in pull request #11007:
URL: https://github.com/apache/kafka/pull/11007#discussion_r667359994



##########
File path: core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
##########
@@ -143,11 +152,11 @@ object ReplicaVerificationTool extends Logging {
     }
 
     val filteredTopicMetadata = topicsMetadata.filter { topicMetaData =>
-      topicWhiteListFiler.isTopicAllowed(topicMetaData.name, excludeInternalTopics = false)
+      topicIncludeFiler.isTopicAllowed(topicMetaData.name, excludeInternalTopics = false)

Review comment:
       looks like there was a typo here before `s/Filter/Filter/` and also make topics plural for consistency
   ```suggestion
         topicsIncludeFilter.isTopicAllowed(topicMetaData.name, excludeInternalTopics = false)
   ```




-- 
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