-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Modernize DistributedPubSub code #7640
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
Modernize DistributedPubSub code #7640
Conversation
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (should probably keep the comment though)
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this a bug before?
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.
No, it wasn't a bug. We access the _settings.Role
property inside the IsMatchingRole
method before, now it is a static method
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.
Got it
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'll also save you the trouble of fully evaluating the list
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.
I'll give that a try
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 decided that the gossip optimization wasn't worth the complexity it introduces
Changes
Modernize DistributedPubSub code
DistributedPubSubMediator
useIWithTimers
instead of using the schedulerTopicLike
(base class forTopic
andGroup
) useIWithTimers
instead of using the schedulerChecklist
For significant changes, please ensure that the following have been completed (delete if not relevant):
Latest
dev
BenchmarksOSVersion: 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