-
Notifications
You must be signed in to change notification settings - Fork 706
[CORE-13043] cluster_link: enable changing bootstrap servers with update shadow link command #27986
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
[CORE-13043] cluster_link: enable changing bootstrap servers with update shadow link command #27986
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enables updating bootstrap servers and TLS configuration for cluster links through the shadow link update command. Previously, these configuration changes were disallowed but are now supported with proper broker connection management.
- Removes restrictions on updating bootstrap_servers and tls_settings in shadow links
- Implements broker configuration updates that properly restart client connections when brokers or TLS settings change
- Adds comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
tests/rptest/tests/cluster_linking_topic_syncing_test.py |
Refactors TLS provider class to shared location |
tests/rptest/tests/cluster_linking_test_base.py |
Moves ClusterLinkingTLSProvider here as shared utility |
tests/rptest/tests/cluster_linking_e2e_test.py |
Adds new test class ShadowLinkUpdateBrokersTests with test for broker updates |
src/v/redpanda/admin/services/shadow_link/shadow_link.cc |
Removes validation that blocked bootstrap_servers and tls_settings updates |
src/v/net/tls.h |
Adds equality operators for TLS-related structs |
src/v/kafka/client/configuration.h |
Adds equality operator for tls_configuration |
src/v/kafka/client/cluster.h |
Adds update_broker_config method and configuration getter |
src/v/kafka/client/cluster.cc |
Implements broker configuration update with proper connection management |
src/v/kafka/client/brokers.h |
Adds clear method declaration |
src/v/kafka/client/brokers.cc |
Implements clear method to remove all brokers |
src/v/kafka/client/BUILD |
Adds dependency on unresolved_address utility |
src/v/cluster_link/utils.h |
Adds declarations for TLS and SASL configuration creation functions |
src/v/cluster_link/utils.cc |
Refactors configuration creation functions to return optionals |
src/v/cluster_link/manager.cc |
Changes update_config call to be async |
src/v/cluster_link/link.h |
Makes update_config async and adds broker configuration update method |
src/v/cluster_link/link.cc |
Implements async configuration updates with broker connection management |
michael-redpanda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
c20d5a8 to
cf01115
Compare
|
changes in force-push:
|
Retry command for Build#73938please wait until all jobs are finished before running the slash command |
michael-redpanda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - want to see if @bharathv is happy with your response and then we can merge
cf01115 to
0e562c6
Compare
|
Changes in force-push:
|
| && broker_tls == client_config.broker_tls) { | ||
| return ss::now(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is worthy of an info log IMO.
Retry command for Build#73956please wait until all jobs are finished before running the slash command |
|
Can we merge this after: #27703 ? |
82006ea
0e562c6 to
82006ea
Compare
|
Force push:
|
|
The type checking issue was fixed here: #28134 |
michael-redpanda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good - would be interested in having @mmaslankaprv take a quick peak since he has done so much work in kafka/client/cluster
Retry command for Build#74614please wait until all jobs are finished before running the slash command |
9a50f05 to
5946d44
Compare
|
changes in force-push:
|
These tests don't call start, so they didn't need to call stop, till now. In the next commit, an ssx::work_queue will be added to the kafka client cluster, which makes the call to stop mandatory, even if it hasn't been started.
…luster This commit also add all the functionality needed to restart brokers. In the update config, we restart the brokers to pick up the config.
Because of the difficulty involved in changing the address of a cluster in dt, this test is a workaround. The second source cluster is meant to represent a change of broker address.
5946d44 to
343caba
Compare
|
changes in force-push:
|
Retry command for Build#74750please wait until all jobs are finished before running the slash command |
Implements: CORE_13043
Add path to handle changes in bootstrap_servers and/or tls config of a cluster link.
When either one of these change as part of a shadow link update request, the brokers in the
kafka::client::clusterneed to be brought down and be remade using the new seeds/tls config.Backports Required
Release Notes