Skip to content

Add broker selection and client termination [KIP-714]#4382

Merged
Milind L (milindl) merged 2 commits intodev_kip_714from
dev_kip_714_broker_selection
Aug 7, 2023
Merged

Add broker selection and client termination [KIP-714]#4382
Milind L (milindl) merged 2 commits intodev_kip_714from
dev_kip_714_broker_selection

Conversation

@milindl
Copy link
Copy Markdown
Contributor

Main changes:

  1. rd_kafka_get_preferred_broker now handles the case when the previous broker was lost
  2. termination is handled gracefully, both in terms of clearing resources, and in terms of sending a Push request with the last pieces of information.

To accommodate this:
Makes changes so that the entire state machine runs on the main thread.
Communication with the broker thread (when preferred broker has to be set) and with the app thread (during termination) is done via ops or conditional variables.
Also updates some documentation comments.

@cla-assistant
Copy link
Copy Markdown

cla-assistant Bot commented Aug 3, 2023

CLA assistant check
All committers have signed the CLA.

@milindl Milind L (milindl) changed the title Add broker selection and client termination Add broker selection and client termination [KIP-714] Aug 3, 2023
Comment thread src/rdkafka_telemetry.c Outdated
rd_kafka_dbg(rk, TELEMETRY, "TELTERM",
"Awaiting termination of telemetry.");
mtx_lock(&rk->rk_telemetry.lock);
cnd_wait(&rk->rk_telemetry.termination_cnd, &rk->rk_telemetry.lock);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it make sense to do a timed_wait if for some case RD_KAFKA_TELEMETRY_TERMINATING_PUSH_SCHEDULED gets blocked somewhere?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

How much should we wait for? 1s?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm not sure, but I think 1s should be good. We can probably tweak it later if the terminating push takes more time.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, 1s

Copy link
Copy Markdown
Contributor

@anchitj Anchit Jain (anchitj) left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM!

@milindl Milind L (milindl) merged commit ef07a95 into dev_kip_714 Aug 7, 2023
@milindl Milind L (milindl) deleted the dev_kip_714_broker_selection branch August 7, 2023 07:01
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.

2 participants