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

ShoppingCartEntity with Akka Typed #1

Closed
wants to merge 8 commits into from

Conversation

patriknw
Copy link

Thought I'd try to convert the Java ShoppingCart to Akka Typed.
First commit is only the copy of the original so look at second commit if you want to see a side-by-side comparison.

Not running yet, because I need the new serializer (for ActorRef). I intend to use this as a playground for rolling updates for Lagom to Akka Typed migration.

@patriknw
Copy link
Author

I have updated and tried this more. My conclusion is that we can support rolling updates from Lagom's PersistentEntity to Akka's EventSourcedEntity with partial outage for some requests.

  • commands from old service to new EventSourcedEntity will not be delivered (resulting in ask timeout)
  • commands from new service to old PersistentEntity will not be delivered (resulting in ask timeout)

The reasons we can't (without much effort) support those cross-requests are:

  • commands are wrapped in different envelopes and then the message extractors would have to understand both envelopes com.lightbend.lagom.javadsl.persistence.CommandEnvelope vs
    akka.cluster.sharding.typed.ShardingEnvelope
    • users could make that work in one direction with a custom message extractor, see my attempt in 970352d, but there are other problems so not worth recommending
  • problems with replies via sender() vs replyTo in message

I think that is anyway pretty good, since the partial failures heal once the full rollout is completed.

@patriknw
Copy link
Author

This is what I have used for testing.

@ignasi35
Copy link
Collaborator

This looks great!

We forgot, on the original code, to shard the tags and it'd be interesting to expose that too. It's a few lines of code but it'd be great to surface all the pieces Lagom does for free. Would this be enough?

  @Override
  public Set<String> tagsFor(Event event) {
    String entityId = getEntityId();
    // When migrating, `numShards` should be extracted 
    // from the `AggregateEventShards.numShards`
    String tag = entityId + selectShard(numShards, entityId);
    // whatever idiom to build a Set with a single-item that's 
    // fancy in Java nowadays.
    Set<String> set = new HashSet<>(1); 
    // TODO: memoize these (entityId,Set<String>) pairs
    set.add(tag);
    return set;
  }

  private String selectShard(int numShards, String entityId) {
    return Math.abs(entityId.hashCode) % numShards;
  }

That, and some other repetition (EntityTypeKey and, entity creation and sharding) could be provided by a thin layer of Lagom code (or even in Akka?).

@patriknw
Copy link
Author

@ignasi35 I can probably make Math.abs(entityId.hashCode) % numShards a public utility in Akka. It's funny, because it's actually slightly incorrect. It should be Math.abs(entityId.hashCode % numShards), see akka/akka#25034
but to avoid getting different values when rolling upgrade we keep the old way, and it doesn't have any serious consequences

An implementation note about your suggestion is that the Set(tag) can be evaluated once, since always the same for the entity.

@ignasi35
Copy link
Collaborator

An implementation note about your suggestion is that the Set(tag) can be evaluated once, since always the same for the entity.

ah, ofc! 😅

@octonato
Copy link
Owner

octonato commented Jun 25, 2019

FYI...

We have an open issue in Lagom lagom/lagom#1877 that shows how the Math.abs(entityId.hashCode) % numShards could cause real issues. I agree that it's very unlikely to have a hashCode resulting in Int.MinValue, but it's not impossible.

Note, there are two cases here.

One thing is the distribution of shard entities and I don't care if we accidentally create one extra shard, probably containing one single instance - what are the odds we get two entities with an id.hashCode resulting in Int.MinValue?

The other case is then we use this same strategy to shard tags. A tag sharded with a negative number will be invisible for read-side processors and topic producers (and upcoming, akka-projections).

The problem is that when we later want to know what are the shard identifiers, we do things like:

val allTags: Set[AggregateEventTag[Event]] = {
    (for (shardNo <- 0 until numShards) yield new AggregateEventTag(
      eventType,
      AggregateEventTag.shardTag(tag, shardNo)
    )).toSet
  }

This actually shows that we are expecting them to be all positive numbers.

I don't know what is the best way to solve that in Lagom.

Considering the unlikeness of running into this, maybe the best solution is to fix the sharded tagging in Lagom and leave a note to users to check their journal and eventually fix by hand in case they notice a tag with a negative shard number.

@ignasi35
Copy link
Collaborator

I don't know what is the best way to solve that in Lagom.

how about lagom/lagom#1877 (comment) ? Stay backwards compatible and fixes Int.MinValue.

@octonato
Copy link
Owner

@patriknw, can you explain "...but there are other problems so not worth recommending"? I imagine that this is mainly related with how we will be serializing/deserializing the commands, correct?

So, here is the follow-up question...

When an untyped node sends a command to a typed node, can we pimp the serialized command with an ActorRef[Response]?

But to be honest, I consider the case of less importance. A partial outage with very quickly full recovery is mostly acceptable. And if not, users can still choose to not migrate to Typed.

If we have a situation in which it's absolutely necessary to provide such a feature we could then consider the engineering effort for doing that.

@patriknw
Copy link
Author

but there are other problems so not worth recommending

Mostly around sender() vs replyTo in message. Akka Typed doesn't know about the sender, and Lagom PersistentEntity doesn't know about the replyTo in the message.

With tricks as I did in akka/akka-samples#110 we could probably get it working, at least for some of the cases, but it would require changes in Lagom and user code.

When an untyped node sends a command to a typed node, can we pimp the serialized command with an ActorRef[Response]?

Adding a replyTo field in the json with the value of sender. Might be possible, but I can't immediately see that we can get hold of the sender when serializing the command or the CommandEnvelope.

But to be honest, I consider the case of less importance. A partial outage with very quickly full recovery is mostly acceptable. And if not, users can still choose to not migrate to Typed.

If we have a situation in which it's absolutely necessary to provide such a feature we could then consider the engineering effort for doing that.

I agree, let's decide that.

@octonato
Copy link
Owner

octonato commented Sep 5, 2019

@patriknw, this PR has served its purpose to demonstrate / experiment with some ideas. I'm closing it because the master this demo project diverged quite a lot.

@octonato octonato closed this Sep 5, 2019
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.

3 participants