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

Akka.Cluster.Sharding GetEntityLocation Query #6101

Merged

Conversation

Aaronontheweb
Copy link
Member

@Aaronontheweb Aaronontheweb commented Sep 16, 2022

Changes

Designed to make it easier for testing and telemetry, the new GetEntityLocation is an IShardRegionQuery that checks for the presence of an entity in a given ShardRegion and reports back if this entity is able to be located.

This query is not designed to be supported over the network and is meant to be local-only.

I've added this primary for testing and telemetry purposes - as we're going to look at using this inside the pbm cluster-sharding palette for Petabridge.Cmd.

Checklist

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

designed to make it easier for testing and telemetry, the new `GetEntityLocation` is an `IShardRegionQuery` that checks for the presence of an entity in a given `ShardRegion` and reports back if this entity is able to be located.

This query is not designed to be supported over the network and is meant to be local-only.
Copy link
Member Author

@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.

Reviewed my own PR to make things easier.

default:
Unhandled(query);
break;
}
}

private void ReplyToGetEntityLocationQuery(GetEntityLocation getEntityLocation, IActorRef sender)
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 is where the meat of these changes are


try
{
var shardId = ExtractShardId(new StartEntity(getEntityLocation.EntityId));
Copy link
Member Author

Choose a reason for hiding this comment

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

One thing that is awkward about this query is that the ShardExtractor has to support the ShardRegion.StartEntity type in order for this to work - the reason being is that even though we already know what the entity id is, the extractors that resolve the ShardId are looking for an input message to generate the ShardId, not the EntityId. Therefore I have to smuggle the EnttityId back through a ShardRegion.StartEntity message. That makes this a bit hacky but it's the best compromise I could make.

else
{
// ShardRegion exists, but shard is not homed
// NOTE: in the event that we're querying a shard's location from a ShardRegionProxy
Copy link
Member Author

Choose a reason for hiding this comment

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

Validated this during our unit tests.

/// In order for this query to work, the <see cref="MessageExtractor"/> must support <see cref="ShardRegion.StartEntity"/>,
/// which is also used when remember-entities=on.
/// </remarks>
public sealed class GetEntityLocation : IShardRegionQuery
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 message never gets sent over the wire, so it doesn't require any changes to the wire type nor does it need to support IClusterShardingSerializable.

Copy link
Contributor

@Arkatufus Arkatufus left a comment

Choose a reason for hiding this comment

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

LGTM, yeah, the StartEntity requirement is a bit of a bummer. But I guess this will make the solution work "out-of-the-box"-ish if the remember entities is turned on?

@Aaronontheweb Aaronontheweb marked this pull request as ready for review September 16, 2022 21:42
@Aaronontheweb
Copy link
Member Author

LGTM, yeah, the StartEntity requirement is a bit of a bummer. But I guess this will make the solution work "out-of-the-box"-ish if the remember entities is turned on?

That's correct - I went for the "least bad" solution here. I don't expect many users to actually use this query much in production, but the one problematic issue here is what we're going to do inside Petabridge.Cmd....

@Aaronontheweb
Copy link
Member Author

Going to do what we discussed in discord and see if I can eliminate the ShardRegion.StartEntity requirement on this pull request - maybe I'll use that as a fallback instead.

@Aaronontheweb
Copy link
Member Author

Sigh - looks like there's no easy way to do this without ShardRegion.StartEntity. Even built-in extractors like the HashCodeMessageExtractor are all designed to extract ShardId from a message - there's nothing exposed to compute the ShardId directly from the EntityId and there's not a lot of need for that inside the ShardRegion. Going to add some documentation for this query and then merge it in.

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.

2 participants