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.

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

No good way to watch leadership #426

Closed
Jille opened this issue Sep 8, 2020 · 6 comments · May be fixed by #427
Closed

No good way to watch leadership #426

Jille opened this issue Sep 8, 2020 · 6 comments · May be fixed by #427
Labels
enhancement waiting-reply

Comments

@Jille
Copy link
Contributor

Jille commented Sep 8, 2020

Raft.LeaderCh() has two problems: It always returns the same channel and it doesn't prepopulate the channel with the current value. Reconstructing this has proven error prone/impossible.

RegisterObserver() was my next attempt, but the LocalAddress isn't exposed (except by the Transport, which not be available to library code).

I propose to make Raft.LeaderCh() return a new channel per call, and prepopulate that with the current value. That's a slight behavior change, but I don't expect it to break clients.
Jille added a commit to Jille/raft that referenced this issue Sep 8, 2020
@Jille
Make Raft.LeaderCh() return a new channel for each invocation
bee73bc 
.. and immediately send the current leadership state over the channel.

This way it can be used by multiple pieces of code with disrupting another.

Sending the value immediately avoids strange race conditions when RaftState gets updated at a slightly different moment.

fixes hashicorp#426
@Jille Jille mentioned this issue Sep 8, 2020
Open
@stale
Copy link

stale bot commented Nov 7, 2020

Hey there,
We wanted to check in on this request since it has been inactive for at least 90 days.
Have you reviewed the latest godocs?
If you think this is still an important issue in the latest version of the Raft library or
its documentation please feel let us know and we'll keep it open for investigation.
If there is still no activity on this request in 30 days, we will go ahead and close it.
Thank you!

@stale stale bot added the waiting-reply label Nov 7, 2020
@Jille
Copy link
Contributor Author

Jille commented Nov 7, 2020

still relevant

@stale stale bot removed the waiting-reply label Nov 7, 2020
@schristoff
Copy link
Contributor

Howdy @Jille,
Thank you for bringing this issue up. I have a couple of follow up questions, but I think I would benefit on understanding the use case. Are you able to elaborate on how you're using Raft.LeaderCh() and how these enhancements would benefit your implementation in a bit more detail?
Also, would you be able to elaborate on why you'd rather have it return a new channel per call?

Thank you!

@Jille
Copy link
Contributor Author

Jille commented Jan 5, 2021

Hey @s-christoff. I wrote a library that marks the gRPC server as unhealthy if it's not the raft leader.

With the current implementation only one place can get the channel and watch it for leader changes. If multiple places would use it, they would steal events from the channel from another and they'd all get an incomplete view. This makes it impossible for a library to use it, because it can't guarantee that the application isn't using it.

https://github.com/Jille/raft-grpc-leader-rpc/blob/reliable-leaderch/leaderhealth/leaderhealth.go#L24 checks whether my patch (#427) is in and uses it if available. The alternative is using LeaderObservation, but that has race conditions.

@ncabatoff
Copy link
Contributor

Hi @Jille,

Sorry this has dragged on for so long. Can you elaborate on the race you speak of with LeaderObservation? I'm aware that it can be lossy, but you can specify that you want blocking, such that events will not be discarded. I think the PR you've authored is probably fine, but I'd prefer to avoid having multiple mechanisms for doing this if possible.

@stale
Copy link

stale bot commented Oct 2, 2021

Hey there, This issue has been automatically closed because there hasn't been any activity for a while. If you are still experiencing problems, or still have questions, feel free to open a new one :+1

@stale stale bot closed this as completed Oct 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement waiting-reply
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make Raft.LeaderCh() return a new channel for each invocation Jille/raft
3 participants
@Jille @ncabatoff @schristoff