Skip to content
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

[PoC] distributed actor gossiper #1159

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ktoso
Copy link
Member

@ktoso ktoso commented Jul 23, 2024

Second commit here shows off how we can implement a simple gossip mechanism.

This is just a quick PoC not a complete implementation and implemented as a sample rather than the core of the library for now, but we should move that over.

@ktoso ktoso changed the title poc of distributed actor gossiper; the algorithm can be improved independent of user code [PoC] distributed actor gossiper Jul 23, 2024
Copy link
Contributor

@akbashev akbashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small comments from my side.

imho should be merged even in the form of PoC, benefits are big and we can figure out later on how to improve.

}

print("Sleeping...")
_Thread.sleep(duration)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess if there is a plan to update repo, we can stop using _Thread and use Task.sleep(for:) instead already?

Comment on lines +96 to +103
guard let target = self.gossipPeers.shuffled().first else {
log.info("No peer to gossip with...")
return
}

guard target.id != self.id else {
return try await gossipRound() // try again
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe missing something, but a bit confusing guard, like why not to filter for id in set?

}

guard target.id != self.id else {
return try await gossipRound() // try again
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also while testing and while cluster is forming this function goes in loop heavily 🤔 maybe adding something like:

Suggested change
return try await gossipRound() // try again
if self.gossipPeers.count == 1 {
try await Task.sleep(for: .seconds(0.1))
}
return try await gossipRound() // try again

Comment on lines +74 to +78
// distributed func acknowledgement(_ acknowledgement: Acknowledgement,
// from peer: ID,
// confirming gossip: Gossip) {
//
// }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it needed? Not like nitpicking, but rather thinking about future refactoring—I've noticed there are some commented code here and there (not much though) and sometimes not sure if I should just remove it or not.

@@ -34,7 +34,7 @@ public struct InvocationMessage: Sendable, Codable, CustomStringConvertible {

// FIXME(distributed): remove [#957](https://github.com/apple/swift-distributed-actors/issues/957)
enum InvocationBehavior {
static func behavior(instance weakInstance: Weak<some DistributedActor>) -> _Behavior<InvocationMessage> {
static func behavior(instance weakInstance: WeakLocalRef<some DistributedActor>) -> _Behavior<InvocationMessage> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if we can just remove WeakLocalRef here and just check for weak in closures bellow.

Comment on lines +251 to +254
enum _Strategy {
case random
case simpleRoundRobin
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants