Modernize DistributedPubSub code #7640
Conversation
Aaronontheweb
left a comment
Aaronontheweb
left a comment
There was a problem hiding this comment.
Overall looks good but I left you some more perf feedback on the gossip side of things.
| throw new ArgumentException($"The cluster member [{_cluster.SelfAddress}] doesn't have the role [{_settings.Role}]"); | ||
|
|
||
| //Start periodic gossip to random nodes in cluster | ||
| _gossipCancelable = Context.System.Scheduler.ScheduleTellRepeatedlyCancelable(_settings.GossipInterval, _settings.GossipInterval, Self, GossipTick.Instance, Self); |
There was a problem hiding this comment.
LGTM (should probably keep the comment though)
There was a problem hiding this comment.
the comment was moved to the PreStart() override where we start the periodic timer
| { | ||
| var nodes = state.Members | ||
| .Where(m => m.Status != MemberStatus.Joining && IsMatchingRole(m)) | ||
| .Where(m => m.Status != MemberStatus.Joining && IsMatchingRole(m, _role)) |
There was a problem hiding this comment.
No, it wasn't a bug. We access the _settings.Role property inside the IsMatchingRole method before, now it is a static method
| private void HandleGossip() | ||
| { | ||
| var node = SelectRandomNode(_nodes.Except(new[] { _cluster.SelfAddress }).ToArray()); | ||
| var node = SelectRandomNode(_nodes.Except([_cluster.SelfAddress]).ToArray()); |
There was a problem hiding this comment.
You might be able to make this work without allocating a list - have SelectRandomNode take an IEnumerable and just apply the random value to .Skip instead of the indexer
There was a problem hiding this comment.
That'll also save you the trouble of fully evaluating the list
There was a problem hiding this comment.
I'll give that a try
Aaronontheweb
left a comment
Aaronontheweb
left a comment
There was a problem hiding this comment.
LGTM - we decided that the gossip optimization wasn't worth the complexity it introduces
2 participants
Changes
Modernize DistributedPubSub code
DistributedPubSubMediatoruseIWithTimersinstead of using the schedulerTopicLike(base class forTopicandGroup) useIWithTimersinstead of using the schedulerChecklist
For significant changes, please ensure that the following have been completed (delete if not relevant):
Latest
devBenchmarksOSVersion: Microsoft Windows NT 6.2.9200.0
ProcessorCount: 24
ClockSpeed: 0 MHZ
Actor Count: 48
Messages sent/received per client: 200000 (2e5)
Is Server GC: True
Thread count: 52
This PR's Benchmarks
OSVersion: Microsoft Windows NT 6.2.9200.0
ProcessorCount: 24
ClockSpeed: 0 MHZ
Actor Count: 48
Messages sent/received per client: 200000 (2e5)
Is Server GC: True
Thread count: 55