Purge brokers no longer reported in metadata #4557

mfleming · 2023-12-12T21:33:11Z

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

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 confluentinc#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.

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 confluentinc#238.

emasab

Hi @mfleming thanks a lot for this contribution and sorry for letting it wait for long. We want to include these fixes in next version. Both fixes are good, just on broker decommission we want to do some additional checks.

Here are some comments mainly for the first fix:

tests/0145-broker-same-host-port.c

tests/0146-purge-brokers.c

src/rdkafka_broker.c

tests/0145-broker-same-host-port.c

tests/0146-purge-brokers.c

emasab · 2024-05-21T08:10:19Z

Hi @mfleming can I apply those changes or do you want to continue the PR? Thanks!

mfleming · 2024-05-21T08:17:50Z

Hey sorry for the delay — yeah you can apply those changes if you have the time. If not I’ll get to them sometime this week.

…

On Tue, 21 May 2024 at 09:10, Emanuele Sabellico ***@***.***> wrote: Hi @mfleming <https://github.com/mfleming> can I apply those changes or do you want to continue the PR? Thanks! — Reply to this email directly, view it on GitHub <#4557 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAXALUEWS5XRZGGJX6EN43ZDL6QDAVCNFSM6AAAAABASEOCUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRSGAZDKMRXG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Co-authored-by: Emanuele Sabellico <[email protected]>

rkb creation.

emasab · 2024-06-07T13:20:26Z

@mfleming thanks, sorry for the delay too, I'm checking it again

as it's now set only on creation and not modified anymore

mock cluster

emasab · 2024-06-10T13:58:08Z

/sem-approve

and move documentation

emasab · 2024-06-10T14:19:03Z

/sem-approve

emasab · 2024-06-12T08:33:18Z

We're not going to merge this for 2.5.0 that is due in July as we need to do more checks on possible regressions but we want to merge it for a maintenance release in September

mfleming · 2024-06-14T13:47:09Z

We're not going to merge this for 2.5.0 that is due in July as we need to do more checks on possible regressions but we want to merge it for a maintenance release in September

Thanks for fixing things :)

emasab · 2024-06-15T11:38:00Z

Thanks for fixing things :)

Thank you for this PR!

benesch · 2024-07-01T14:57:26Z

Just wanted to say a big thank you to both of you—@mfleming for writing this and @emasab for reviewing. We just ran into a slow thread leak in a Kafka consumer at @MaterializeInc that will be fixed by this patch.

pranavrth · 2024-09-16T23:42:54Z

While going through the PR, I found that there are a few issues in the PR.

The number of threads are increased as part of this PR. Currently, all the bootstrap servers threads are updated to the real broker thread if they appear in the metadata response. This PR creates new broker threads for the new brokers which are seen in the metadata. The bootstrap servers threads are not reused for this purpose in the PR. Due to this, we generally have 'n*2' brokers threads for 'n' bootstrap servers. Earlier there were only 'n' threads.
In the current librdkafka implementation, we always keep bootstrap broker threads even if they are not reported in the metadata. This ensured that we always have bootstrap broker threads if required to rebootstrap. This PR removed this functionality.
To fix the issue introduced in point number 2, KIP-899 needs to be implemented.

We need to fix the above issues before releasing this PR and need of more testing. As a result, it won't be part of the upcoming 2.6 release.

emasab · 2024-10-22T10:11:14Z

Closes #4881

emasab · 2024-12-13T14:58:05Z

We agreed KIP-899 and KIP-1102 aren't strictly necessary, in Java client they were disabled by default until recently.
The reason is that when a change in broker set happens slow enough to be detected by the periodic (5 min) metadata refresh, the client cannot remain without brokers.

In any case the metadata response contains at least the majority of KRaft elegible brokers. If majority changes, it must contain at least one broker from previous set.
So it's usually not possible the client remains without brokers, unless the set of brokers changes to fast it cannot reach any of them and that's the case KIP-899 and KIP-1102 are addressing. But this last case is also a problem with current librdkafka code so we leave the improvement for a later PR.

There are a few things left to check, at least:

removing bootstrap brokers without id (-1) and adding brokers with real ids and advertised hostnames, to avoid duplicating the threads
check some fields in rd_kafka_s: no_idemp_brokers, rk_telemetry.preferred_broker, rk_broker_down_cnt, rk_broker_up_cnt, rk_broker_cnt, ... when brokers are removed. It could help changing the state to DOWN before removing them.
maybe there's something to change in the stats_cb, but probably nothing to do
brokers configured with rd_kafka_brokers_add must be removed after usage too
currently, decommissioned threads are still joined on destroy, is it possible to join them from time to time without accumulating them but also without blocking the main thread because of that? At specific intervals maybe, so they're already terminated when joined
check thoroughly for memory leaks and use-after-free cases

mfleming added 3 commits November 2, 2023 11:31

Fix locking

cf114ef

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 confluentinc#238.

emasab requested changes May 16, 2024

View reviewed changes

mfleming and others added 3 commits May 24, 2024 13:53

Update tests/0145-broker-same-host-port.c

ba5a3c4

Co-authored-by: Emanuele Sabellico <[email protected]>

Update tests/0146-purge-brokers.c

a735614

Co-authored-by: Emanuele Sabellico <[email protected]>

Remove the possibility to modify rkb_nodeid after

05e8950

rkb creation.

emasab added 4 commits June 7, 2024 16:26

Remove locking when accessing rkb_nodeid

df12c6a

as it's now set only on creation and not modified anymore

Add new brokers and reassign partitions in the

4365c9e

mock cluster

CHANGELOG

6253618

Automatic style fix

d16b8ce

emasab force-pushed the purge-brokers branch from 9570aa9 to d16b8ce Compare June 10, 2024 13:56

emasab requested a review from a team as a code owner June 10, 2024 13:56

Rename to rd_kafka_brokers_learned_ids

959bf16

and move documentation

Remove debug configuration in test

f6c6811

emasab mentioned this pull request Jul 3, 2024

KIP 714 with compression support #4721

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Purge brokers no longer reported in metadata #4557

Purge brokers no longer reported in metadata #4557

mfleming commented Dec 12, 2023

emasab left a comment

emasab commented May 21, 2024

mfleming commented May 21, 2024 via email

emasab commented Jun 7, 2024

emasab commented Jun 10, 2024

emasab commented Jun 10, 2024

emasab commented Jun 12, 2024

mfleming commented Jun 14, 2024

emasab commented Jun 15, 2024 •

edited

Loading

benesch commented Jul 1, 2024

pranavrth commented Sep 16, 2024 •

edited

Loading

emasab commented Oct 22, 2024

emasab commented Dec 13, 2024 •

edited

Loading

Purge brokers no longer reported in metadata #4557

Are you sure you want to change the base?

Purge brokers no longer reported in metadata #4557

Conversation

mfleming commented Dec 12, 2023

emasab left a comment

Choose a reason for hiding this comment

emasab commented May 21, 2024

mfleming commented May 21, 2024 via email

emasab commented Jun 7, 2024

emasab commented Jun 10, 2024

emasab commented Jun 10, 2024

emasab commented Jun 12, 2024

mfleming commented Jun 14, 2024

emasab commented Jun 15, 2024 • edited Loading

benesch commented Jul 1, 2024

pranavrth commented Sep 16, 2024 • edited Loading

emasab commented Oct 22, 2024

emasab commented Dec 13, 2024 • edited Loading

emasab commented Jun 15, 2024 •

edited

Loading

pranavrth commented Sep 16, 2024 •

edited

Loading

emasab commented Dec 13, 2024 •

edited

Loading