Skip to content

[KIP-1102] Enable clients to rebootstrap based on timeout or error code #4981

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 50 commits into
base: master
Choose a base branch
from
Draft

[KIP-1102] Enable clients to rebootstrap based on timeout or error code #4981

wants to merge 50 commits into from

Conversation

emasab
Copy link
Contributor

@emasab emasab commented Feb 28, 2025

No description provided.

mfleming and others added 21 commits November 2, 2023 11:31
@mfleming
Fix for brokers with different Ids but same host:port
8dc8665 
The Kafka protocol allows for brokers to have multiple host:port pairs
for a given node Id, e.g. see UpdateMetadata request which contains a
live_brokers list where each broker Id has a list of host:port pairs. It
follows from this that the thing that uniquely identifies a broker is
its Id, and not the host:port.

The behaviour right now is that if we have multiple brokers with the
same host:port but different Ids, the first broker in the list will be
updated to have the Id of whatever broker we're looking at as we iterate
through the brokers in the Metadata response in
rd_kafka_parse_Metadata0(), e.g.

 Step 1. Broker[0] = Metadata.brokers[0]
 Step 2. Broker[0] = Metadata.brokers[1]
 Step 3. Broker[0] = Metadata.brokers[2]

A typical situation where brokers have the same host:port pair but
differ in their Id is if the brokers are behind a load balancer.

The NODE_UPDATE mechanism responsible for this was originally added in
b09ff60 ("Handle broker name and nodeid updates (issue #343)") as a way
to forcibly update a broker hostname if an Id is reused with a new host
after the original one was decommissioned. But this isn't how the Java
Kafka client works, so let's use the Metadata response as the source of
truth instead of updating brokers if we can only match by their
host:port.
@mfleming
Fix locking
cf114ef
@mfleming
Fix for purging brokers no longer reported in metadata
9754ee7 
Brokers that are not in the metadata should be purged from the internal
client lists. This helps to avoid annoying "No route to host" and other
connection failure messages.

Fixes #238.
@mfleming @emasab
Update tests/0145-broker-same-host-port.c
ba5a3c4 
Co-authored-by: Emanuele Sabellico <[email protected]>
@mfleming @emasab
Update tests/0146-purge-brokers.c
a735614 
Co-authored-by: Emanuele Sabellico <[email protected]>
@emasab
Remove the possibility to modify rkb_nodeid after
05e8950 
rkb creation.
@emasab
Remove locking when accessing rkb_nodeid
df12c6a 
as it's now set only on creation
and not modified anymore
@emasab
Add new brokers and reassign partitions in the
4365c9e 
mock cluster
@emasab
CHANGELOG
6253618
@emasab
Automatic style fix
d16b8ce
@emasab
Rename to rd_kafka_brokers_learned_ids
959bf16 
and move documentation
@emasab
Remove debug configuration in test
f6c6811
@emasab
Merge branch 'master' into purge-brokers
90d330e
@emasab
Remove bootstrap broker after receiving learned
fd4c298 
ones, except for currently used bootstrap broker.
Wait decommissioned threads after they've stopped instead of on termination.
@emasab
Handle the _DESTROY_BROKER local error,
67d4b12 
triggered when a broker is removed
without terminating the client.
@emasab
Change test number
ac54d6a
@emasab
Test 0151 improved with cluster replacement
4200fd6 
and cluster roll
@emasab
Fix for test 0105, do_test_txn_broker_down_in_txn:
066d698 
remove left references when decommissioning a broker
and avoid it's selected as leader again or that partitions
are delegated to it
@emasab
Avoid selecting a configured broker as a logical or telemetry broker
a070ba0
@emasab
Avoid selecting terminating brokers for sending calls or new connections
27a1386
@emasab
Remove addressless count and avoid counting the logical
302c90c 
broker for the all brokers down error, to send
the error in all the cases
@confluent-cla-assistant
Copy link

🎉 All Contributor License Agreements have been signed. Ready to merge.
Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.

@pranavrth pranavrth mentioned this pull request Mar 13, 2025
Merged
emasab added 3 commits March 13, 2025 12:01
@emasab
fixup: Handle the _DESTROY_BROKER local error,
eca3fd9 
triggered when a broker is removed
without terminating the client.

Move the operation after the interceptors to avoid blocking the main thread.
@emasab
fixup: rd_kafka_brokers_learned_ids
efc3954 
Exclude terminating brokers
@emasab
Test: verify that decommissining a broker while adding a new one with…
1d04f86 
… same id isn't causing problems
emasab added 2 commits March 28, 2025 16:37
@emasab
fixup: Address rest of comments
3a2cba6
@emasab
[test 0151] Simplify the test removing await_verification. It's pos…
120cb59 
…sible to await for the correct list of brokers in all tests given since decommissioning brokers are excluded from that list
@airlock-confluentinc airlock-confluentinc bot force-pushed the dev_kip1102 branch 2 times, most recently from f294613 to 3eb055f Compare March 31, 2025 09:33
@emasab
Verify nodename change through a
d3471be 
test log interceptor.
Used the test log interceptor for test 0151 too
@airlock-confluentinc airlock-confluentinc bot force-pushed the dev_kip899 branch 2 times, most recently from e9889e5 to 919a9bc Compare April 1, 2025 17:52
Base automatically changed from dev_kip899 to master April 1, 2025 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@emasab @mfleming