KAFKA-12495: Improve rebalance allocation algorithm by C0urante · Pull Request #1 · vamossagar12/kafka

C0urante · 2022-10-23T23:26:32Z

Follow-up as discussed on the open PR for KAFKA-12495, which ports some changes from apache#12019 onto that PR branch.

These changes should fix some issues with load-balancing revocations and assignment of new and lost connectors and tasks. The primary differences are when load-balancing revocations take place (a number of currently-missed cases are added), when lost assignments are reassigned, and how load-balancing revocations are calculated.

One result is that a number of unnecessary follow-up rebalances are skipped since a balanced allocation can be achieved sooner, and potential regressions introduced in the upstream PR for KAFKA-12495 are avoided as a result.

Another result is that resizing connectors should no longer lead to unbalanced allocations across the cluster during rebalance.

Committer Checklist (excluded from commit message)

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

C0urante · 2022-10-23T23:30:39Z

This is ready for review now, but I've marked it as a draft to signal that it should not be merged into the upstream branch yet. I'm hoping we can review this PR separately until it's approved to be merged into the trunk branch of the upstream repo, then merge it and the upstream KAFKA-12495 branch together into trunk all at once.

vamossagar12

@C0urante , Thanks for the follow up PR! I have a few comments mainly for my understanding and some naming related stuffs which I felt could change

C0urante

Thanks @vamossagar12, this is ready for another round when you have time. I've also removed some now-unused methods, as a minor cleanup step.

vamossagar12 · 2022-10-27T12:10:45Z

Thanks @vamossagar12, this is ready for another round when you have time. I've also removed some now-unused methods, as a minor cleanup step.

Thanks @C0urante . I made another pass.

C0urante · 2022-11-01T00:19:55Z

Thanks @vamossagar12. Ready for the next round when you have time.

vamossagar12 · 2022-11-02T13:31:30Z

@C0urante , I looked at it again. The changes look good to me and can't think of new comments at this point. Thanks for the changes !

C0urante · 2022-11-02T15:50:13Z

Thanks @vamossagar12!

@showuon if you can spare a few cycles, mind taking a look? Once this is ready to go we should be able to finally merge a fix for KAFKA-12495!

showuon · 2022-11-11T06:52:33Z

Sorry, quite busy these days. I'll try to review it next week.

showuon

@C0urante , thanks for the improvement. Overall LGTM! Just some minor comments.

showuon · 2022-11-14T03:22:05Z

I think we can add a comment above this for loop to mention we're going to calculate how many connectors/tasks to revoke from existing worker

showuon · 2022-11-14T03:36:36Z

Why changed to 0 delay now?

Due to optimizations in the allocation algorithm, we no longer perform a task-balancing revocation in the fourth round, which allows us to reset the delay.

showuon · 2022-11-14T03:38:54Z

Nice improvement

showuon · 2022-11-14T03:43:48Z

showuon · 2022-11-14T03:44:31Z

Just want to confirm if we don't have this patch, this test will fail in apache#12561 , right?

That's correct 👍

showuon · 2022-11-14T03:46:20Z

I don't have preference here, but we changed the variable name from newSubmissions to created, should we also update the comment?

👍 Good call, done

showuon · 2022-11-14T03:54:01Z

I don't understand this comment:
Any workers that left the group within the scheduled rebalance delay permanently left the group.
The word permanently seems to be confusing to me.

TBH, the javadoc can be more clear if we remove all these 3 bullets, because I felt more confused after reading these 3 bullets. So, maybe change to:
Revoke connectors and tasks from each worker in the cluster until no worker is running more than it would be

WDYT?

Fine with simplifying this 👍
I updated it to Revoke connectors and tasks from each worker in the cluster until no worker is running more than it would be with a perfectly-balanced assignment.; LMKWYT

C0urante · 2022-11-14T15:22:33Z

Thanks @showuon! Pushed a new commit; ready when you have time.

showuon

LGTM! Great improvement!

vamossagar12 · 2022-11-15T06:03:10Z

Thanks for your time @showuon ! @C0urante , how do we want to take this further? Merge this into my branch and merge that or you want to create another PR against trunk and merge the 2 of them together? I guess it would be better to do the latter to highlight the great improvements you have made here (former might be simpler I reckon)

showuon · 2022-11-15T06:22:52Z

Merge it into this branch, and check the jenkins build status

vamossagar12 · 2022-11-15T15:51:02Z

@showuon , Chris had a different idea I think =>

This is ready for review now, but I've marked it as a draft to signal that it should not be merged into the upstream branch yet. I'm hoping we can review this PR separately until it's approved to be merged into the trunk branch of the upstream repo, then merge it and the upstream KAFKA-12495 branch together into trunk all at once.

Would wait for his response.

C0urante · 2022-11-15T16:01:02Z

@vamossagar12 it's fine to merge this one now :)

vamossagar12

LGTM

vamossagar12 · 2022-11-15T16:12:38Z

@C0urante done! Thanks for your contribution on this one!

This change introduces some basic clean up and refactoring for forthcoming commits related to the revised fetch code for the consumer threading refactor project. Reviewers: Christo Lolov <lolovc@amazon.com>, Jun Rao <junrao@gmail.com>

C0urante marked this pull request as draft October 23, 2022 23:28

C0urante mentioned this pull request Oct 23, 2022

KAFKA-12495: Exponential backoff retry to prevent rebalance storms when worker joins after revoking rebalance apache/kafka#12561

Merged

vamossagar12 reviewed Oct 25, 2022

View reviewed changes

Comment thread ...c/main/java/org/apache/kafka/connect/runtime/distributed/IncrementalCooperativeAssignor.java Outdated

C0urante commented Oct 26, 2022

View reviewed changes

showuon reviewed Nov 14, 2022

View reviewed changes

showuon approved these changes Nov 15, 2022

View reviewed changes

C0urante marked this pull request as ready for review November 15, 2022 16:00

vamossagar12 force-pushed the KAFKA-12495 branch from c7bf4d8 to 71297c3 Compare November 15, 2022 16:05

vamossagar12 approved these changes Nov 15, 2022

View reviewed changes

C0urante added 3 commits November 15, 2022 11:09

KAFKA-12495: Improve rebalance allocation algorithm

0261a23

Address review comments, remove unused code

fb34e5c

Address review comments

3eb5e13

C0urante force-pushed the kafka-12495-followup branch from b4fb6fc to 3eb5e13 Compare November 15, 2022 16:09

vamossagar12 merged commit 422c81f into vamossagar12:KAFKA-12495 Nov 15, 2022

C0urante deleted the kafka-12495-followup branch November 15, 2022 16:11

C0urante restored the kafka-12495-followup branch November 15, 2022 17:13

C0urante deleted the kafka-12495-followup branch November 15, 2022 17:31

Conversation

C0urante commented Oct 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

C0urante commented Oct 23, 2022

Uh oh!

vamossagar12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

C0urante left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vamossagar12 commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

C0urante commented Nov 1, 2022

Uh oh!

vamossagar12 commented Nov 2, 2022

Uh oh!

C0urante commented Nov 2, 2022

Uh oh!

showuon commented Nov 11, 2022

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

C0urante commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

showuon left a comment

Choose a reason for hiding this comment

C0urante commented Oct 23, 2022 •

edited

Loading

vamossagar12 commented Oct 27, 2022 •

edited

Loading

C0urante commented Nov 14, 2022 •

edited

Loading

vamossagar12 commented Nov 15, 2022 •

edited

Loading