-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Lease API & integration #4344
Lease API & integration #4344
Conversation
sharding integration would be better to do after Timers API (#3778) |
@zbynek001 is #3778 good to go? |
@Aaronontheweb in theory yes. There is only the open question if the code in |
@zbynek001 ok, got it - I'll review the PR then benchmark it. |
@zbynek001 reviewed #3778 - do you need to address those before this PR gets finished? |
Refactor OldestData to use option for actor reference
8387dd2
to
66baa35
Compare
Working on reviewing this now that #3778 has been merged. Might take a bit as I'm familiarizing myself with the API as I go. Going to need to add documentation for this too if the API is public. |
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 - have some questions, but we really need to move the leasing code out of the core Akka module and into its own project instead: Akka.Coordination.
src/contrib/cluster/Akka.Cluster.Sharding.Tests/Akka.Cluster.Sharding.Tests.csproj
Show resolved
Hide resolved
src/contrib/cluster/Akka.Cluster.Sharding.Tests/ClusterShardingLeaseSpec.cs
Outdated
Show resolved
Hide resolved
Log.Info("Oldest observed OldestChanged: [{0} -> {1}]", _cluster.SelfAddress, oldestChanged.Oldest?.Address); | ||
switch (oldestChanged.Oldest) | ||
Log.Info("Acquire lease result {0}", alr.HoldingLease); | ||
if (alr.HoldingLease) |
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.
So, removing a lot of this business logic from the cluster singleton implementation itself and moving it to the leasing system - ok, that makes sense.
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.
Oh I see - it's been moved to the state rather than the event-handling methods themselves.
One question I have that wasn't totally clear to me based on what I reviewed - do any of these leasing messages go over the wire? Didn't look that way when I first looked but it might be implementation-specific for things like Akka.Cluster.Tools.Singleton |
@zbynek001 this will be a great addition to Akka.NET - happy to do it once we can address these comments. |
@Aaronontheweb basically no messages here go over wire, this is just adding API to create your own |
@Aaronontheweb I think all changes should be done. Not sure about the new projects, if there is something else to change in build script or somewhere else |
@zbynek001 I'll check that now. |
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.
Code looks great - just need some help with meta-data, XML-DOC, and copyright headers in a couple of areas.
Nice work @zbynek001 |
General question about this, this a timed lease that works similar to Azure Blob leases? ie Only one actor/process can hold a reference to an actor in a bounded time period? |
@Lutando that's correct - you could implement a lease that is backed by Azure blobs, a Redis object, etc... |
Note: this depends on Timers (#3778), will rebase it when it's merged
Open quesions:
migrated from akka/akka#26629