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/01/20 17:11:55 UTC

[GitHub] [kafka] dajac opened a new pull request #9938: MINOR: Refactor DescribeAuthorizedOperationsTest

dajac opened a new pull request #9938:
URL: https://github.com/apache/kafka/pull/9938


   I was looking at `DescribeAuthorizedOperationsTest` in the context of KIP-700. I have made few changes to improve the readability of the code/tests. The PR only moves code around.
   
   ### 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.

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

[GitHub] [kafka] chia7712 commented on a change in pull request #9938: MINOR: Refactor DescribeAuthorizedOperationsTest

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



##########
File path: core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -32,17 +32,55 @@ import org.junit.jupiter.api.{AfterEach, BeforeEach, Test}
 
 import scala.jdk.CollectionConverters._
 
+object DescribeAuthorizedOperationsTest {
+  val Group1 = "group1"

Review comment:
       Is ```GROUP_1" more better? it is global variable in this test.

##########
File path: core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -86,54 +122,36 @@ class DescribeAuthorizedOperationsTest extends IntegrationTestHarness with SaslS
     closeSasl()
   }
 
-  val group1Acl = new AclBinding(new ResourcePattern(ResourceType.GROUP, group1, PatternType.LITERAL),
-    new AccessControlEntry("User:" + JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, "*", AclOperation.ALL, AclPermissionType.ALLOW))
-
-  val group2Acl = new AclBinding(new ResourcePattern(ResourceType.GROUP, group2, PatternType.LITERAL),
-    new AccessControlEntry("User:" + JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, "*", AclOperation.DESCRIBE, AclPermissionType.ALLOW))
-
-  val group3Acl = new AclBinding(new ResourcePattern(ResourceType.GROUP, group3, PatternType.LITERAL),
-    new AccessControlEntry("User:" + JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, "*", AclOperation.DELETE, AclPermissionType.ALLOW))
-
-  val clusterAllAcl = new AclBinding(new ResourcePattern(ResourceType.CLUSTER, Resource.CLUSTER_NAME, PatternType.LITERAL),
-    new AccessControlEntry("User:" + JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, "*", AclOperation.ALL, AclPermissionType.ALLOW))
-
-  val topic1Acl = new AclBinding(new ResourcePattern(ResourceType.TOPIC, topic1, PatternType.LITERAL),
-    new AccessControlEntry("User:" + JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, "*", AclOperation.ALL, AclPermissionType.ALLOW))
-
-  val topic2All = new AclBinding(new ResourcePattern(ResourceType.TOPIC, topic2, PatternType.LITERAL),
-    new AccessControlEntry("User:" + JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, "*", AclOperation.DELETE, AclPermissionType.ALLOW))
-
-  def createConfig(): Properties = {
+  private def createConfig(): Properties = {
     val adminClientConfig = new Properties()
     adminClientConfig.put(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG, brokerList)
     adminClientConfig.put(AdminClientConfig.REQUEST_TIMEOUT_MS_CONFIG, "20000")
     val securityProps: util.Map[Object, Object] =
       TestUtils.adminClientSecurityConfigs(securityProtocol, trustStoreFile, clientSaslProperties)
-    securityProps.forEach { (key, value) => adminClientConfig.put(key.asInstanceOf[String], value) }
+    securityProps.forEach((key, value) => adminClientConfig.put(key.asInstanceOf[String], value))

Review comment:
       How about ```adminClientConfig.putAll(securityProps)```?

##########
File path: core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -32,17 +32,55 @@ import org.junit.jupiter.api.{AfterEach, BeforeEach, Test}
 
 import scala.jdk.CollectionConverters._
 
+object DescribeAuthorizedOperationsTest {
+  val Group1 = "group1"
+  val Group2 = "group2"
+  val Group3 = "group3"
+  val Topic1 = "topic1"
+  val Topic2 = "topic2"
+
+  val Group1Acl = new AclBinding(
+    new ResourcePattern(ResourceType.GROUP, Group1, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, ALL, ALLOW))
+
+  val Group2Acl = new AclBinding(
+    new ResourcePattern(ResourceType.GROUP, Group2, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, DESCRIBE, ALLOW))
+
+  val Group3Acl = new AclBinding(
+    new ResourcePattern(ResourceType.GROUP, Group3, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, DELETE, ALLOW))
+
+  val ClusterAllAcl = new AclBinding(
+    new ResourcePattern(ResourceType.CLUSTER, Resource.CLUSTER_NAME, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, ALL, ALLOW))
+
+  val Topic1Acl = new AclBinding(
+    new ResourcePattern(ResourceType.TOPIC, Topic1, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, ALL, ALLOW))
+
+  val Topic2All = new AclBinding(
+    new ResourcePattern(ResourceType.TOPIC, Topic2, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, DELETE, ALLOW))
+
+  private def accessControlEntry(
+    userName: String,
+    operation: AclOperation,
+    permissionType: AclPermissionType
+  ): AccessControlEntry = {
+    new AccessControlEntry(new KafkaPrincipal(KafkaPrincipal.USER_TYPE, userName).toString,
+      AclEntry.WildcardHost, operation, permissionType)
+  }
+}
+
 class DescribeAuthorizedOperationsTest extends IntegrationTestHarness with SaslSetup {
+  import DescribeAuthorizedOperationsTest._
+
   override val brokerCount = 1
   this.serverConfig.setProperty(KafkaConfig.ZkEnableSecureAclsProp, "true")
   this.serverConfig.setProperty(KafkaConfig.AuthorizerClassNameProp, classOf[AclAuthorizer].getName)
 
   var client: Admin = _

Review comment:
       all test cases need client so we should create it in construction or ```BeforeEach``` block.

##########
File path: core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -32,17 +32,55 @@ import org.junit.jupiter.api.{AfterEach, BeforeEach, Test}
 
 import scala.jdk.CollectionConverters._
 
+object DescribeAuthorizedOperationsTest {
+  val Group1 = "group1"
+  val Group2 = "group2"
+  val Group3 = "group3"
+  val Topic1 = "topic1"
+  val Topic2 = "topic2"
+
+  val Group1Acl = new AclBinding(
+    new ResourcePattern(ResourceType.GROUP, Group1, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, ALL, ALLOW))
+
+  val Group2Acl = new AclBinding(
+    new ResourcePattern(ResourceType.GROUP, Group2, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, DESCRIBE, ALLOW))
+
+  val Group3Acl = new AclBinding(
+    new ResourcePattern(ResourceType.GROUP, Group3, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, DELETE, ALLOW))
+
+  val ClusterAllAcl = new AclBinding(
+    new ResourcePattern(ResourceType.CLUSTER, Resource.CLUSTER_NAME, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, ALL, ALLOW))
+
+  val Topic1Acl = new AclBinding(
+    new ResourcePattern(ResourceType.TOPIC, Topic1, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, ALL, ALLOW))
+
+  val Topic2All = new AclBinding(
+    new ResourcePattern(ResourceType.TOPIC, Topic2, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, DELETE, ALLOW))
+
+  private def accessControlEntry(
+    userName: String,
+    operation: AclOperation,
+    permissionType: AclPermissionType

Review comment:
       It seems this filed is always ```ALLOW```

##########
File path: core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -73,11 +114,6 @@ class DescribeAuthorizedOperationsTest extends IntegrationTestHarness with SaslS
     TestUtils.waitUntilBrokerMetadataIsPropagated(servers)
   }
 
-  private def accessControlEntry(userName: String, permissionType: AclPermissionType, operation: AclOperation): AccessControlEntry = {
-    new AccessControlEntry(new KafkaPrincipal(KafkaPrincipal.USER_TYPE, userName).toString,
-      AclEntry.WildcardHost, operation, permissionType)
-  }
-
   @AfterEach
   override def tearDown(): Unit = {
     if (client != null)

Review comment:
       unnecessary 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.

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

[GitHub] [kafka] dajac commented on a change in pull request #9938: MINOR: Refactor DescribeAuthorizedOperationsTest

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



##########
File path: core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -32,17 +32,55 @@ import org.junit.jupiter.api.{AfterEach, BeforeEach, Test}
 
 import scala.jdk.CollectionConverters._
 
+object DescribeAuthorizedOperationsTest {
+  val Group1 = "group1"
+  val Group2 = "group2"
+  val Group3 = "group3"
+  val Topic1 = "topic1"
+  val Topic2 = "topic2"
+
+  val Group1Acl = new AclBinding(
+    new ResourcePattern(ResourceType.GROUP, Group1, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, ALL, ALLOW))
+
+  val Group2Acl = new AclBinding(
+    new ResourcePattern(ResourceType.GROUP, Group2, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, DESCRIBE, ALLOW))
+
+  val Group3Acl = new AclBinding(
+    new ResourcePattern(ResourceType.GROUP, Group3, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, DELETE, ALLOW))
+
+  val ClusterAllAcl = new AclBinding(
+    new ResourcePattern(ResourceType.CLUSTER, Resource.CLUSTER_NAME, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, ALL, ALLOW))
+
+  val Topic1Acl = new AclBinding(
+    new ResourcePattern(ResourceType.TOPIC, Topic1, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, ALL, ALLOW))
+
+  val Topic2All = new AclBinding(
+    new ResourcePattern(ResourceType.TOPIC, Topic2, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, DELETE, ALLOW))
+
+  private def accessControlEntry(
+    userName: String,
+    operation: AclOperation,
+    permissionType: AclPermissionType

Review comment:
       Indeed, let me remove that field.




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

[GitHub] [kafka] dajac commented on a change in pull request #9938: MINOR: Refactor DescribeAuthorizedOperationsTest

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



##########
File path: core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -73,11 +114,6 @@ class DescribeAuthorizedOperationsTest extends IntegrationTestHarness with SaslS
     TestUtils.waitUntilBrokerMetadataIsPropagated(servers)
   }
 
-  private def accessControlEntry(userName: String, permissionType: AclPermissionType, operation: AclOperation): AccessControlEntry = {
-    new AccessControlEntry(new KafkaPrincipal(KafkaPrincipal.USER_TYPE, userName).toString,
-      AclEntry.WildcardHost, operation, permissionType)
-  }
-
   @AfterEach
   override def tearDown(): Unit = {
     if (client != null)

Review comment:
       I think that it is better to keep it as `client` could not be initialized. For instance, let's say that there is an exception raised when the client is constructed. In this case, `client` would remain `null`.




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

[GitHub] [kafka] chia7712 commented on a change in pull request #9938: MINOR: Refactor DescribeAuthorizedOperationsTest

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



##########
File path: core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -73,11 +114,6 @@ class DescribeAuthorizedOperationsTest extends IntegrationTestHarness with SaslS
     TestUtils.waitUntilBrokerMetadataIsPropagated(servers)
   }
 
-  private def accessControlEntry(userName: String, permissionType: AclPermissionType, operation: AclOperation): AccessControlEntry = {
-    new AccessControlEntry(new KafkaPrincipal(KafkaPrincipal.USER_TYPE, userName).toString,
-      AclEntry.WildcardHost, operation, permissionType)
-  }
-
   @AfterEach
   override def tearDown(): Unit = {
     if (client != null)

Review comment:
       Sorry for unclear comment. 
   
   My point was ```Utils.closeQuietly``` includes null check so we don't need to add same check again.




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

[GitHub] [kafka] dajac merged pull request #9938: MINOR: Refactor DescribeAuthorizedOperationsTest

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


   


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

[GitHub] [kafka] dajac commented on a change in pull request #9938: MINOR: Refactor DescribeAuthorizedOperationsTest

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



##########
File path: core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -32,17 +32,55 @@ import org.junit.jupiter.api.{AfterEach, BeforeEach, Test}
 
 import scala.jdk.CollectionConverters._
 
+object DescribeAuthorizedOperationsTest {
+  val Group1 = "group1"

Review comment:
       In Scala, we tend to only capitalize the first letter of constants. At least, this how I have been doing it so far. I prefer to keep it as is.




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

[GitHub] [kafka] dajac commented on pull request #9938: MINOR: Refactor DescribeAuthorizedOperationsTest

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


   The failed test is not related to this PR:
   * Build / JDK 15 / org.apache.kafka.clients.consumer.internals.FetcherTest.testEarlierOffsetResetArrivesLate()
   


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

[GitHub] [kafka] chia7712 commented on a change in pull request #9938: MINOR: Refactor DescribeAuthorizedOperationsTest

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



##########
File path: core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -73,11 +114,6 @@ class DescribeAuthorizedOperationsTest extends IntegrationTestHarness with SaslS
     TestUtils.waitUntilBrokerMetadataIsPropagated(servers)
   }
 
-  private def accessControlEntry(userName: String, permissionType: AclPermissionType, operation: AclOperation): AccessControlEntry = {
-    new AccessControlEntry(new KafkaPrincipal(KafkaPrincipal.USER_TYPE, userName).toString,
-      AclEntry.WildcardHost, operation, permissionType)
-  }
-
   @AfterEach
   override def tearDown(): Unit = {
     if (client != null)

Review comment:
       Sorry for unclear comment. 
   
   My point was ```Utils.closeQuietly``` includes null check so we don't need to add same check again.




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

[GitHub] [kafka] dajac merged pull request #9938: MINOR: Refactor DescribeAuthorizedOperationsTest

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


   


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

[GitHub] [kafka] dajac commented on pull request #9938: MINOR: Refactor DescribeAuthorizedOperationsTest

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






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

[GitHub] [kafka] dajac commented on a change in pull request #9938: MINOR: Refactor DescribeAuthorizedOperationsTest

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



##########
File path: core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -73,11 +114,6 @@ class DescribeAuthorizedOperationsTest extends IntegrationTestHarness with SaslS
     TestUtils.waitUntilBrokerMetadataIsPropagated(servers)
   }
 
-  private def accessControlEntry(userName: String, permissionType: AclPermissionType, operation: AclOperation): AccessControlEntry = {
-    new AccessControlEntry(new KafkaPrincipal(KafkaPrincipal.USER_TYPE, userName).toString,
-      AclEntry.WildcardHost, operation, permissionType)
-  }
-
   @AfterEach
   override def tearDown(): Unit = {
     if (client != null)

Review comment:
       Got it, thanks for the clarification. I have removed the null 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.

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

[GitHub] [kafka] dajac commented on a change in pull request #9938: MINOR: Refactor DescribeAuthorizedOperationsTest

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



##########
File path: core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -32,17 +32,55 @@ import org.junit.jupiter.api.{AfterEach, BeforeEach, Test}
 
 import scala.jdk.CollectionConverters._
 
+object DescribeAuthorizedOperationsTest {
+  val Group1 = "group1"
+  val Group2 = "group2"
+  val Group3 = "group3"
+  val Topic1 = "topic1"
+  val Topic2 = "topic2"
+
+  val Group1Acl = new AclBinding(
+    new ResourcePattern(ResourceType.GROUP, Group1, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, ALL, ALLOW))
+
+  val Group2Acl = new AclBinding(
+    new ResourcePattern(ResourceType.GROUP, Group2, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, DESCRIBE, ALLOW))
+
+  val Group3Acl = new AclBinding(
+    new ResourcePattern(ResourceType.GROUP, Group3, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, DELETE, ALLOW))
+
+  val ClusterAllAcl = new AclBinding(
+    new ResourcePattern(ResourceType.CLUSTER, Resource.CLUSTER_NAME, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, ALL, ALLOW))
+
+  val Topic1Acl = new AclBinding(
+    new ResourcePattern(ResourceType.TOPIC, Topic1, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, ALL, ALLOW))
+
+  val Topic2All = new AclBinding(
+    new ResourcePattern(ResourceType.TOPIC, Topic2, PatternType.LITERAL),
+    accessControlEntry(JaasTestUtils.KafkaClientPrincipalUnqualifiedName2, DELETE, ALLOW))
+
+  private def accessControlEntry(
+    userName: String,
+    operation: AclOperation,
+    permissionType: AclPermissionType

Review comment:
       Indeed, let me remove that field.

##########
File path: core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -32,17 +32,55 @@ import org.junit.jupiter.api.{AfterEach, BeforeEach, Test}
 
 import scala.jdk.CollectionConverters._
 
+object DescribeAuthorizedOperationsTest {
+  val Group1 = "group1"

Review comment:
       In Scala, we tend to only capitalize the first letter of constants. At least, this how I have been doing it so far. I prefer to keep it as is.

##########
File path: core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -73,11 +114,6 @@ class DescribeAuthorizedOperationsTest extends IntegrationTestHarness with SaslS
     TestUtils.waitUntilBrokerMetadataIsPropagated(servers)
   }
 
-  private def accessControlEntry(userName: String, permissionType: AclPermissionType, operation: AclOperation): AccessControlEntry = {
-    new AccessControlEntry(new KafkaPrincipal(KafkaPrincipal.USER_TYPE, userName).toString,
-      AclEntry.WildcardHost, operation, permissionType)
-  }
-
   @AfterEach
   override def tearDown(): Unit = {
     if (client != null)

Review comment:
       I think that it is better to keep it as `client` could not be initialized. For instance, let's say that there is an exception raised when the client is constructed. In this case, `client` would remain `null`.

##########
File path: core/src/test/scala/integration/kafka/api/DescribeAuthorizedOperationsTest.scala
##########
@@ -73,11 +114,6 @@ class DescribeAuthorizedOperationsTest extends IntegrationTestHarness with SaslS
     TestUtils.waitUntilBrokerMetadataIsPropagated(servers)
   }
 
-  private def accessControlEntry(userName: String, permissionType: AclPermissionType, operation: AclOperation): AccessControlEntry = {
-    new AccessControlEntry(new KafkaPrincipal(KafkaPrincipal.USER_TYPE, userName).toString,
-      AclEntry.WildcardHost, operation, permissionType)
-  }
-
   @AfterEach
   override def tearDown(): Unit = {
     if (client != null)

Review comment:
       Got it, thanks for the clarification. I have removed the null 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.

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

[GitHub] [kafka] dajac commented on pull request #9938: MINOR: Refactor DescribeAuthorizedOperationsTest

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


   @chia7712 Thanks for your review. I have addressed your comments. Do you mind having a second quick look at it?


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