Skip to content

MINOR: fix Streams version-probing system test#6764

Merged
mjsax merged 1 commit into
apache:trunkfrom
mjsax:minor-fix-streams-version-probing-test-trunk
May 24, 2019
Merged

MINOR: fix Streams version-probing system test#6764
mjsax merged 1 commit into
apache:trunkfrom
mjsax:minor-fix-streams-version-probing-test-trunk

Conversation

@mjsax

@mjsax mjsax commented May 19, 2019

Copy link
Copy Markdown
Member

With KAFKA-8284, the AbstractCoordinator does not allow to suppress sending LeaveGroupRequest on Consumer#close() any longer. Hence, we get a different rebalance pattern.

This implies, that the last rebalance, does not do any version probing any longer. When the last instance is bounced, an immediate rebalance "stabilized the group" and prepares it to upgrade to the new metadata version, because all members are on old version but support new version already. On restart of the last instance, we immediately switch to the new version instead of doing a version probing.

@mjsax mjsax added streams tests Test fixes (including flaky tests) labels May 19, 2019
@mjsax

mjsax commented May 19, 2019

Copy link
Copy Markdown
Member Author

Retest this please

@mjsax

mjsax commented May 19, 2019

Copy link
Copy Markdown
Member Author

@mjsax

mjsax commented May 19, 2019

Copy link
Copy Markdown
Member Author

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Just Java8 cleanup.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is one fix. If rebalance happens quickly, we might see this message for the second rebalance early. Cf. #6763

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

After #6779 is merged, I can rebase this PR -- we still want this fix I guess :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is the actual fix (similar below).

@mjsax

mjsax commented May 19, 2019

Copy link
Copy Markdown
Member Author

@vvcephei vvcephei 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.

Ah. Thanks for figuring this out!

@guozhangwang

Copy link
Copy Markdown
Contributor

I think we should consider whether streams/consumer should still maintain the leaveOnshutdown flag or not, and depending on that this PR should be refactored.

@guozhangwang

Copy link
Copy Markdown
Contributor

cc @abbccdda

@guozhangwang

Copy link
Copy Markdown
Contributor

@mjsax could you rebase this one given the other PR has been merged now?

@abbccdda abbccdda left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM with minor comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: do we want to define 3 instead of leaving it as magic number?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

With the latest fixes, I think we actually don't need this any longer. We only needed if, if we get immediate rebalance. With disabling leave group request, this this should not be required any longer.

@mjsax
mjsax force-pushed the minor-fix-streams-version-probing-test-trunk branch from 915a20a to 0080b24 Compare May 23, 2019 22:53
@mjsax

mjsax commented May 23, 2019

Copy link
Copy Markdown
Member Author

Rebased and updated this PR. Version probing system test passed locally. Also triggered https://jenkins.confluent.io/job/system-test-kafka-branch-builder/2629/

If no objection, I'll merge this after Jenkins and system test build passed.

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

@mjsax feel free to merge after green builds.

@mjsax

mjsax commented May 24, 2019

Copy link
Copy Markdown
Member Author

System test passed. Jenkins failed with env issue.

Retest this please.

@ijuma

ijuma commented May 24, 2019

Copy link
Copy Markdown
Member

@mjsax seems ready to merge.

@mjsax
mjsax merged commit d286051 into apache:trunk May 24, 2019
mjsax added a commit that referenced this pull request May 24, 2019
Reviewers: John Roesler <john@confluent.io>, Bill Bejeck <bill@confluent.io>, Guozhang Wang <guozhang@confluent.io>, Boyang Chen <boyang@confluent.io>
@mjsax

mjsax commented May 24, 2019

Copy link
Copy Markdown
Member Author

Merged to trunk and cherry-picked to 2.3.

@mjsax
mjsax deleted the minor-fix-streams-version-probing-test-trunk branch May 24, 2019 15:10
haidangdam pushed a commit to haidangdam/kafka that referenced this pull request May 29, 2019
Reviewers: John Roesler <john@confluent.io>, Bill Bejeck <bill@confluent.io>, Guozhang Wang <guozhang@confluent.io>, Boyang Chen <boyang@confluent.io>
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
Reviewers: John Roesler <john@confluent.io>, Bill Bejeck <bill@confluent.io>, Guozhang Wang <guozhang@confluent.io>, Boyang Chen <boyang@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

streams tests Test fixes (including flaky tests)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants