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

feat: add get_coordinator function in group subscriber v2 #608

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yastanotheruser
Copy link

Add an API function to get the group subscriber process coordinator PID. This allows to dynamically change the topics the group subscribes to by calling brod_group_coordinator:update_topics.

@zmstone
Copy link
Contributor

zmstone commented Nov 16, 2024

to lower the chance of race condition, maybe delete the gen_call to group subscriber v2.

@yastanotheruser
Copy link
Author

sorry I'm a bit slow. how could I read the state of the subscriber without a call to the gen_server?

@zmstone
Copy link
Contributor

zmstone commented Nov 24, 2024

sorry I'm a bit slow. how could I read the state of the subscriber without a call to the gen_server?

hm.... my bad, I must have lost a few lines when copy-paste.
The race condition (although very unlikely to happen) is:

  • Application code calls get_coordinator
  • The coordinator process has to restart
  • Application code calls update_topics

If what you want is to be able to call brod_coordinator:update_topics, I would suggest to expose brod_group_subscriber_v2:update_topics which delegates the gen_cast to coordinator process.

And add a test case to verify that new subscribers will be started and messages will be fetched after the function is called.

@zmstone
Copy link
Contributor

zmstone commented Nov 24, 2024

We can still add get_coordinator API, maybe it can be useful for some other purposes too.

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