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

Backport of the feature called ClusterDistribution in Lagom #4455

Merged
merged 1 commit into from
Jun 17, 2020

Conversation

ismaelhamed
Copy link
Member

@ismaelhamed ismaelhamed commented Jun 7, 2020

This is a feature backported from Lagom which essentially works like a sharded singleton. A number of actor instances are kept alive and balanced across the cluster.
The main motivation to bring it into Akka is the use case where workers that update projections. By tagging akka-persistence events with N different tags and then running N workers, one for each tag which consumes that partition of the events and updates a projection.

This feature was implemented in Akka typed, not yet available in Akka.NET. But given that users will either borrow Lagom's implementation anyway or roll their own, IMO it's worthwhile having this on classical Akka.

  • Implementation
  • Unit tests
  • Multinode tests

@ismaelhamed ismaelhamed force-pushed the sharded-daemon branch 2 times, most recently from a44ef10 to 43093ab Compare June 8, 2020 11:33
@Aaronontheweb
Copy link
Member

@ismaelhamed we should add some documentation for this feature too - honestly that'll help me review it. We were just looking at akka-projections in the Gitter chat room the other day too. Seems like a useful feature in the long run.

@ismaelhamed
Copy link
Member Author

ismaelhamed commented Jun 8, 2020

@Aaronontheweb this is pretty much the only documentation you'll find, and is already included in this PR. But because this feature was borrowed from Lagom, you could also read its documentation for a little more of background (though this implementation is way more straightforward).

Actually, the recommended way to distribute the projections in akka-projections is by leveraging this new ShardedDaemonProcess.

# `role` setting limit what nodes the daemon processes and the keep alive pingers will run on.
# Some settings can not be changed (remember-entitites and related settings, passivation, number-of-shards),
# overriding those settings will be ignored.
sharding = ${akka.cluster.sharding}
Copy link
Member Author

Choose a reason for hiding this comment

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

This seems to work, but akka.cluster.sharding.coordinator-singleton = ${akka.cluster.singleton} does not. I guess the current hocon implementation cannot load a config section from another file!?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so - cc @Arkatufus

public void ShardedDaemonProcess_Should_Init_Actor_Set()
{
// HACK
RunOn(() => FormCluster(_config.First, _config.Second, _config.Third), _config.First);
Copy link
Member Author

Choose a reason for hiding this comment

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

Test does not pass unless I put FormCluster() inside a RunOn(). I'm missing something.

Copy link
Member

Choose a reason for hiding this comment

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

I'll take a look during review

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, off the top of my head, what the issue might be here - what type of error do you get?

Copy link
Member Author

Choose a reason for hiding this comment

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

I only get one ProcessActorEvent instead of the 4 expected (one per ProcessActor), so the test fails with timeout.

@ismaelhamed ismaelhamed marked this pull request as ready for review June 14, 2020 18:13
@ismaelhamed ismaelhamed changed the title [WIP] Backport of the feature called ClusterDistribution in Lagom Backport of the feature called ClusterDistribution in Lagom Jun 14, 2020
@Aaronontheweb Aaronontheweb self-requested a review June 15, 2020 21:12
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.

Have some minor changes requested here

@@ -190,7 +190,7 @@ namespace Akka.Cluster.Sharding
public override bool Equals(object obj) { }
public override int GetHashCode() { }
}
public sealed class StartEntityAck : Akka.Cluster.Sharding.IClusterShardingSerializable
public sealed class StartEntityAck : Akka.Cluster.Sharding.IClusterShardingSerializable, Akka.Event.IDeadLetterSuppression
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

public void ShardedDaemonProcess_Should_Init_Actor_Set()
{
// HACK
RunOn(() => FormCluster(_config.First, _config.Second, _config.Third), _config.First);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure, off the top of my head, what the issue might be here - what type of error do you get?

To set up a set of actors running with Sharded Daemon process each node in the cluster needs to run the same initialization
when starting up:

```csharp
Copy link
Member

Choose a reason for hiding this comment

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

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

@Aaronontheweb
Copy link
Member

Is there a reason why I can't update this PR myself? I don't see an "Update Branch" button :\

@ismaelhamed
Copy link
Member Author

ismaelhamed commented Jun 17, 2020

"Allow edits by maintainers" was unchecked. Is this option unchecked now by default?

@Aaronontheweb
Copy link
Member

@ismaelhamed maybe - I'll need to check!

@Aaronontheweb Aaronontheweb merged commit 5e559f7 into akkadotnet:dev Jun 17, 2020
@Aaronontheweb Aaronontheweb added this to the 1.4.8 milestone Jun 17, 2020
This was referenced Jun 17, 2020
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