[GitHub] [kafka] omkreddy commented on a change in pull request #8184: KAKFA-9612 CLI Dynamic Configuration with file input

[GitBox <gi...@apache.org>]

omkreddy commented on a change in pull request #8184:
URL: https://github.com/apache/kafka/pull/8184#discussion_r415015661



##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -40,8 +41,8 @@ import org.junit.Assert._
 import org.junit.Test
 import org.scalatest.Assertions.intercept
 
+import scala.collection.JavaConverters._

Review comment:
       `JavaConverters` is deprecated.
    we need to use `scala.jdk.CollectionConverters._` (deleted line below)

##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -246,6 +247,18 @@ object ConfigCommand extends Config {
 
   private[admin] def parseConfigsToBeAdded(opts: ConfigCommandOptions): Properties = {
     val props = new Properties
+    if (opts.options.has(opts.addConfigFile)) {
+      val file = opts.options.valueOf(opts.addConfigFile)
+      val inputStream = try new FileInputStream(file) catch {

Review comment:
       maybe we can just reuse `Utils.loadProps()` util method.

##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -643,6 +656,9 @@ object ConfigCommand extends Config {
             s"Entity types '${ConfigType.User}' and '${ConfigType.Client}' may be specified together to update config for clients of a specific user.")
             .withRequiredArg
             .ofType(classOf[String])
+    val addConfigFile = parser.accepts("add-config-file", "Path to a properties file with configs to add. See add-config for a list of valid configurations.")
+            .withRequiredArg
+            .ofType(classOf[File])

Review comment:
       we can use String  type and use `Utils.loadProps()` to load

##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -226,6 +240,51 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
     assertTrue(addedProps2.getProperty("f").isEmpty)
   }
 
+  @Test(expected = classOf[IllegalArgumentException])
+  def shouldFailIfAddAndAddFile(): Unit = {
+    // Should not parse correctly
+    val createOpts = new ConfigCommandOptions(Array("--bootstrap-server", "localhost:9092",
+      "--entity-name", "1",
+      "--entity-type", "brokers",
+      "--alter",
+      "--add-config", "a=b,c=d",
+      "--add-config-file", "/tmp/new.properties"
+    ))
+    createOpts.checkArgs()
+  }
+
+  @Test
+  def testParseConfigsToBeAddedForAddConfigFile(): Unit = {
+    val fileContents =
+      """a=b
+        |c = d
+        |json = {"key": "val"}
+        |nested = [[1, 2], [3, 4]]
+        |""".stripMargin
+
+    val file = File.createTempFile("testParseConfigsToBeAddedForAddConfigFile", ".properties")

Review comment:
       we can use `TestUtils.tempFile()`

##########
File path: core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala
##########
@@ -424,12 +483,33 @@ class ConfigCommandTest extends ZooKeeperTestHarness with Logging {
 
   @Test
   def shouldAlterTopicConfig(): Unit = {
+    doShouldAlterTopicConfig(false)
+  }
+
+  @Test
+  def shouldAlterTopicConfigFile(): Unit = {
+    doShouldAlterTopicConfig(true)
+  }
+
+  def doShouldAlterTopicConfig(file: Boolean): Unit = {
+    var filePath = ""
+    val addedConfigs = Seq("delete.retention.ms=1000000", "min.insync.replicas=2")
+    if (file) {
+      val file = File.createTempFile("testParseConfigsToBeAddedForAddConfigFile", ".properties")

Review comment:
       TestUtils.tempFile().  we can also add a new util method  `TestUtils.tempFile(String data)` and use at both the places. 




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