Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-8678: fix leave group protocol bug in throttling and error response #7101

Merged
merged 2 commits into from Jul 22, 2019

Conversation

@abbccdda
Copy link
Contributor

commented Jul 18, 2019

This is a bug fix PR to resolve errors introduced in #6188.
The PR fixes 2 things:

  1. throttle time should be set on version >= 1 instead of version >= 2
  2. getErrorResponse should set throwable exception within LeaveGroupResponseData

In the meanwhile, adding more unit tests to guarantee correctness for leave group protocol.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@abbccdda abbccdda force-pushed the abbccdda:fix_leave branch Jul 18, 2019

@abbccdda abbccdda force-pushed the abbccdda:fix_leave branch to 53678da Jul 18, 2019

@abbccdda

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

retest this please

@guozhangwang
Copy link
Contributor

left a comment

Thanks for the added test coverage, lgtm except one question.

@@ -72,4 +73,15 @@ public static LeaveGroupResponse parse(ByteBuffer buffer, short versionId) {
public boolean shouldClientThrottle(short version) {
return version >= 2;
}

@Override

This comment has been minimized.

Copy link
@guozhangwang

guozhangwang Jul 18, 2019

Contributor

Is it necessary to add equals and hashCode in this fix?

This comment has been minimized.

Copy link
@abbccdda

abbccdda Jul 18, 2019

Author Contributor

It should be useful for unit test purpose.

@hachikuji
Copy link
Contributor

left a comment

Thanks, good find. Left a couple comments.

@hachikuji
Copy link
Contributor

left a comment

LGTM. Just one additional comment.

clients/src/test/java/org/apache/kafka/common/requests/LeaveGroupResponseTest.java Outdated
LeaveGroupResponseData responseData = new LeaveGroupResponseData()
.setErrorCode(Errors.NONE.code())
.setThrottleTimeMs(throttleTimeMs);
for (short version = 0; version < LeaveGroupResponseData.SCHEMAS.length; version++) {

This comment has been minimized.

Copy link
@hachikuji

hachikuji Jul 18, 2019

Contributor

nit: use ApiKeys.LEAVE_GROUP.latestVersion() here also

@abbccdda abbccdda force-pushed the abbccdda:fix_leave branch to 059348a Jul 19, 2019

@abbccdda

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

retest this please

1 similar comment
@abbccdda

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

retest this please

@abbccdda

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

We already got 4 1/3 builds and manually checked all tests failures are flaky. Will kick off one more to aim for 2/3

retest this please

@hachikuji hachikuji merged commit f98e176 into apache:trunk Jul 22, 2019

2 of 3 checks passed

JDK 11 and Scala 2.12 FAILURE 11691 tests run, 67 skipped, 2 failed.
Details
JDK 11 and Scala 2.13 SUCCESS 11691 tests run, 67 skipped, 0 failed.
Details
JDK 8 and Scala 2.11 SUCCESS 11691 tests run, 67 skipped, 0 failed.
Details
hachikuji added a commit that referenced this pull request Jul 22, 2019
KAFKA-8678; Fix leave group protocol bug in throttling and error resp…
…onse (#7101)

This is a bug fix PR to resolve errors introduced in #6188. The PR fixes 2 things:

1. throttle time should be set on version >= 1 instead of version >= 2
2. `getErrorResponse` should set throwable exception within LeaveGroupResponseData

The patch also adds more unit tests to guarantee correctness for leave group protocol.

Reviewers: Guozhang Wang <wangguoz@gmail.com>, Jason Gustafson <jason@confluent.io>
hachikuji added a commit that referenced this pull request Jul 22, 2019
KAFKA-8678; Fix leave group protocol bug in throttling and error resp…
…onse (#7101)

This is a bug fix PR to resolve errors introduced in #6188. The PR fixes 2 things:

1. throttle time should be set on version >= 1 instead of version >= 2
2. `getErrorResponse` should set throwable exception within LeaveGroupResponseData

The patch also adds more unit tests to guarantee correctness for leave group protocol.

Reviewers: Guozhang Wang <wangguoz@gmail.com>, Jason Gustafson <jason@confluent.io>
ijuma added a commit to confluentinc/kafka that referenced this pull request Aug 4, 2019
Merge remote-tracking branch 'apache-github/2.3' into ccs-2.3
* apache-github/2.3:
  MINOR: Avoid dividing by zero (apache#7143)
  KAFKA-8731: InMemorySessionStore throws NullPointerException on startup (apache#7132)
  KAFKA-8715; Fix buggy reliance on state timestamp in static member.id generation (apache#7116)
  KAFKA-8678; Fix leave group protocol bug in throttling and error response (apache#7101)
xiowu0 added a commit to linkedin/kafka that referenced this pull request Aug 22, 2019
[LI-CHERRY-PICK] [f94ec1c] KAFKA-8678; Fix leave group protocol bug i…
…n throttling and error response (apache#7101)

TICKET = KAFKA-8678
LI_DESCRIPTION =
EXIT_CRITERIA = HASH [f94ec1c]
ORIGINAL_DESCRIPTION =

This is a bug fix PR to resolve errors introduced in apache#6188. The PR fixes 2 things:

1. throttle time should be set on version >= 1 instead of version >= 2
2. `getErrorResponse` should set throwable exception within LeaveGroupResponseData

The patch also adds more unit tests to guarantee correctness for leave group protocol.

Reviewers: Guozhang Wang <wangguoz@gmail.com>, Jason Gustafson <jason@confluent.io>
(cherry picked from commit f94ec1c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.