[WIP] possible fix for #2492 #2498

maxim-s · 2017-02-02T06:08:27Z

No description provided.

Horusiath · 2017-02-02T07:17:57Z

It sounds super! ... if we actually can verify it somehow :) I've seen, that people from #2492 have shown a possible repro steps for this issue. Can we verify it somehow?

maxim-s · 2017-02-02T09:08:00Z

@Horusiath in progress of reproducing this case

maxim-s · 2017-02-02T09:09:01Z

also Akka.Cluster.Tests.ClusterSpec.A_cluster_must_complete_LeaveAsync_task_upon_being_removed racy spec

alexvaluyskiy · 2017-02-02T09:23:36Z

@maxim-s Have you tried to implement the whole PR from Akka Scala? akka/akka#21859

maxim-s · 2017-02-02T09:27:49Z

@alexvaluyskiy I didn't see this PR, but looks like we have the same issue with random gossip

maxim-s · 2017-02-02T15:24:25Z

@Aaronontheweb looks like test Akka.Cluster.Tests.ClusterSpec.A_cluster_must_complete_LeaveAsync_task_upon_being_removed is not correct now, could you review it?

alexvaluyskiy · 2017-02-02T15:48:05Z

src/core/Akka/Util/Internal/Collections/ListExtensions.cs

+namespace Akka.Util.Internal.Collections
+{
+    public static class ListExtensions
+    {


Internal should be internal, not public. Use InternalVisibleTo attribute

Aaronontheweb

I've requested some individual changes line-by-line, but a few high level notes:

Akka.Cluster.Tests.ClusterSpec.A_cluster_must_complete_LeaveAsync_task_upon_being_removed racy spec - if this is still failing, it means that #2492 is still a problem. That spec fails when the node doesn't receive a MemberRemoved event for itself when it gracefully leaves the cluster. So that test remains valid.

I'd follow @alexvaluyskiy's suggestion and implement akka/akka#21859 while you're at it. Seems to address some of the same issues we're observing.

You've already said you're working on a spec to help verify and I think that's one of the other things I'd add.

Aaronontheweb · 2017-02-03T00:15:39Z

src/core/Akka.Cluster/ClusterDaemon.cs

@@ -935,7 +936,7 @@ internal class ClusterCoreDaemon : UntypedActor, IRequiresMessageQueue<IUnbounde
        /// TBD
        /// </summary>
        protected readonly UniqueAddress SelfUniqueAddress;
-        private const int NumberOfGossipsBeforeShutdownWhenLeaderExits = 3;
+        private const int NumberOfGossipsBeforeShutdownWhenLeaderExits = 5;


What was the purpose of increasing this value? Not questioning the decision to do it, but just want to understand the reason for the change.

Ah, I get it now - increases the number of random nodes we might gossip to when the leader exits.

Aaronontheweb · 2017-02-03T00:16:43Z

src/core/Akka.Cluster/ClusterDaemon.cs

+            if (!IsSingletonCluster && n > 0)
+            {
+                var localGossip = _latestGossip;
+                var possibleTargets = new List<UniqueAddress>(localGossip.Members.Where(m => ValidNodeForGossip(m.UniqueAddress)).Select(m => m.UniqueAddress));


just call ToList here

Aaronontheweb · 2017-02-03T00:17:44Z

src/core/Akka.Cluster/ClusterDaemon.cs

@@ -2164,7 +2180,7 @@ public void ReapUnreachableMembers()
                                _cluster.SelfAddress, nonExiting.Select(m => m.ToString()).Aggregate((a, b) => a + ", " + b), string.Join(",", _cluster.SelfRoles));

                        if (!exiting.IsEmpty)
-                            _log.Warning("Marking exiting node(s) as UNREACHABLE [{0}]. This is expected and they will be removed.",
+                            _log.Warning("Cluster Node [{0}] - Marking exiting node(s) as UNREACHABLE [{1}]. This is expected and they will be removed.",


Good catch.

@Aaronontheweb Can we do this one as INFO if it is a normal exit?

@maxcherednik @maxim-s yep, I think that's a good idea.

Akka (JVM) does not have "Cluster Node" parameter in this log entry. Maybe we could just remove the parameter, instead of extending the log message?

I think it's fine

Aaronontheweb · 2017-02-03T00:19:54Z

src/core/Akka/Util/Internal/Collections/ListExtensions.cs

+{
+    public static class ListExtensions
+    {
+        public static List<T> Shuffle<T>(this List<T> @this)


Which shuffle algorithm are you using here? Fischer-Yates?

not yet, just extracted class

Aaronontheweb · 2017-02-03T00:20:47Z

src/core/Akka/Util/Internal/Collections/ListExtensions.cs

+        public static List<T> Shuffle<T>(this List<T> @this)
+        {
+            var list = new List<T>(@this);
+            var rng = new Random();


Use ThreadLocalRandom here - avoids allocations and also spreads out the seeds (as ThreadLocalRandom gets used gradually across available threads)

thank you but I thought to replace with Math.round and Math.random as Java implementation

maxim-s · 2017-02-03T15:04:48Z

@Aaronontheweb racy spec I mean, that some sort of race in the LeaveAsync task, MemberRemoved has been received success, problem with callback and I'm working on it

Aaronontheweb · 2017-02-03T16:45:56Z

@maxim-s oh I see what you mean. I must have misunderstood. So you think it's an issue with how the callback is implemented and not the spec itself?

alexvaluyskiy · 2017-02-04T13:31:40Z

@maxim-s The better way is: implement akka/akka#21859 first in a one PR. And only after merge try to fix #2492
We have to very carefully to add something which is not exists in Akka JVM

maxim-s · 2017-02-06T05:48:27Z

@Aaronontheweb yes this PR #2501 should solve problem with Akka.Cluster.Tests.ClusterSpec.A_cluster_must_complete_LeaveAsync_task_upon_being_removed

alexvaluyskiy · 2017-02-16T14:13:24Z

src/core/Akka.Cluster/ClusterDaemon.cs

@@ -2164,7 +2183,7 @@ public void ReapUnreachableMembers()
                                _cluster.SelfAddress, nonExiting.Select(m => m.ToString()).Aggregate((a, b) => a + ", " + b), string.Join(",", _cluster.SelfRoles));

                        if (!exiting.IsEmpty)
-                            _log.Warning("Marking exiting node(s) as UNREACHABLE [{0}]. This is expected and they will be removed.",
+                            _log.Warning("Cluster Node [{0}] - Marking exiting node(s) as UNREACHABLE [{1}]. This is expected and they will be removed.",
                                _cluster.SelfAddress, exiting.Select(m => m.ToString()).Aggregate((a, b) => a + ", " + b));


We could simplify this code and use something like this

_log.Info("Marking exiting node(s) as UNREACHABLE [{0}]. This is expected and they will be removed.", string.Join(", ", exiting));

alexvaluyskiy · 2017-02-16T14:16:45Z

src/core/Akka.Cluster/ClusterDaemon.cs

@@ -1125,6 +1126,7 @@ private void TryingToJoin(object message, Address joinWith, Deadline deadline)
            else if (message is InternalClusterAction.JoinSeedNodes)
            {
                var js = message as InternalClusterAction.JoinSeedNodes;
+                BecomeUninitialized();


Good catch!

Aaronontheweb · 2017-02-18T21:48:26Z

I approve the changes. @alexvaluyskiy how do you feel? Are there changes you still want to see made?

alexvaluyskiy · 2017-02-18T22:27:56Z

@Aaronontheweb I've already reviewed the code and approved it.

Horusiath added akka-cluster WIP labels Feb 2, 2017

maxim-s force-pushed the 2492 branch from 541433c to 58da9a6 Compare February 2, 2017 09:06

alexvaluyskiy reviewed Feb 2, 2017

View reviewed changes

Aaronontheweb requested changes Feb 3, 2017

View reviewed changes

kostrse mentioned this pull request Feb 4, 2017

Cluster LeaveAsync() with CancellationToken support #2501

Merged

maxim-s force-pushed the 2492 branch from c6c995b to c630a05 Compare February 10, 2017 04:31

maxim-s added 3 commits February 16, 2017 12:18

possible fix for akkadotnet#2492

94ee8e9

added random gossip

3efe1f3

cleanup

d9e89f6

maxim-s force-pushed the 2492 branch from 4d512a7 to d9e89f6 Compare February 16, 2017 08:18

alexvaluyskiy reviewed Feb 16, 2017

View reviewed changes

Aaronontheweb approved these changes Feb 18, 2017

View reviewed changes

Aaronontheweb merged commit e628f73 into akkadotnet:dev Feb 19, 2017

Aaronontheweb added this to the 1.1.4 milestone Feb 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] possible fix for #2492 #2498

[WIP] possible fix for #2492 #2498

maxim-s commented Feb 2, 2017

Horusiath commented Feb 2, 2017

maxim-s commented Feb 2, 2017

maxim-s commented Feb 2, 2017

alexvaluyskiy commented Feb 2, 2017

maxim-s commented Feb 2, 2017

maxim-s commented Feb 2, 2017

alexvaluyskiy Feb 2, 2017

Aaronontheweb left a comment •

edited

Loading

Aaronontheweb Feb 3, 2017

Aaronontheweb Feb 3, 2017

Aaronontheweb Feb 3, 2017

Aaronontheweb Feb 3, 2017

maxcherednik Feb 3, 2017

Aaronontheweb Feb 4, 2017

alexvaluyskiy Feb 16, 2017

Aaronontheweb Feb 18, 2017

Aaronontheweb Feb 3, 2017

maxim-s Feb 3, 2017

Aaronontheweb Feb 3, 2017

maxim-s Feb 3, 2017

maxim-s commented Feb 3, 2017

Aaronontheweb commented Feb 3, 2017

alexvaluyskiy commented Feb 4, 2017

maxim-s commented Feb 6, 2017

alexvaluyskiy Feb 16, 2017

alexvaluyskiy Feb 16, 2017

Aaronontheweb commented Feb 18, 2017

alexvaluyskiy commented Feb 18, 2017

[WIP] possible fix for #2492 #2498

[WIP] possible fix for #2492 #2498

Conversation

maxim-s commented Feb 2, 2017

Horusiath commented Feb 2, 2017

maxim-s commented Feb 2, 2017

maxim-s commented Feb 2, 2017

alexvaluyskiy commented Feb 2, 2017

maxim-s commented Feb 2, 2017

maxim-s commented Feb 2, 2017

Choose a reason for hiding this comment

Aaronontheweb left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxim-s commented Feb 3, 2017

Aaronontheweb commented Feb 3, 2017

alexvaluyskiy commented Feb 4, 2017

maxim-s commented Feb 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aaronontheweb commented Feb 18, 2017

alexvaluyskiy commented Feb 18, 2017

Aaronontheweb left a comment •

edited

Loading