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

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

Merged
merged 2 commits into from
Aug 7, 2023

Conversation

milindl
Copy link
Contributor

@milindl milindl commented Aug 3, 2023

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.

@milindl milindl requested a review from anchitj August 3, 2023 09:24
@cla-assistant
Copy link

cla-assistant bot commented Aug 3, 2023

CLA assistant check
All committers have signed the CLA.

@milindl milindl changed the title Add broker selection and client termination Add broker selection and client termination [KIP-714] Aug 3, 2023
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
Member

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
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
Member

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
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
Member

@anchitj 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 milindl merged commit ef07a95 into dev_kip_714 Aug 7, 2023
@milindl 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