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 2022/08/03 16:30:48 UTC

[GitHub] [kafka] mimaison opened a new pull request, #12480: MINOR: Small cleanups in integration.kafka tests

mimaison opened a new pull request, #12480:
URL: https://github.com/apache/kafka/pull/12480

   
   
   ### 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] showuon commented on a diff in pull request #12480: MINOR: Small cleanups in integration.kafka tests

Posted by GitBox <gi...@apache.org>.
showuon commented on code in PR #12480:
URL: https://github.com/apache/kafka/pull/12480#discussion_r954465533


##########
core/src/test/scala/integration/kafka/api/MetricsTest.scala:
##########
@@ -226,7 +226,7 @@ class MetricsTest extends IntegrationTestHarness with SaslSetup {
 
   private def verifyBrokerErrorMetrics(server: KafkaServer): Unit = {
 
-    def errorMetricCount = KafkaYammerMetrics.defaultRegistry.allMetrics.keySet.asScala.filter(_.getName == "ErrorsPerSec").size
+    def errorMetricCount = KafkaYammerMetrics.defaultRegistry.allMetrics.keySet.asScala.count(_.getName == "ErrorsPerSec")

Review Comment:
   nice



##########
core/src/test/scala/integration/kafka/api/SaslPlainSslEndToEndAuthorizationTest.scala:
##########
@@ -70,13 +70,12 @@ object SaslPlainSslEndToEndAuthorizationTest {
     def handle(callbacks: Array[Callback]): Unit = {
       var username: String = null
       for (callback <- callbacks) {
-        if (callback.isInstanceOf[NameCallback])
-          username = callback.asInstanceOf[NameCallback].getDefaultName
-        else if (callback.isInstanceOf[PlainAuthenticateCallback]) {
-          val plainCallback = callback.asInstanceOf[PlainAuthenticateCallback]
-          plainCallback.authenticated(Credentials.allUsers(username) == new String(plainCallback.password))
-        } else
-          throw new UnsupportedCallbackException(callback)
+        callback match {
+          case nameCallback: NameCallback => username = nameCallback.getDefaultName
+          case plainCallback: PlainAuthenticateCallback =>
+            plainCallback.authenticated(Credentials.allUsers(username) == new String(plainCallback.password))
+          case _ => throw new UnsupportedCallbackException(callback)
+        }

Review Comment:
   nice refactor!



-- 
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] clolov commented on pull request #12480: MINOR: Small cleanups in integration.kafka tests

Posted by GitBox <gi...@apache.org>.
clolov commented on PR #12480:
URL: https://github.com/apache/kafka/pull/12480#issuecomment-1204980467

   Looks great! 👍 


-- 
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] mimaison merged pull request #12480: MINOR: Small cleanups in integration.kafka tests

Posted by GitBox <gi...@apache.org>.
mimaison merged PR #12480:
URL: https://github.com/apache/kafka/pull/12480


-- 
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] mimaison commented on a diff in pull request #12480: MINOR: Small cleanups in integration.kafka tests

Posted by GitBox <gi...@apache.org>.
mimaison commented on code in PR #12480:
URL: https://github.com/apache/kafka/pull/12480#discussion_r939052756


##########
core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala:
##########
@@ -1249,7 +1244,7 @@ class DynamicBrokerReconfigurationTest extends QuorumTestHarness with SaslSetup
     verifyTimeout(consumerFuture)
   }
 
-  private def verifyListener(securityProtocol: SecurityProtocol, saslMechanism: Option[String], groupId: String): Unit = {
+  private def verifyListener(securityProtocol: SecurityProtocol, saslMechanism: Option[String]): Unit = {

Review Comment:
   True, we can pass `groupId` into `ConsumerBuilder`. I've pushed an update. 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] mimaison commented on pull request #12480: MINOR: Small cleanups in integration.kafka tests

Posted by GitBox <gi...@apache.org>.
mimaison commented on PR #12480:
URL: https://github.com/apache/kafka/pull/12480#issuecomment-1226892788

   None of the failures are related, merging to trunk


-- 
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] mimaison commented on a diff in pull request #12480: MINOR: Small cleanups in integration.kafka tests

Posted by GitBox <gi...@apache.org>.
mimaison commented on code in PR #12480:
URL: https://github.com/apache/kafka/pull/12480#discussion_r954630874


##########
core/src/test/scala/integration/kafka/api/MetricsTest.scala:
##########
@@ -226,7 +226,7 @@ class MetricsTest extends IntegrationTestHarness with SaslSetup {
 
   private def verifyBrokerErrorMetrics(server: KafkaServer): Unit = {
 
-    def errorMetricCount = KafkaYammerMetrics.defaultRegistry.allMetrics.keySet.asScala.filter(_.getName == "ErrorsPerSec").size
+    def errorMetricCount = KafkaYammerMetrics.defaultRegistry.allMetrics.keySet.asScala.count(_.getName == "ErrorsPerSec")

Review Comment:
   Credits for finding that one goes to IntelliJ :grin:



-- 
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] showuon commented on pull request #12480: MINOR: Small cleanups in integration.kafka tests

Posted by GitBox <gi...@apache.org>.
showuon commented on PR #12480:
URL: https://github.com/apache/kafka/pull/12480#issuecomment-1226729774

   Jenkins build seems failed with infra's issue. I've re-trigger it: https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-12480/3/


-- 
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] divijvaidya commented on a diff in pull request #12480: MINOR: Small cleanups in integration.kafka tests

Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #12480:
URL: https://github.com/apache/kafka/pull/12480#discussion_r937932086


##########
core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala:
##########
@@ -217,7 +213,7 @@ class DynamicBrokerReconfigurationTest extends QuorumTestHarness with SaslSetup
       keyStoreProps.forEach { configName =>
         val desc = configEntry(configDesc, s"$prefix$configName")
         val isSensitive = configName.contains("password")
-        verifyConfig(configName, desc, isSensitive, isReadOnly = prefix.nonEmpty, if (prefix.isEmpty) invalidSslProperties else sslProperties1)
+        verifyConfig(configName, desc, isSensitive, isReadOnly = prefix.nonEmpty, expectedProps)

Review Comment:
   nice!



##########
core/src/test/scala/integration/kafka/server/DynamicBrokerReconfigurationTest.scala:
##########
@@ -1249,7 +1244,7 @@ class DynamicBrokerReconfigurationTest extends QuorumTestHarness with SaslSetup
     verifyTimeout(consumerFuture)
   }
 
-  private def verifyListener(securityProtocol: SecurityProtocol, saslMechanism: Option[String], groupId: String): Unit = {
+  private def verifyListener(securityProtocol: SecurityProtocol, saslMechanism: Option[String]): Unit = {

Review Comment:
   Instead of removing the group as a parameter (I understand that it is unused), please fix the validation by adding the group ID in the consumer below?



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