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/04/14 05:08:32 UTC

[GitHub] [kafka] dengziming opened a new pull request #10535: MINOR: Remove duplicate method in test classes

dengziming opened a new pull request #10535:
URL: https://github.com/apache/kafka/pull/10535


   *More detailed description of your change*
   1. Remove duplicate serializing auto-generated data in `RequestConvertToJsonTest`, this is inspired by #9964
   2. Remove `RequestTestUtils.serializeRequestWithHeader` since we added a `AbstractRequest.serializeWithHeader` in #10142
   
   *Summary of testing strategy (including rationale)*
   Unit test
   
   ### 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 pull request #10535: MINOR: Remove duplicate method in test classes

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


   @dengziming Could you merge trunk to trigger QA again? There are some flaky fixes which can stabilize the QA.


-- 
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] dengziming commented on a change in pull request #10535: MINOR: Remove duplicate method in test classes

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



##########
File path: core/src/test/scala/unit/kafka/server/AbstractApiVersionsRequestTest.scala
##########
@@ -49,7 +49,9 @@ abstract class AbstractApiVersionsRequestTest(cluster: ClusterInstance) {
     val overrideHeader = IntegrationTestUtils.nextRequestHeader(ApiKeys.API_VERSIONS, Short.MaxValue)
     val socket = IntegrationTestUtils.connect(cluster.brokerSocketServers().asScala.head, cluster.clientListener())
     try {
-      IntegrationTestUtils.sendWithHeader(request, overrideHeader, socket)
+      val serializedBytes = Utils.toArray(

Review comment:
       Because this method is used to test the version inconsistency between request header and request body, but `IntegrationTestUtils.sendWithHeader` will inspect the version and throws `IllegalArgumentException` on inconsistency after this PR.




-- 
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 #10535: MINOR: Remove duplicate method in test classes

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



##########
File path: core/src/test/scala/unit/kafka/server/AbstractApiVersionsRequestTest.scala
##########
@@ -49,7 +49,9 @@ abstract class AbstractApiVersionsRequestTest(cluster: ClusterInstance) {
     val overrideHeader = IntegrationTestUtils.nextRequestHeader(ApiKeys.API_VERSIONS, Short.MaxValue)
     val socket = IntegrationTestUtils.connect(cluster.brokerSocketServers().asScala.head, cluster.clientListener())
     try {
-      IntegrationTestUtils.sendWithHeader(request, overrideHeader, socket)
+      val serializedBytes = Utils.toArray(

Review comment:
       Why this change is necessary? It seems to me the code gets more complicated.




-- 
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] dengziming commented on pull request #10535: MINOR: Remove duplicate method in test classes

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


   `AbstractRequest.serializeWithHeader` will verify apiVersion of request body and header so `ApiVersionsRequestTest.testApiVersionsRequestWithUnsupportedVersion` failed, I just fixed this.


-- 
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 #10535: MINOR: Remove duplicate method in test classes

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



##########
File path: core/src/test/scala/unit/kafka/server/BaseRequestTest.scala
##########
@@ -136,7 +135,7 @@ abstract class BaseRequestTest extends IntegrationTestHarness {
   }
 
   def sendWithHeader(request: AbstractRequest, header: RequestHeader, socket: Socket): Unit = {
-    val serializedBytes = Utils.toArray(RequestTestUtils.serializeRequestWithHeader(header, request))
+    val serializedBytes = Utils.toArray(request.serializeWithHeader(header))
     sendRequest(socket, serializedBytes)

Review comment:
       sounds good to me




-- 
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] dengziming commented on pull request #10535: MINOR: Remove duplicate method in test classes

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


   @chia7712 , PTAL at `RequestConvertToJsonTest` since it's the same with your refactor in #9964;
   @abbccdda , I remove `RequestTestUtils.serializeResponseWithHeader` since it's the same with `AbstractRequest.serializeWithHeader` you added in #10142; 
   Thank you!


-- 
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 #10535: MINOR: Remove duplicate method in test classes

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



##########
File path: core/src/test/scala/unit/kafka/server/BaseRequestTest.scala
##########
@@ -136,7 +135,7 @@ abstract class BaseRequestTest extends IntegrationTestHarness {
   }
 
   def sendWithHeader(request: AbstractRequest, header: RequestHeader, socket: Socket): Unit = {
-    val serializedBytes = Utils.toArray(RequestTestUtils.serializeRequestWithHeader(header, request))
+    val serializedBytes = Utils.toArray(request.serializeWithHeader(header))
     sendRequest(socket, serializedBytes)

Review comment:
       It seems this method can be replaced by `IntegrationTestUtils.sendRequest`




-- 
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 merged pull request #10535: MINOR: Remove duplicate method in test classes

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


   


-- 
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] dengziming commented on a change in pull request #10535: MINOR: Remove duplicate method in test classes

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



##########
File path: core/src/test/scala/unit/kafka/server/BaseRequestTest.scala
##########
@@ -136,7 +135,7 @@ abstract class BaseRequestTest extends IntegrationTestHarness {
   }
 
   def sendWithHeader(request: AbstractRequest, header: RequestHeader, socket: Socket): Unit = {
-    val serializedBytes = Utils.toArray(RequestTestUtils.serializeRequestWithHeader(header, request))
+    val serializedBytes = Utils.toArray(request.serializeWithHeader(header))
     sendRequest(socket, serializedBytes)

Review comment:
       This is a great suggestion! I even found that the whole `BaseRequestTest` is a duplicate of `IntegrationTestUtils`, how about open another PR for this refactor since this would change a lot of classes and the `IntegrationTestUtils` is under `integration` package while `BaseRequestTest` is under `unit` package.




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