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

Singleton class and settings based on current Akka Typed implementation #6050

Merged
merged 2 commits into from
Aug 12, 2022

Conversation

ismaelhamed
Copy link
Member

@ismaelhamed ismaelhamed commented Jul 27, 2022

Fixes #6036

Changes

Since this already exists in Akka Typed and it isn't built on top of anything Akka-Typed specific, I put together a quick port with this functionality:

  • If there already is a manager running for the given singletonName on this node, no additional manager is started.
  • If there already is a proxy running for the given singletonName on this node, an IActorRef to that is returned.

Usage

var singleton = ClusterSingleton.Get(system);
var settings = ClusterSingletonSettings.Create(system).WithRole("singleton");

var proxy = singleton.Init(SingletonActor.Create(Props.Create<GlobalCounter>(), "GlobalCounter").WithSettings(settings));
proxy.Tell(Counter.Increment);

This will required a complete cluster restart though, since the names for both the ClusterSingletonManager and ClusterSingletonManagerProxy are now automatically constructed (i.e., based on the code above, the new manager will be named singletonManagerGlobalCounter and the proxy singletonProxyGlobalCounter).

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

@Aaronontheweb
Copy link
Member

This looks great @ismaelhamed - I'll review this shortly.

@ismaelhamed
Copy link
Member Author

ismaelhamed commented Jul 29, 2022

The only thing I've found with this implementation is that if the user already have a ClusterSingletonManager that is persistent, due to how the manager's name is now automatically constructed, it probably won't match the existing PersistentId in the journal/snapshot-store tables.

@ismaelhamed ismaelhamed force-pushed the clustersingleton-ext branch from 0cf3f2f to dd5e0ef Compare July 30, 2022 10:24
@ismaelhamed
Copy link
Member Author

Added some test and documentation.

@ismaelhamed ismaelhamed force-pushed the clustersingleton-ext branch 2 times, most recently from b5fc70c to 3e461d3 Compare July 30, 2022 16:51
@Aaronontheweb
Copy link
Member

The only thing I've found with this implementation is that if the user already have a ClusterSingletonManager that is persistent, due to how the manager's name is now automatically constructed, it probably won't match the existing PersistentId in the journal/snapshot-store tables.

What would an end-user need to do to work around that? Is there something we'll have to document or help them migrate with?

@ismaelhamed
Copy link
Member Author

The only thing I've found with this implementation is that if the user already have a ClusterSingletonManager that is persistent, due to how the manager's name is now automatically constructed, it probably won't match the existing PersistentId in the journal/snapshot-store tables.

What would an end-user need to do to work around that? Is there something we'll have to document or help them migrate with?

I would probably only recommend using this newer API for brand new applications since, at the very least, a cluster restart is needed. IMO, if you already have your singletons figured out, this will only save you a few lines of code which may not be worth the effort.

@ismaelhamed ismaelhamed marked this pull request as ready for review August 8, 2022 14:33
@Aaronontheweb Aaronontheweb added this to the 1.5.0 milestone Aug 9, 2022
@ismaelhamed ismaelhamed force-pushed the clustersingleton-ext branch from 3e461d3 to 589d69c Compare August 12, 2022 10:22
@Aaronontheweb
Copy link
Member

it probably won't match the existing PersistentId in the journal/snapshot-store tables.

so this will only affect users who are using the singleton path as the PersistenceId - not the end of the world, but I appreciate the mention here. We'll make sure to include that in the release notes.

Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

LGTM - greatly simplifies the API for managing ClusterSingletons. This is a major usability improvement.


Any `Actor` can be run as a singleton. E.g. a basic counter:

```csharp
Copy link
Member

Choose a reason for hiding this comment

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

Generally, we prefer to use DocFx's [code] blocks that reference a runnable source file instead of traditional markdown blocks - see https://getakka.net/community/contributing/documentation-guidelines.html#code-samples-must-use-code-references for examples on how to do this.

Reason why we care: if the sample is updated, the documentation is also automatically updated too.

Copy link
Member

Choose a reason for hiding this comment

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

I'm going to merge this in now and we can update those docs to reference your unit tests' code in a separate PR.

}

[Fact]
public void A_cluster_singleton_must_be_accessible_from_two_nodes_in_a_cluster()
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

}
catch (InvalidActorNameException ex) when (ex.Message.EndsWith("is not unique!"))
{
// This is fine. We just wanted to make sure it is running and it already is
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@Aaronontheweb Aaronontheweb enabled auto-merge (squash) August 12, 2022 13:35
@Aaronontheweb Aaronontheweb merged commit 899c3a2 into akkadotnet:dev Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Akka.Cluster.Singleton: should singleton creation match what we do with ShardRegions?
2 participants