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

Make KafkaSinkCluster rack aware #1527

Merged
merged 1 commit into from
Mar 14, 2024
Merged

Conversation

rukai
Copy link
Member

@rukai rukai commented Mar 14, 2024

Progress towards #1526

This PR implements "Extended shotover nodes config" and "Message rewriting" from #1526
But it leaves "Rack aware routing" for follow up work.

The old message rewriting implementation was quite hacky, it attempted to assign various kafka roles (coordinators, controllers etc) to different shotover nodes but with only a poor understanding of what the shotover cluster looks like (no broker ids)
However I believe this PR gives us a much more robust implementation.
This is because we now have an exact model of our shotover cluster and can return that in our metadata.

cluster_2_racks_single_shotover is deleted since the case no longer makes sense. When kafka is configured with racks we need one shotover node per rack however this test case has 2 racks but only one shotover node.

I'm hopeful this PR will also fix the java driver cluster issues, but I wont get into that rabbit hole until this PR lands.

@rukai rukai force-pushed the kafka_rack_aware branch 5 times, most recently from 45cb3cc to c172e24 Compare March 14, 2024 05:51
@rukai rukai force-pushed the kafka_rack_aware branch from c172e24 to efab1ba Compare March 14, 2024 05:53
@rukai rukai marked this pull request as ready for review March 14, 2024 06:05
@rukai rukai requested a review from conorbros March 14, 2024 06:17
@conorbros conorbros merged commit 5523020 into shotover:main Mar 14, 2024
40 checks passed
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