Skip to content

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

Merged
hachikuji merged 2 commits into
apache:trunkfrom
abbccdda:fix_leave
Jul 22, 2019
Merged

KAFKA-8678: fix leave group protocol bug in throttling and error response#7101
hachikuji merged 2 commits into
apache:trunkfrom
abbccdda:fix_leave

Conversation

@abbccdda

@abbccdda abbccdda commented Jul 18, 2019

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Author

retest this please

@guozhangwang guozhangwang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return version >= 2;
}

@Override

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be useful for unit test purpose.

@hachikuji hachikuji left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good find. Left a couple comments.

Comment thread clients/src/test/java/org/apache/kafka/common/requests/LeaveGroupRequestTest.java Outdated

@hachikuji hachikuji left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just one additional comment.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@abbccdda

Copy link
Copy Markdown
Author

retest this please

1 similar comment
@abbccdda

Copy link
Copy Markdown
Author

retest this please

@abbccdda

Copy link
Copy Markdown
Author

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
hachikuji pushed a commit that referenced this pull request Jul 22, 2019
…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 pushed a commit that referenced this pull request Jul 22, 2019
…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
* 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 pushed a commit to linkedin/kafka that referenced this pull request Aug 22, 2019
…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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants