-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Make JoinAsync and JoinSeedNodesAsync more robust by checking cluster UP status #6033
Make JoinAsync and JoinSeedNodesAsync more robust by checking cluster UP status #6033
Conversation
by design, although the member up actors could be programmed to shut themselves down after they've executed. |
Unlikely with closures, but theoretically possible. I wouldn't worry about it - end-user's responsibility. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes don't look 100% safe to me
Changed how the state is handled, no longer using null as a marker. @Aaronontheweb @to11mtm, Would be great if you can re-review this code again. |
src/core/Akka.Cluster/Cluster.cs
Outdated
{ | ||
_isUp.GetAndSet(true); | ||
// If there is an async join operation in progress, complete it. | ||
_asyncJoinTaskSource?.Complete(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a -little- closer...
IMO Ideally, we should only lock long enough to see if there is an existing continuation set up, and if so re-use that.
Does _asyncJoinTaskSource
have a TryComplete
to use instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It uses TryComplete
internally, the lock is to guard against possible null value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_asyncJoinTaskSource
is actually a custom wrapper class around TaskCompletionSource
to provide timeout with custom exception support.
….com:Arkatufus/akka.net into cluster/fix_JoinAsync_and_JoinSeedNodesAsync
Here is how I would simplify this:
|
I've checked the cluster status change listener actor, its appropriately designed, no problem there. |
….com:Arkatufus/akka.net into cluster/fix_JoinAsync_and_JoinSeedNodesAsync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
_clusterDaemons.Tell(new InternalClusterAction.AddOnMemberRemovedListener(() => tcs.TrySetResult(null))); | ||
_clusterDaemons.Tell(new InternalClusterAction.AddOnMemberRemovedListener(() => | ||
{ | ||
tcs.TrySetResult(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We used the `cluster.seed-node-timeout` property incorrectly in akkadotnet#6033 - that is a "how long did it take from me to hear back from a seed" setting, but we're using it like a "how much time do I have to join the cluster?" setting. Added a function designed to give us at least 20s of leeway.
* fix timing regressions in `Cluster.JoinAsync` methods We used the `cluster.seed-node-timeout` property incorrectly in #6033 - that is a "how long did it take from me to hear back from a seed" setting, but we're using it like a "how much time do I have to join the cluster?" setting. Added a function designed to give us at least 20s of leeway. * Update Cluster.cs * fixed addr * Fix timeout calculation * Fix CancellationTokenSource --------- Co-authored-by: Gregorius Soedharmo <[email protected]>
Changes
JoinAsync and JoinSeedNodesAsync are now idempotent, always returning the same Task instance when called multiple times.More robust memory clean-upKISS and code reuse, both JoinAsync and JoinSeedNodesAsync shares the same async instance.