Coordinated downing improvements #3551
Conversation
…erStatus is Joining/WeaklyUp/Up.
|
To be honest I think that #3284 may have covered this PR. |
|
It might, but I guess that's still far away. |
| } | ||
|
|
||
| /// <summary> | ||
| /// Using region 2 as it is not shutdown in either test. |
There was a problem hiding this comment.
Good to know (the comment was helpful during my review :p)
| @@ -379,8 +379,15 @@ private void SetupCoordinatedShutdown() | |||
| var self = Self; | |||
| _coordShutdown.AddTask(CoordinatedShutdown.PhaseClusterShardingShutdownRegion, "region-shutdown", () => | |||
| { | |||
| self.Tell(GracefulShutdown.Instance); | |||
| return _gracefulShutdownProgress.Task; | |||
| if (Cluster.IsTerminated || Cluster.SelfMember.Status == MemberStatus.Down) | |||
| /// predefined reasons, but external libraries applications may also define | ||
| /// other reasons. | ||
| /// </summary> | ||
| public class Reason |
There was a problem hiding this comment.
Actually this should be an enum.
There was a problem hiding this comment.
Disagree. If it needs properties and data that can be overridden in the future, for whatever reason, you can't have that extensibility with an Enum
There was a problem hiding this comment.
I.E. the CustomReason example used in the spec earlier illustrates one such case for that.
There was a problem hiding this comment.
Then you can easily change it into tagged union or just change from enum to class. It's time to stop putting enum-like types into singletons:
- They blow up the code base.
- They are hardly maintainable. You need a lot of boilerplate to keep things like equality, hash codes and string formatting in place.
- They are not safe - you never know if you have covered all switch cases and no tool on earth will help you with that.
- They are slow and memory-consuming (this don't matter much in this particular case).
If you can model some cases as enums, you should do that. This is not JVM, they won't be optimized automatically.
There was a problem hiding this comment.
None of the above is true in this particular case - it's a class that gets used once on shutdown. I don't think these are even true generally.
There was a problem hiding this comment.
I actually started this as an enum, but then I got to the custom reason and I got stuck.
To support custom reasons with enum, we'd need to cast it to another enum extended with the custom reasons and I didn't like that. Honestly, the current approach I also don't like so much
| @@ -1668,7 +1668,7 @@ public void Welcome(Address joinWith, UniqueAddress from, Gossip gossip) | |||
| public void Leaving(Address address) | |||
| { | |||
| // only try to update if the node is available (in the member ring) | |||
| if (_latestGossip.Members.Any(m => m.Address.Equals(address) && m.Status == MemberStatus.Up)) | |||
| if (_latestGossip.Members.Any(m => m.Address.Equals(address) && (m.Status == MemberStatus.Joining || m.Status == MemberStatus.WeaklyUp || m.Status == MemberStatus.Up))) | |||
There was a problem hiding this comment.
Resolves #2347 while we're at it.
contains
#3263
akka/akka#24048
akka/akka#24140
akka/akka#23449