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/06/21 08:01:34 UTC
[GitHub] [kafka] tombentley commented on a change in pull request #10827: [WIP] KAFKA-12899: Support --bootstrap-server in ReplicaVerificationTool
tombentley commented on a change in pull request #10827:
URL: https://github.com/apache/kafka/pull/10827#discussion_r655154960
##########
File path: core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
##########
@@ -73,53 +74,61 @@ object ReplicaVerificationTool extends Logging {
ReplicaVerificationTool.dateFormat.format(new Date(Time.SYSTEM.milliseconds))
}
- def main(args: Array[String]): Unit = {
- val parser = new OptionParser(false)
- val brokerListOpt = parser.accepts("broker-list", "REQUIRED: The list of hostname and port of the server to connect to.")
- .withRequiredArg
- .describedAs("hostname:port,...,hostname:port")
- .ofType(classOf[String])
- val fetchSizeOpt = parser.accepts("fetch-size", "The fetch size of each request.")
- .withRequiredArg
- .describedAs("bytes")
- .ofType(classOf[java.lang.Integer])
- .defaultsTo(ConsumerConfig.DEFAULT_MAX_PARTITION_FETCH_BYTES)
- val maxWaitMsOpt = parser.accepts("max-wait-ms", "The max amount of time each fetch request waits.")
- .withRequiredArg
- .describedAs("ms")
- .ofType(classOf[java.lang.Integer])
- .defaultsTo(1000)
+ // Non-private for testing
+ sealed class ReplicaVerificationToolOptions(args: Array[String]) extends CommandDefaultOptions(args) {
+ private val brokerListOpt = parser.accepts("broker-list", "DEPRECATED, use --bootstrap-server instead; ignored if --bootstrap-server is specified. The list of hostname and port of the server to connect to.")
+ .withRequiredArg
+ .describedAs("HOST1:PORT1,...,HOST3:PORT3")
Review comment:
See later comment as for bootstrap-server.
##########
File path: core/src/test/scala/kafka/tools/ReplicaVerificationToolTest.scala
##########
@@ -17,14 +17,40 @@
package kafka.tools
+import kafka.tools.ReplicaVerificationTool.ReplicaVerificationToolOptions
import org.apache.kafka.common.TopicPartition
import org.apache.kafka.common.message.FetchResponseData
import org.apache.kafka.common.record.{CompressionType, MemoryRecords, SimpleRecord}
import org.junit.jupiter.api.Test
-import org.junit.jupiter.api.Assertions.assertTrue
+import org.junit.jupiter.api.Assertions.{assertEquals, assertThrows, assertTrue}
+import kafka.utils.Exit
class ReplicaVerificationToolTest {
+ @Test
+ def testExitWithoutBootstrapServers(): Unit = {
+ Exit.setExitProcedure {
+ (exitCode: Int, _: Option[String]) =>
+ assertEquals(1, exitCode)
+ throw new RuntimeException
+ }
+
+ try assertThrows(classOf[RuntimeException], () => new ReplicaVerificationToolOptions(Array("--fetch-size", "1024")))
+ finally Exit.resetExitProcedure()
+ }
+
+ @Test
+ def testConfigOptWithBootstrapServers(): Unit = {
+ val opts1 = new ReplicaVerificationToolOptions(Array("--bootstrap-server", "localhost:9092"))
+ assertEquals("localhost:9092", opts1.bootstrapServer)
+
+ val opts2 = new ReplicaVerificationToolOptions(Array("--broker-list", "127.0.0.1:9092"))
+ assertEquals("127.0.0.1:9092", opts2.bootstrapServer)
+
+ val opts3 = new ReplicaVerificationToolOptions(Array("--broker-list", "127.0.0.1:9092", "--bootstrap-server", "localhost:9092"))
+ assertEquals("localhost:9092", opts3.bootstrapServer)
+ }
Review comment:
We should have an assertion with multiple `--bootstrap-servers` given.
##########
File path: core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
##########
@@ -73,53 +74,61 @@ object ReplicaVerificationTool extends Logging {
ReplicaVerificationTool.dateFormat.format(new Date(Time.SYSTEM.milliseconds))
}
- def main(args: Array[String]): Unit = {
- val parser = new OptionParser(false)
- val brokerListOpt = parser.accepts("broker-list", "REQUIRED: The list of hostname and port of the server to connect to.")
- .withRequiredArg
- .describedAs("hostname:port,...,hostname:port")
- .ofType(classOf[String])
- val fetchSizeOpt = parser.accepts("fetch-size", "The fetch size of each request.")
- .withRequiredArg
- .describedAs("bytes")
- .ofType(classOf[java.lang.Integer])
- .defaultsTo(ConsumerConfig.DEFAULT_MAX_PARTITION_FETCH_BYTES)
- val maxWaitMsOpt = parser.accepts("max-wait-ms", "The max amount of time each fetch request waits.")
- .withRequiredArg
- .describedAs("ms")
- .ofType(classOf[java.lang.Integer])
- .defaultsTo(1000)
+ // Non-private for testing
+ sealed class ReplicaVerificationToolOptions(args: Array[String]) extends CommandDefaultOptions(args) {
+ private val brokerListOpt = parser.accepts("broker-list", "DEPRECATED, use --bootstrap-server instead; ignored if --bootstrap-server is specified. The list of hostname and port of the server to connect to.")
+ .withRequiredArg
+ .describedAs("HOST1:PORT1,...,HOST3:PORT3")
+ .ofType(classOf[String])
+ private val bootstrapServerOpt = parser.accepts("bootstrap-server", "REQUIRED. The list of hostname and port of the server to connect to.")
+ .requiredUnless("broker-list")
+ .withRequiredArg
+ .describedAs("HOST1:PORT1,...,HOST3:PORT3")
+ .ofType(classOf[String])
+ private val fetchSizeOpt = parser.accepts("fetch-size", "The fetch size of each request.")
+ .withRequiredArg
+ .describedAs("bytes")
+ .ofType(classOf[java.lang.Integer])
+ .defaultsTo(ConsumerConfig.DEFAULT_MAX_PARTITION_FETCH_BYTES)
+ private val maxWaitMsOpt = parser.accepts("max-wait-ms", "The max amount of time each fetch request waits.")
+ .withRequiredArg
+ .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.")
Review comment:
If we're changing this class we ought to deprecate this option and replace it with one with a less contentious name.
##########
File path: core/src/main/scala/kafka/tools/ReplicaVerificationTool.scala
##########
@@ -73,53 +74,61 @@ object ReplicaVerificationTool extends Logging {
ReplicaVerificationTool.dateFormat.format(new Date(Time.SYSTEM.milliseconds))
}
- def main(args: Array[String]): Unit = {
- val parser = new OptionParser(false)
- val brokerListOpt = parser.accepts("broker-list", "REQUIRED: The list of hostname and port of the server to connect to.")
- .withRequiredArg
- .describedAs("hostname:port,...,hostname:port")
- .ofType(classOf[String])
- val fetchSizeOpt = parser.accepts("fetch-size", "The fetch size of each request.")
- .withRequiredArg
- .describedAs("bytes")
- .ofType(classOf[java.lang.Integer])
- .defaultsTo(ConsumerConfig.DEFAULT_MAX_PARTITION_FETCH_BYTES)
- val maxWaitMsOpt = parser.accepts("max-wait-ms", "The max amount of time each fetch request waits.")
- .withRequiredArg
- .describedAs("ms")
- .ofType(classOf[java.lang.Integer])
- .defaultsTo(1000)
+ // Non-private for testing
+ sealed class ReplicaVerificationToolOptions(args: Array[String]) extends CommandDefaultOptions(args) {
+ private val brokerListOpt = parser.accepts("broker-list", "DEPRECATED, use --bootstrap-server instead; ignored if --bootstrap-server is specified. The list of hostname and port of the server to connect to.")
+ .withRequiredArg
+ .describedAs("HOST1:PORT1,...,HOST3:PORT3")
+ .ofType(classOf[String])
+ private val bootstrapServerOpt = parser.accepts("bootstrap-server", "REQUIRED. The list of hostname and port of the server to connect to.")
+ .requiredUnless("broker-list")
+ .withRequiredArg
+ .describedAs("HOST1:PORT1,...,HOST3:PORT3")
Review comment:
`TopicCommandOtions` and `ConfigCommandOptions` describes this as `.describedAs("server to connect to")`, so I guess we should do the same here.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org