[GitHub] [kafka] omkreddy commented on a change in pull request #8808: KAFKA-10109: Fix double AdminClient creation in AclCommand

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

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



##########
File path: core/src/test/scala/unit/kafka/admin/AclCommandTest.scala
##########
@@ -161,10 +193,34 @@ class AclCommandTest extends ZooKeeperTestHarness with Logging {
     testProducerConsumerCli(adminArgs)
   }
 
+  @Test
+  def testAclCliWithClientId(): Unit = {
+    val adminClientConfig = File.createTempFile(classOf[AclCommandTest].getName, "createServer")

Review comment:
       We can use `TestUtils.tempFile/TestUtils.tempFile(String)`.

##########
File path: core/src/test/scala/unit/kafka/admin/AclCommandTest.scala
##########
@@ -161,10 +193,34 @@ class AclCommandTest extends ZooKeeperTestHarness with Logging {
     testProducerConsumerCli(adminArgs)
   }
 
+  @Test
+  def testAclCliWithClientId(): Unit = {
+    val adminClientConfig = File.createTempFile(classOf[AclCommandTest].getName, "createServer")
+    adminClientConfig.deleteOnExit()
+    val pw = new PrintWriter(adminClientConfig)
+    pw.println("client.id=my-client")
+    pw.close()
+
+    createServer(Some(adminClientConfig))
+
+    val appender = LogCaptureAppender.createAndRegister()
+    val previousLevel = LogCaptureAppender.setClassLoggerLevel(classOf[AppInfoParser], Level.WARN)
+
+    testAclCli(adminArgs)
+
+    LogCaptureAppender.setClassLoggerLevel(classOf[AppInfoParser], previousLevel)

Review comment:
       we may  want to reset it in `finally` block?

##########
File path: core/src/main/scala/kafka/admin/AclCommand.scala
##########
@@ -130,33 +130,39 @@ object AclCommand extends Logging {
           }
         }
 
-        listAcls()
+        listAcls(adminClient)
       }
     }
 
     def listAcls(): Unit = {
       withAdminClient(opts) { adminClient =>
-        val filters = getResourceFilter(opts, dieIfNoResourceFound = false)
-        val listPrincipals = getPrincipals(opts, opts.listPrincipalsOpt)
-        val resourceToAcls = getAcls(adminClient, filters)
+        listAcls(adminClient)
+      }
+    }
 
-        if (listPrincipals.isEmpty) {
-          for ((resource, acls) <- resourceToAcls)
-            println(s"Current ACLs for resource `$resource`: $Newline ${acls.map("\t" + _).mkString(Newline)} $Newline")
-        } else {
-          listPrincipals.foreach(principal => {
-            println(s"ACLs for principal `$principal`")
-            val filteredResourceToAcls =  resourceToAcls.map { case (resource, acls) =>
-              resource -> acls.filter(acl => principal.toString.equals(acl.principal))
-            }.filter { case (_, acls) => acls.nonEmpty }
+    private def listAcls(adminClient: Admin) = {

Review comment:
       nit: Can we add `: Unit` return type here and  in other new methods?




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