You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "FrankYang0529 (via GitHub)" <gi...@apache.org> on 2024/12/27 11:39:51 UTC

[PR] KAFKA-17631: Convert SaslApiVersionsRequestTest to kraft [kafka]

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

   We disabled `SaslApiVersionsRequestTest` when implementing KAFKA-17614. Re-enable with a new test framework.
   
   This PR is based on https://github.com/apache/kafka/pull/17671, so we can just review the last commit. I will rebase code after https://github.com/apache/kafka/pull/17671 is merged.
   
   ### 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

Re: [PR] KAFKA-17631: Convert SaslApiVersionsRequestTest to kraft [kafka]

Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #18330:
URL: https://github.com/apache/kafka/pull/18330#issuecomment-2614027958

   @FrankYang0529 could you please fix the conflicts?


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

Re: [PR] KAFKA-17631: Convert SaslApiVersionsRequestTest to kraft [kafka]

Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on code in PR #18330:
URL: https://github.com/apache/kafka/pull/18330#discussion_r1939313578


##########
core/src/test/scala/unit/kafka/server/SaslApiVersionsRequestTest.scala:
##########
@@ -112,20 +82,18 @@ class SaslApiVersionsRequestTest(cluster: ClusterInstance) extends AbstractApiVe
       assertEquals(Errors.UNSUPPORTED_VERSION.code, apiVersionsResponse.data.errorCode)
       val apiVersionsResponse2 = IntegrationTestUtils.sendAndReceive[ApiVersionsResponse](
         new ApiVersionsRequest.Builder().build(0), socket)
-      validateApiVersionsResponse(apiVersionsResponse2,
+      validateApiVersionsResponse(
+        apiVersionsResponse2,
         enableUnstableLastVersion = !"false".equals(
-          cluster.config().serverProperties().get("unstable.api.versions.enable")))
+          cluster.config().serverProperties().get("unstable.api.versions.enable")),
+        apiVersion = 0.toShort
+      )
       sendSaslHandshakeRequestValidateResponse(socket)
     } finally {
       socket.close()
     }
   }
 
-  @AfterEach
-  def closeSasl(): Unit = {
-    sasl.closeSasl()

Review Comment:
   I think we can keep it as current implementation since we have thread leak detection. We can catch error if there is thread leak.



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

Re: [PR] KAFKA-17631: Convert SaslApiVersionsRequestTest to kraft [kafka]

Posted by "FrankYang0529 (via GitHub)" <gi...@apache.org>.
FrankYang0529 commented on PR #18330:
URL: https://github.com/apache/kafka/pull/18330#issuecomment-2614034121

   > @FrankYang0529 could you please fix the conflicts?
   
   Resolved conflict. 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

Re: [PR] KAFKA-17631: Convert SaslApiVersionsRequestTest to kraft [kafka]

Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #18330:
URL: https://github.com/apache/kafka/pull/18330#discussion_r1939031559


##########
core/src/test/scala/unit/kafka/server/SaslApiVersionsRequestTest.scala:
##########
@@ -112,20 +82,18 @@ class SaslApiVersionsRequestTest(cluster: ClusterInstance) extends AbstractApiVe
       assertEquals(Errors.UNSUPPORTED_VERSION.code, apiVersionsResponse.data.errorCode)
       val apiVersionsResponse2 = IntegrationTestUtils.sendAndReceive[ApiVersionsResponse](
         new ApiVersionsRequest.Builder().build(0), socket)
-      validateApiVersionsResponse(apiVersionsResponse2,
+      validateApiVersionsResponse(
+        apiVersionsResponse2,
         enableUnstableLastVersion = !"false".equals(
-          cluster.config().serverProperties().get("unstable.api.versions.enable")))
+          cluster.config().serverProperties().get("unstable.api.versions.enable")),
+        apiVersion = 0.toShort
+      )
       sendSaslHandshakeRequestValidateResponse(socket)
     } finally {
       socket.close()
     }
   }
 
-  @AfterEach
-  def closeSasl(): Unit = {
-    sasl.closeSasl()

Review Comment:
   I'm considering whether we need to close all login modules within Testkit. However, these modules are primarily used to prevent consumer leaks, and we already have thread detection mechanisms in place. @FrankYang0529 WDYT?



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

Re: [PR] KAFKA-17631: Convert SaslApiVersionsRequestTest to kraft [kafka]

Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #18330:
URL: https://github.com/apache/kafka/pull/18330


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