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/09/01 08:00:14 UTC

[GitHub] [kafka] mimaison opened a new pull request #11288: MINOR: Fix error response generation

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


   AlterClientQuotas, DescribeProducers and FindCoordinator have issues when building error responses. This can lead to brokers returning responses without errors even when some have happened.
   
   ### 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] mimaison merged pull request #11288: KAFKA-13258/13259/13260: Fix error response generation

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


   


-- 
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] dajac commented on pull request #11288: MINOR: Fix error response generation

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


   @mimaison Is it a regression? It would be good to file a bug for it in order to have a trace about it. I was about to cut a RC for 2.8.1 but I'll wait on this one.


-- 
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 #11288: MINOR: Fix error response generation

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






-- 
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 #11288: MINOR: Fix error response generation

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


   Thanks @dajac for the feedback. I've addressed your comments.
   
   I agree with your comment about testing of these classes. `RequestResponseTest` is messy and it's hard to tell what's actually tested. I wouldn't be surprized if some requests or responses are completely missed!


-- 
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 edited a comment on pull request #11288: MINOR: Fix error response generation

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


   Yes it's partly a regression:
   - AlterClientQuotas worked correctly up to 2.7.0 and is broken since 2.8.0.
   - FindCoordinator worked correctly up to 2.8.0 and is broken since 3.0.0
   
   DescribeProducers is broken since releasing in 2.8.0.
   
   It almost feels like we should create a ticket (and PR) for each of 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.

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 #11288: MINOR: Fix error response generation

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


   Backports:
   - 2.8: https://github.com/apache/kafka/pull/11299
   - 3.0: https://github.com/apache/kafka/pull/11300


-- 
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 #11288: MINOR: Fix error response generation

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


   This affects 2.8.0 too so part of this PR will need to be backported.


-- 
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] dajac commented on pull request #11288: MINOR: Fix error response generation

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


   Thanks. I wonder if we should consider them as blockers for 3.0. KAFKA-13258 is clearly a regression introduced in 2.8 and KAFKA-13260 could be quite bad as well. What do you think?


-- 
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 #11288: MINOR: Fix error response generation

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


   I opened:
   - https://issues.apache.org/jira/browse/KAFKA-13258
   - https://issues.apache.org/jira/browse/KAFKA-13259
   - https://issues.apache.org/jira/browse/KAFKA-13260
   
   We can't easily split the PR as the test makes them all fail


-- 
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 edited a comment on pull request #11288: MINOR: Fix error response generation

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


   Yes it's partly a regression:
   - AlterClientQuotas worked correctly up to 2.7.0 and is broken since 2.8.0.
   - FindCoordinator worked correctly up to 2.8.0 and is broken since 3.0.0
   
   DescribeProducers is broken since releasing in 2.8.0.
   
   It almost feels like we should create a ticket (and PR) for each of 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.

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 change in pull request #11288: MINOR: Fix error response generation

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



##########
File path: clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
##########
@@ -706,6 +706,8 @@ private void checkDescribeConfigsResponseVersions() {
     private void checkErrorResponse(AbstractRequest req, Throwable e, boolean checkEqualityAndHashCode) {
         AbstractResponse response = req.getErrorResponse(e);
         checkResponse(response, req.version(), checkEqualityAndHashCode);
+        Map<Errors, Integer> errorCounts = response.errorCounts();
+        assertTrue(errorCounts.containsKey(Errors.forException(e)), "API Key " + req.apiKey().name + "V" + req.version() + " failed errorCounts test");

Review comment:
       @hachikuji Thanks for the review! I pushed an update




-- 
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] hachikuji commented on a change in pull request #11288: MINOR: Fix error response generation

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



##########
File path: clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
##########
@@ -706,6 +706,8 @@ private void checkDescribeConfigsResponseVersions() {
     private void checkErrorResponse(AbstractRequest req, Throwable e, boolean checkEqualityAndHashCode) {
         AbstractResponse response = req.getErrorResponse(e);
         checkResponse(response, req.version(), checkEqualityAndHashCode);
+        Map<Errors, Integer> errorCounts = response.errorCounts();
+        assertTrue(errorCounts.containsKey(Errors.forException(e)), "API Key " + req.apiKey().name + "V" + req.version() + " failed errorCounts test");

Review comment:
       Is it possible to make this assertion stronger? Would we expect any other errors in the response?




-- 
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] hachikuji commented on pull request #11288: MINOR: Fix error response generation

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


   @mimaison Ah, I missed you already opened JIRAs. Never mind my comment.


-- 
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 change in pull request #11288: MINOR: Fix error response generation

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



##########
File path: clients/src/main/java/org/apache/kafka/common/requests/DescribeProducersRequest.java
##########
@@ -80,6 +80,7 @@ public DescribeProducersResponse getErrorResponse(int throttleTimeMs, Throwable
                         .setErrorCode(error.code())

Review comment:
       should we add `.setErrorMessage(error.message())` here?




-- 
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 #11288: MINOR: Fix error response generation

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


   Yes it's partly a regression:
   - AlterClientQuotas worked correctlyup to 2.7.0 and is broken since 2.8.0.
   - FindCoordinator worked correctly up to 2.8.0 and is broken since 3.0.0
   
   DescribeProducers is broken since releasing in 2.8.0.
   
   It almost feels like we should create a ticket (and PR) for each of 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.

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 #11288: MINOR: Fix error response generation

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


   KAFKA-13260 is a regression in 3.0. However, I'm not sure if it actually has any impact. Looking at all callers of `errorCounts()`, it looks like it's mostly used to check for `NOT_CONTROLLER` error which can't happen with `FindCoordinator`. 
   
   KAFKA-13258 is a regression in 2.8. This is directly visible by users when calling `Admin.alterClientQuotas()`, so I consider this high impact. 
   
   KAFKA-13259 is here since the `DescribeProducers` API was introduced in 2.8. But `Admin.describeProducers()` is only being added in 3.0. I've not tried explicitly with a 3.0 client but I expect it behaves like KAFKA-13258, ie you don't get an Exception even when the call fails, so it's also high impact.
   
   @kkonstantine can you take a look an decide whether we want this PR in 3.0? KAFKA-13258 and KAFKA-13259 are not technically regressions in 3.0 but are pretty annoying for users.


-- 
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] hachikuji commented on a change in pull request #11288: MINOR: Fix error response generation

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



##########
File path: clients/src/test/java/org/apache/kafka/common/requests/RequestResponseTest.java
##########
@@ -706,6 +706,11 @@ private void checkDescribeConfigsResponseVersions() {
     private void checkErrorResponse(AbstractRequest req, Throwable e, boolean checkEqualityAndHashCode) {
         AbstractResponse response = req.getErrorResponse(e);
         checkResponse(response, req.version(), checkEqualityAndHashCode);
+        Errors error = Errors.forException(e);
+        Map<Errors, Integer> errorCounts = response.errorCounts();
+        assertEquals(1, errorCounts.size());

Review comment:
       nit: could probably simplify these two assertions a little:
   ```java
   assertEquals(Collections.singleton(error), errorCounts.keySet());
   ```




-- 
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 #11288: MINOR: Fix error response generation

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


   Also, this PR title should be updated to link to JIRA tickets. 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] dajac commented on pull request #11288: MINOR: Fix error response generation

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


   @mimaison Is it a regression? It would be good to file a bug for it in order to have a trace about it. I was about to cut a RC for 2.8.1 but I'll wait on this one.


-- 
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] dajac commented on a change in pull request #11288: MINOR: Fix error response generation

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



##########
File path: clients/src/main/java/org/apache/kafka/common/requests/DescribeProducersRequest.java
##########
@@ -69,7 +71,7 @@ public DescribeProducersRequestData data() {
     @Override
     public DescribeProducersResponse getErrorResponse(int throttleTimeMs, Throwable e) {
         Errors error = Errors.forException(e);
-        DescribeProducersResponseData response = new DescribeProducersResponseData();
+        List<TopicResponse> topics = new ArrayList<>();

Review comment:
       nit: We could keep instantiating `DescribeProducersResponseData` here and add the topic to it with `reponse.topics.add(..)`.

##########
File path: clients/src/main/java/org/apache/kafka/common/requests/AlterClientQuotasRequest.java
##########
@@ -125,7 +127,10 @@ public AlterClientQuotasResponse getErrorResponse(int throttleTimeMs, Throwable
                     .setEntityType(entityData.entityType())
                     .setEntityName(entityData.entityName()));
             }
-            responseEntries.add(new AlterClientQuotasResponseData.EntryData().setEntity(responseEntities));
+            responseEntries.add(new AlterClientQuotasResponseData.EntryData()
+                    .setEntity(responseEntities)
+                    .setErrorCode(error.code())
+                    .setErrorMessage(error.message()));

Review comment:
       nit: Should we use 4 spaces to indent those ones here? We use 4 spaces for similar code at L127/128 so the code would remain a bit more homogeneous. 




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