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 2020/12/13 09:51:53 UTC

[GitHub] [kafka] dengziming opened a new pull request #9741: MINOR; Remove redudant parseResponse implimention

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


   *More detailed description of your change*
   #7409 has removed the conversion to Struct when parsing resp, `ApiVersionsResponse.parse` will be used instead of `API_VERSIONS.parseResponse`, so the overwrite in `ApiKeys.API_VERSIONS` is useless. 
   
   *Summary of testing strategy (including rationale)
   for the feature or bug fix. Unit and/or integration
   tests are expected for any behaviour change and
   system tests should be considered for larger changes.*
   
   ### 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] dengziming commented on pull request #9741: KAFKA-10849: Remove useless ApiKeys#parseResponse and ApiKeys#parseRequest

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


   @chia7712 I removed `responseSchema ` which was used in `ApiKeysTest.testResponseThrottleTime` to inspect all response,  I think it's similar to tests in `ApiVersionsResponseTest`, so I just also moved 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

[GitHub] [kafka] chia7712 commented on pull request #9741: KAFKA-10849: Remove useless ApiKeys#parseResponse and ApiKeys#parseRequest

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


   @dengziming I just notice there is a PR (#9748) covering this one. Could you please close this PR and then be a reviewer for that 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] dengziming commented on pull request #9741: MINOR; Remove redudant parseResponse implementation

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


   Hi, @chia7712 , PTAL.


----------------------------------------------------------------
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 #9741: KAFKA-10849: Remove useless ApiKeys#parseResponse and ApiKeys#parseRequest

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


   @chia7712 Thank you, I also created an issue and changed the title.


----------------------------------------------------------------
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 #9741: KAFKA-10849: Remove useless ApiKeys#parseResponse and ApiKeys#parseRequest

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


   @chia7712 Oh, sorry for the repeated work, I just closed 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 pull request #9741: KAFKA-10849: Remove useless ApiKeys#parseResponse and ApiKeys#parseRequest

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


   @dengziming Thanks for your updating. ```requestSchema``` and ```responseSchema``` are not used by production code also. Could you remove them? 


----------------------------------------------------------------
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 #9741: KAFKA-10849: Remove useless ApiKeys#parseResponse and ApiKeys#parseRequest

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



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -178,15 +175,7 @@
             DescribeGroupsResponseData.SCHEMAS),
     LIST_GROUPS(16, "ListGroups", ListGroupsRequestData.SCHEMAS, ListGroupsResponseData.SCHEMAS),
     SASL_HANDSHAKE(17, "SaslHandshake", SaslHandshakeRequestData.SCHEMAS, SaslHandshakeResponseData.SCHEMAS),
-    API_VERSIONS(18, "ApiVersions", ApiVersionsRequestData.SCHEMAS, ApiVersionsResponseData.SCHEMAS) {
-        @Override
-        public Struct parseResponse(short version, ByteBuffer buffer) {
-            // Fallback to version 0 for ApiVersions response. If a client sends an ApiVersionsRequest
-            // using a version higher than that supported by the broker, a version 0 response is sent
-            // to the client indicating UNSUPPORTED_VERSION.
-            return parseResponse(version, buffer, (short) 0);

Review comment:
       @dajac Nice question. It is related to #7409 and the replacement is https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/requests/ApiVersionsResponse.java#L80
   




----------------------------------------------------------------
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 closed pull request #9741: KAFKA-10849: Remove useless ApiKeys#parseResponse and ApiKeys#parseRequest

Posted by GitBox <gi...@apache.org>.
dengziming closed pull request #9741:
URL: https://github.com/apache/kafka/pull/9741


   


----------------------------------------------------------------
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 #9741: KAFKA-10849: Remove useless ApiKeys#parseResponse and ApiKeys#parseRequest

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



##########
File path: clients/src/main/java/org/apache/kafka/common/protocol/ApiKeys.java
##########
@@ -178,15 +175,7 @@
             DescribeGroupsResponseData.SCHEMAS),
     LIST_GROUPS(16, "ListGroups", ListGroupsRequestData.SCHEMAS, ListGroupsResponseData.SCHEMAS),
     SASL_HANDSHAKE(17, "SaslHandshake", SaslHandshakeRequestData.SCHEMAS, SaslHandshakeResponseData.SCHEMAS),
-    API_VERSIONS(18, "ApiVersions", ApiVersionsRequestData.SCHEMAS, ApiVersionsResponseData.SCHEMAS) {
-        @Override
-        public Struct parseResponse(short version, ByteBuffer buffer) {
-            // Fallback to version 0 for ApiVersions response. If a client sends an ApiVersionsRequest
-            // using a version higher than that supported by the broker, a version 0 response is sent
-            // to the client indicating UNSUPPORTED_VERSION.
-            return parseResponse(version, buffer, (short) 0);

Review comment:
       @dengziming Do we maintain this fallback mechanism somewhere?




----------------------------------------------------------------
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 edited a comment on pull request #9741: KAFKA-10849: Remove useless ApiKeys#parseResponse and ApiKeys#parseRequest

Posted by GitBox <gi...@apache.org>.
dengziming edited a comment on pull request #9741:
URL: https://github.com/apache/kafka/pull/9741#issuecomment-744267489


   @chia7712 I removed `responseSchema` which was used in `ApiKeysTest.testResponseThrottleTime` to inspect all response,  I think it's similar to tests in `ApiVersionsResponseTest`, so I just also moved 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