KAFKA-8649: send latest commonly supported version in assignment by ableegoldman · Pull Request #7423 · apache/kafka

ableegoldman · 2019-09-30T20:15:07Z

...instead of sending the leader's version and having older members try to blindly upgrade.

The only other real change here is that we will also set the VERSION_PROBING error code and return early from onAssignment when we are upgrading our used subscription version (not just downgrading it) since this implies the whole group has finished the rolling upgrade and all members should rejoin with the new subscription version.

Also piggy-backing on a fix for a potentially dangerous edge case, where every thread of an instance is assigned the same set of active tasks.

Should be cherry-picked back to 2.1
2.4/trunk: this PR
2.3: PR #7425
2.2: PR #7426
2.1: PR #7427

ableegoldman · 2019-09-30T20:17:57Z

Not sure if we need any additional changes to StreamsUpgradeTest but I kicked off the version probing system test: https://jenkins.confluent.io/job/system-test-kafka-branch-builder/3055/

At some point we definitely do want to make some adjustments to StreamsUpgradeTest so it can catch bugs like this, but that doesn't need to block the bugfix.

guozhangwang

This PR looks great to me, and I like the cleanup a lot! Just some minor comments.

guozhangwang · 2019-09-30T20:51:50Z

Now it seems all constructors would trigger the version check anyways, so shall we just move this in to the last constructor (with 6 params)?

guozhangwang · 2019-09-30T20:55:13Z

I like this cleanup.

guozhangwang · 2019-09-30T21:04:26Z

nit: maybe rename the field in AssignmentInfo as minSupportedVersion -- or commonlySupportedVersion is also fine -- and also rename the access function names as well as the local fields here? I felt latestSupportedVersion is becoming confusing now.

Yeah I kept flip-flopping on what I thought a good name would be -- happy to just go with commonlySupportedVersion

guozhangwang · 2019-09-30T21:13:57Z

If receivedAssignmentMetadataVersion < EARLIEST_PROBEABLE_VERSION, it is also a pretty bad situation since it means that the leader is too old to even be covered with version probing --- in fact, if it is that old, upon getting the new version it would throw exception and die immediately, so we'd not even see the assignment back at all --- so we'd better also log it as an ERROR to complete the logic.

True, we should actually check for this (in validateMetadataVersions) and throw an exception -- it basically means someone tried to do a rolling bounce from < 2.0 without setting the UPGRADE_FROM config

Actually, this would not be an error but possibly just an upgrade from a very old version -- see #7436

guozhangwang · 2019-09-30T21:15:01Z

I'd suggest adding some javadoc here explaining our logic:

latestCommonlySupportedVersion belongs to [usedSubscriptionMetadataVersion, LATEST_SUPPORTED_VERSION] receivedAssignmentMetadataVersion belongs to [EARLIEST_PROBEABLE_VERSION, usedSubscriptionMetadataVersion] usedSubscriptionMetadataVersion would be upgraded to latestCommonlySupportedVersion when necessary.

guozhangwang · 2019-09-30T21:20:51Z

We can collapse these two into a single private function.

guozhangwang · 2019-09-30T21:22:20Z

:streams:checkstyleTest failed.

…e client's prevTasks to each consumer

fix system test (hopefully...)

ableegoldman · 2019-10-01T00:51:27Z

This error message didn't make sense, it should be referring to upgrading the metadata to the newer version

set leader counter

guozhangwang · 2019-10-02T15:51:17Z

                    monitors[first_other_processor] = first_other_monitor
                    monitors[second_other_processor] = second_other_monitor

-                    leader_monitor.wait_until("Received a future (version probing) subscription (version: 6). Sending empty assignment back (with supported version 5).",


I'm +1 on removing this logic, we could not assume that the leader did not change even if it is not the one who get's bounced, since it may join the group late.

guozhangwang · 2019-10-02T15:53:22Z

Failed JDK 11 and Scala 2.12, JDK 8 and Scala 2.11 on different tests (one for each run), no overlapping on the fail test.

guozhangwang · 2019-10-02T15:53:31Z

The PR LGTM, merging to trunk.

Small follow-up to trunk PR #7423 While debugging the 2.3 VP PR we realized we should remove the leader-tracking from the VP system test altogether. We'd already merged the corresponding trunk PR so I made a quick new PR for trunk (also fixes a missed version bump in one of the log messages) Reviewers: Guozhang Wang <wangguoz@gmail.com>

ableegoldman mentioned this pull request Sep 30, 2019

KAFKA-8649: version probing with upgraded leader #7413

Closed

guozhangwang reviewed Sep 30, 2019

View reviewed changes

use commonly supported version not leader version and dont send entir…

1fa7003

…e client's prevTasks to each consumer

ableegoldman force-pushed the VP-lie branch from b850496 to 1fa7003 Compare September 30, 2019 21:31

address github comments

1afc31f

fix system test (hopefully...)

ableegoldman force-pushed the VP-lie branch from 6150c5d to 1afc31f Compare September 30, 2019 23:56

ableegoldman added 2 commits September 30, 2019 17:13

fix expected log message

84a0999

fix stupid system test parsing

0709273

ableegoldman commented Oct 1, 2019

View reviewed changes

This was referenced Oct 1, 2019

KAFKA-8649: send latest commonly supported version in assignment #7425

Merged

KAFKA-8649: send latest commonly supported version in assignment #7426

Merged

KAFKA-8649: send latest commonly supported version in assignment #7427

Merged

ableegoldman force-pushed the VP-lie branch from eaa9d20 to ecd071f Compare October 1, 2019 02:40

pause partitions

cc65789

set leader counter

ableegoldman force-pushed the VP-lie branch 2 times, most recently from 72f10aa to 6b7ea3c Compare October 1, 2019 04:09

allow for leader to change in upgrade test

0f0e78e

ableegoldman force-pushed the VP-lie branch from f01915c to 0f0e78e Compare October 1, 2019 04:27

ableegoldman added 2 commits September 30, 2019 23:08

apparently forgot how to write Python

39079a7

just ignore multiple leaders

40c9250

ableegoldman mentioned this pull request Oct 1, 2019

KAFKA-8179: Part 7, cooperative rebalancing in Streams #7386

Merged

remove flaky line

55080af

guozhangwang reviewed Oct 2, 2019

View reviewed changes

guozhangwang merged commit 8da6993 into apache:trunk Oct 2, 2019

ableegoldman mentioned this pull request Oct 4, 2019

MINOR: followup to Version Probing improvements #7447

Merged

ableegoldman deleted the VP-lie branch June 26, 2020 22:38

Conversation

ableegoldman commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ableegoldman commented Sep 30, 2019

Uh oh!

guozhangwang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Sep 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Oct 2, 2019

Uh oh!

guozhangwang commented Oct 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ableegoldman commented Sep 30, 2019 •

edited

Loading