feat: Spike of API changes, #50 #62

patriknw · 2021-06-23T09:32:12Z

API proposal that is introducing the Effect return type and changes to state management.

This is a "compile only" spike until we agree on the API.

Probably easies to look at the changes of the examples:

valueentity-counter ... CounterImpl.java
java-eventsourced-shopping-cart ... ShoppingCartImpl.java

Refs #50

patriknw · 2021-06-23T12:51:49Z

...entsourced-shopping-cart/src/main/java/com/example/shoppingcart/domain/ShoppingCartImpl.java

-        .addAllItems(cart.values().stream().map(this::convert).collect(Collectors.toList()))
-        .build();
+  protected ShoppingCartDomain.Cart emptyState() {
+    return ShoppingCartDomain.Cart.getDefaultInstance();


This is something we need to decide. Shall we have an emptyState or use Optional in all currentState parameters? For ValueEntity it's Optional.

I like how explicit this is.

If we have emptyState we have to include make that accessible from the SDK also, not only in generated code like this. We could have an interface or abstract class that the impl has to implement.

@octonato not that I would like to bring back memories to our old discussion about emptyState 😄 , but what do you think we should do here? emptyState or Optional parameters everywhere? Same in ValueEntity or is the difference motivated?

Thanks for bring back those memories! :-)

My whole idea in the past is that I wanted to avoid exposing users to Option because I believe there is a clear point in time where the journal is empty and therefore we start with None. So, first event moves from None to Some. All subsequent events can only work on a Some.

From that observation, we could say there are two possible event handlers (and command handlers): Event => State and (State, Event) => State. And we can then hide the Option from the user.

Anyway, that just to remind my opinion about it since you triggered my memories. :-)

Back to the present...

I think we don't want to introduce two kinds of command and event handlers because that would require the user to annotate the methods in the proto file. Event handlers would even be more challenging because there are not even defined in the proto file.

So, that to say, my opinion is that we should not bother users with an Optional type if most of the time it will be a Some. Only the first command handler and event handler will see it as whatever the user defines as empty state.

And most probably, java developers will use a null.

For the ValueEntity, I think we can apply the same reasoning. The emtpyState will only be used when there is no row (including after deleting a row and starting over again).

I agree. Then I introduce emptyState in ValueEntity also to make it consistent.

null or the proto empty instance can be used as starting point.

Added emptyState for ValueEntity also: bd4aff8

patriknw · 2021-06-23T12:54:00Z

...entsourced-shopping-cart/src/main/java/com/example/shoppingcart/domain/ShoppingCartImpl.java

-              .build();
-    }
-    cart.put(item.getProductId(), item);
+  protected ShoppingCartDomain.Cart itemAdded(ShoppingCartDomain.ItemAdded itemAdded, ShoppingCartDomain.Cart currentState) {


The order of the parameters is something we need to decide. Now I use event/command as the first parameter, but it might read better with state first. We have the state first in Akka.

Yes, I think it reads better with state is first. In my brain I read it as: given this State, what should happen if we apply this Command.

Which also reads as, given this State what is the command handler (Cmd => Effect) we have for it.

Same reasoning applies event handlers.

So, yeah. State first all the way down.

State first would probably read better. JS SDK has command, state, context. Probably so that handlers can just have the command if current state is not needed.

I have changed the order, state first: 9017436

patriknw · 2021-06-23T12:56:38Z

...rced-shopping-cart/src/main/java/com/example/shoppingcart/domain/ShoppingCartInterface2.java

+            ShoppingCartApi.RemoveLineItem command,
+            ShoppingCartDomain.Cart currentState,
+            EventSourcedEntityEffect.Builder<Empty, ShoppingCartDomain.Cart> effectBuilder,
+            CommandContext ctx);


The CommandContext will rarely be needed so can we remove that parameter somehow?

Combine with with the effectBuilder?

Use a ThreadLocal accessor instead of a method parameter? CommandContext.currentContext().

what will be left in the command context?

Otherwise, we could have a CallContext<State> that we use to get command context and build the reply.

(CallContext sucks, I just need to pick a name.)

Combining into one context / effect builder would be good. And a simpler name too. Does look too heavy currently.

I have included the effect builder in the CommandContext: fc680a3

👍🏼 looks better behind the CommandContext.

Small thing, but I wonder if it would be cleaner as just effects rather than effectBuilder:

return context.effects() .emitEvent(event) .thenReply(newState -> Empty.getDefaultInstance());

but maybe it's clearer to say that it's a builder explicitly.

I like the suggestion of context.effects().

On that topic. Effect is typically well know terminology for those that know some functional programming, but is it known for the target Java developers? Do we have any alternative naming suggestion?

Action? but that would be confusing since we already have that for Action components.

Just looking at the english dictionary for Effect, it might be just fine.

I think Effect is fine, and could be useful having it aligned with Akka.

Agree that Action would be confusing. Alternative could be something like Operation.

patriknw · 2021-06-23T12:58:48Z

...entsourced-shopping-cart/src/main/java/com/example/shoppingcart/domain/ShoppingCartImpl.java

  @SuppressWarnings("unused")
  private final String entityId;

-  private final Map<String, ShoppingCartApi.LineItem> cart = new LinkedHashMap<>();
-
  public ShoppingCartImpl(@EntityId String entityId) {


I think we should move to a factory function in the registration of the components instead of register them by class. There can be other shared things that should be possible to "inject" into the component when creating it.

Absolutely, one thing that we might one to inject are clients to other services (local component or any other service)

patriknw · 2021-06-23T13:01:28Z

sdk/src/main/java/com/akkaserverless/javasdk/eventsourcedentity/EventSourcedEntityEffect.java

+  public static class Builder<T, S> {
+    private Builder() {}
+
+    public Builder2<T, S> emitEvent(Object event) {


Object here is not great, but there is no interface that all events would implement.

A marker interface here?

it's typically the generated proto messages that are the events

Ah yes, thanks.

patriknw · 2021-06-23T13:02:13Z

sdk/src/main/java/com/akkaserverless/javasdk/eventsourcedentity/EventSourcedEntityEffect.java

+     * @param payload The payload of the reply.
+     * @return A message reply.
+     */
+    public MessageReply<T> message(T payload) { // FIXME rename to reply? Note thenReply


yes, this should be renamed to reply(T message)

octonato

I did a quick pass. I will checkout and look more in detail later.

octonato · 2021-06-23T14:10:09Z

...entsourced-shopping-cart/src/main/java/com/example/shoppingcart/domain/ShoppingCartImpl.java

-              .build();
-    }
-    cart.put(item.getProductId(), item);
+  protected ShoppingCartDomain.Cart itemAdded(ShoppingCartDomain.ItemAdded itemAdded, ShoppingCartDomain.Cart currentState) {


Yes, I think it reads better with state is first. In my brain I read it as: given this State, what should happen if we apply this Command.

Which also reads as, given this State what is the command handler (Cmd => Effect) we have for it.

Same reasoning applies event handlers.

So, yeah. State first all the way down.

octonato · 2021-06-23T14:15:18Z

...entsourced-shopping-cart/src/main/java/com/example/shoppingcart/domain/ShoppingCartImpl.java

+          EventSourcedEntityEffect.Builder<Empty, ShoppingCartDomain.Cart> effectBuilder,
+          CommandContext ctx) {


EventSourcedEntityEffect.Builder + CommandContext is looking heavy.

Not fully clear why we need it. I have to checkout the code and feel it under my fingers.

In any case, worth experiment more to reduce it.

octonato · 2021-06-23T14:18:42Z

...rced-shopping-cart/src/main/java/com/example/shoppingcart/domain/ShoppingCartInterface2.java

+            ShoppingCartApi.RemoveLineItem command,
+            ShoppingCartDomain.Cart currentState,
+            EventSourcedEntityEffect.Builder<Empty, ShoppingCartDomain.Cart> effectBuilder,
+            CommandContext ctx);


what will be left in the command context?

Otherwise, we could have a CallContext<State> that we use to get command context and build the reply.

(CallContext sucks, I just need to pick a name.)

patriknw · 2021-06-29T16:42:25Z

...entsourced-shopping-cart/src/main/java/com/example/shoppingcart/domain/ShoppingCartImpl.java

-        .addAllItems(cart.values().stream().map(this::convert).collect(Collectors.toList()))
-        .build();
+  protected ShoppingCartDomain.Cart emptyState() {
+    return ShoppingCartDomain.Cart.getDefaultInstance();


If we have emptyState we have to include make that accessible from the SDK also, not only in generated code like this. We could have an interface or abstract class that the impl has to implement.

patriknw · 2021-06-29T16:44:39Z

...entsourced-shopping-cart/src/main/java/com/example/shoppingcart/domain/ShoppingCartImpl.java

+import java.util.Comparator;
+import java.util.List;
+import java.util.Optional;
+import java.util.function.Predicate;
 import java.util.stream.Collectors;

 @EventSourcedEntity(entityType = "eventsourced-shopping-cart")


Where should the entityType be defined? I think it belongs to the registration, similar to how the viewId is defined in the registration of views.

In the registration sounds good. There may be an option for that already? (I remember this being discussed before)

since we are touching all this, it might be good to rename this to entityTypeHint. Maybe on a follow-up PR.

patriknw · 2021-06-29T16:51:31Z

sdk/src/main/java/com/akkaserverless/javasdk/eventsourcedentity/EventSourcedEntityEffect.java

+     * @param description The description of the failure.
+     * @return A failure reply.
+     */
+    public FailureReply<T> failure(String description) {


Shall we rename this to error? https://www.reactivemanifesto.org/glossary#Failure

public ErrorReply<T> error(String description)

Makes sense

error it is: b5cea76

octonato

Looking good @patriknw

I drop some new ideas about the effect builders and commands.

octonato · 2021-06-30T12:14:29Z

...entsourced-shopping-cart/src/main/java/com/example/shoppingcart/domain/ShoppingCartImpl.java

+import java.util.Comparator;
+import java.util.List;
+import java.util.Optional;
+import java.util.function.Predicate;
 import java.util.stream.Collectors;

 @EventSourcedEntity(entityType = "eventsourced-shopping-cart")


since we are touching all this, it might be good to rename this to entityTypeHint. Maybe on a follow-up PR.

octonato · 2021-06-30T12:15:22Z

...entsourced-shopping-cart/src/main/java/com/example/shoppingcart/domain/ShoppingCartImpl.java

  @SuppressWarnings("unused")
  private final String entityId;

-  private final Map<String, ShoppingCartApi.LineItem> cart = new LinkedHashMap<>();
-
  public ShoppingCartImpl(@EntityId String entityId) {


Absolutely, one thing that we might one to inject are clients to other services (local component or any other service)

octonato · 2021-06-30T12:34:46Z

...entsourced-shopping-cart/src/main/java/com/example/shoppingcart/domain/ShoppingCartImpl.java

+
+    return context.effectBuilder()
+            .emitEvent(event)
+            .thenReply(newState -> Empty.getDefaultInstance());


This is looking much better.

I wonder what we could do to remove the context at all.

For instance, in Akka Typed, the entity needs to implement a given type. Could we do the same here?

abstract class ShoppingCartInterface2 implements EventSourcedEntity<Cart> class ShoppingCartImpl extends ShoppingCartInterface2

EventSourcedEntity would have the effect builders already typed on Cart and will also have the commandHandler and eventHandler entry points to be implemented.

ShoppingCartInterface2 would implement commandHandler and eventHandler and do the dispatch to the methods the user needs to implement. And we make handler final in the interface. The user only touch the method to dispatch to.

For the return type, I don't think the builder needs to have a type argument, because the compiler will force the user to finish it with the right type, ie: .thenReply(newState -> "I'm done"); won't compile.

We are left with the data that is provide by the context, command name, command id and seqNum (for event sourcing).

Would that be a good idea to wrap the command instead?

class ShoppingCartImpl extends implements EventSourcedEntity<Cart> { public Effect<Empty> addItem( ShoppingCartDomain.Cart currentState, Command<ShoppingCartApi.AddLineItem> input) { // use wrap to access command and other call related info input.command() // returns ShoppingCartApi.AddLineItem input.commandName() input.commandId() return effectBuilder() // already typed on Cart because of interface .emitEvent(event) // compile enforces return type .thenReply(newState -> Empty.getDefaultInstance()); } }

I think we need the implements EventSourcedEntity<Cart> anyway, for other reasons. I think I mentioned that in relation to the emptyState.

I like the idea of having the effectBuilder() (or effects() as was suggested elsewhere) in that base interface. I have to try if that really works with the reply type as you say.

I'm skeptical to the Command wrapping. One less parameter, but it becomes more indirection for the most common usage, which is accessing the command message. The context parameter can often be ignored.

Having the interface typed looks good, and for empty / initial state, and with the effects builder in the base rather than context.

I'm skeptical to the Command wrapping. One less parameter, but it becomes more indirection for the most common usage, which is accessing the command message. The context parameter can often be ignored.

Yeah, you may be right. I just dislike having params that are not used. Specially if they are often ignored. They are always available and often ignored. Just noise.

I wish we didn't have that at all. Do we really need a commandName if we have already the command?

And what is a commandId exactly?
If that's something generated by the framework, why do I need such a thing in my domain? And if it's something that I need in my domain than that should be a field in my command.

I don't think we will be able to fully get rid of the context. There are several metadata related things that should be accessible, such as

CommandContext.commandName CommandContext.commandId MetadataContext.metadata EntityContext.entityId Context.serviceCallFactory EventContext.sequenceNumber ActionContext.eventSubject UpdateHandlerContext.eventSubject UpdateHandlerContext.commandName ViewContext.viewId

Let me try the reply type first...

I tried with the base class and the effects builder from there. It works great. The part I was worried about was that type inference wouldn't work with the lambda in .thenReply(newState -> Empty.getDefaultInstance()). No problem, and the compiler error message is even good:

ShoppingCartImpl.java:[66,23] incompatible types: inference variable T has incompatible bounds [ERROR] equality constraints: com.google.protobuf.Empty [ERROR] lower bounds: java.lang.String

I tried with both Java 8 and 11.

Ok, let's go by pieces.

I believe that some of then can be a constructor parameter or deemed obsolete (or not crucial). We will see..

Moved to the base class approach: 23c7fb9
Looking good.

I'll give the context some more thought. We could always give access to it via the base class instead of a parameter.

I removed the context parameter, and it's accessed via the base class instead. Some runtime caveats but that is worth it. 05239d2

octonato · 2021-06-30T12:40:26Z

samples/valueentity-counter/src/main/java/com/example/domain/CounterImpl.java

-        return Empty.getDefaultInstance();
+        return context.effectBuilder()
+                .updateState(newState) // <5>
+                .thenReply(Empty.getDefaultInstance()); // FIXME add convenience shortcut for reply Empty?


Yep, I was having the same thought.

We should consider is if it realistic to think that one day we will support other formats than protobuf. If not (most likely not), it's fine to bound ourselves to Empty.

An alternative is to have our own type (eg: Done) to replace Empty. And then promote the idea that if you just want to return an ack, you can use Done and the builder has a convenience for it.

Anyway, not a big deal if we are sure about having a long term relationship with proto's Empty

We already support json. I agree that this is just a convenience that can be added if needed.

Rather than tying this to protobuf Empty specifically, it could be nicer if it returned a default instance of whatever the gRPC service method's return type is. I think that's supported in the JS SDK. Not sure how easy with the new approach, but possible if it has a handle on the method descriptor?

That would be interesting, but I don't know if I understand what you mean with a handle on the method descriptor.

If a method returns Foo, we need to return an Effect<Foo>. If the builder is not already typed on Foo we will need something like: replyWith(Foo.class) and later behind the scenes build a Foo.getDefaultInstance() on behalf of the user.

Between replyWith(Foo.class) and thenReply(Foo.getDefaultInstance()), we save some characters but we increase API surface for something that is not so hard for the users to do.

I think it would work with return effects().replyWithEmpty(). The Foo can be inferred from the return type of the method. We would create an EmptyReply<Foo>, which we will translate to the Foo.getDefaultInstance().

and Foo.getDefaultInstance() would be constructed via the proto method from the gRPC service method that Peter mentioned.

Yes, I was thinking of just replyWithEmpty() or replyWithDefault() or similar and the actual default instance created using the gRPC method descriptor.

octonato · 2021-06-30T12:41:53Z

sdk/src/main/java/com/akkaserverless/javasdk/SideEffect.java

+import com.akkaserverless.javasdk.impl.reply.EffectImpl;
+
+/** A side effect. */
+public interface SideEffect {


👍 Good to call it by its name!

patriknw · 2021-07-01T12:19:31Z

...entsourced-shopping-cart/src/main/java/com/example/shoppingcart/domain/ShoppingCartImpl.java

  }

  @Override
-  public void handleSnapshot(ShoppingCartDomain.Cart cart) {


One thing we can consider adding would be conversion methods to/from storage representation, so the business logic in command and event handlers can use a more convenient state representation than the proto objects (or json). That class would be somewhat difficult to define in the proto codegen options though, unless it's defined by it's fully qualified java class name.

Yeah, working with protobuf objects directly is more awkward. With going to and from builders, changing anything deeper. Would be interesting to see what could be improved, whether that's conversions, or wrappers that make it easier to mutate, in the way lenses are used for this.

created issue #102 for this idea

raboof · 2021-07-01T12:37:24Z

sdk/src/main/java/com/akkaserverless/javasdk/eventsourcedentity/EventSourcedEntityBase.java

+   *
+   * <p><code>null</code> is an allowed value.
+   */
+  protected abstract S emptyState();


I wonder if we should pass the entityId to the emptyState?

entityId will be passed to the factory lambda (constructor) via the CreationContext, so it's possible to keep that in an instance variable and accessed here

BREAKING CHANGE: many things renamed and replaced * rename Effect to SideEffect * add Effect and ValueEntityEffect * add state mgmt in ValueEntityEffect * builder for ValueEntityEffect * deprecate context.fail * add EventSourcedEffect and managed state * remove CommandNotImplementedException * change order parameters, currentState first * move low level api to separate package * to get it out of the way, not intended for end-users * might be removed? * also moved the registration methods for low level api in AkkaServerless to separate .lowLevel() class * rename message to reply * add base class and effects via the base class * remove context parameter, accessed via base class instead * emptyState instead of Optional for ValueEntity also * rename failure to error * exclude samples from build

patriknw · 2021-07-01T14:42:56Z

We are starting a feature branch sdk-codegen-dev and this PR is now squashed and targeting that.

patriknw · 2021-07-01T16:00:04Z

Merging into feature branch

* feat!: Spike of new Java SDK API BREAKING CHANGE: many things renamed and replaced * rename Effect to SideEffect * add Effect and ValueEntityEffect * add state mgmt in ValueEntityEffect * builder for ValueEntityEffect * deprecate context.fail * add EventSourcedEffect and managed state * remove CommandNotImplementedException * change order parameters, currentState first * move low level api to separate package * to get it out of the way, not intended for end-users * might be removed? * also moved the registration methods for low level api in AkkaServerless to separate .lowLevel() class * rename message to reply * add base class and effects via the base class * remove context parameter, accessed via base class instead * emptyState instead of Optional for ValueEntity also * rename failure to error * exclude samples from build * exclude mvn test and integration tests

patriknw commented Jun 23, 2021

View reviewed changes

octonato reviewed Jun 23, 2021

View reviewed changes

patriknw commented Jun 29, 2021

View reviewed changes

raboof mentioned this pull request Jun 30, 2021

Replace reflection with generated code #52

Closed

20 tasks

octonato reviewed Jun 30, 2021

View reviewed changes

patriknw commented Jul 1, 2021

View reviewed changes

raboof reviewed Jul 1, 2021

View reviewed changes

patriknw force-pushed the wip-50-effect-patriknw branch from b5cea76 to 5e170c3 Compare July 1, 2021 14:41

patriknw changed the base branch from main to sdk-codegen-dev July 1, 2021 14:41

patriknw marked this pull request as ready for review July 1, 2021 14:42

patriknw added 2 commits July 1, 2021 16:49

exclude mvn test

18ddc5c

skip integration tests

383bc08

patriknw force-pushed the wip-50-effect-patriknw branch from 54ec981 to 383bc08 Compare July 1, 2021 15:43

raboof mentioned this pull request Jul 1, 2021

Java - register components in the impl classes #84

Closed

patriknw merged commit 5b4fe30 into sdk-codegen-dev Jul 1, 2021

patriknw deleted the wip-50-effect-patriknw branch July 1, 2021 16:00

patriknw mentioned this pull request Jul 5, 2021

Transformation to/from snapshot representation #102

Open

johanandren mentioned this pull request Jul 12, 2021

Generate event sourced effects base class and impl #132

Merged

		EventSourcedEntityEffect.Builder<Empty, ShoppingCartDomain.Cart> effectBuilder,
		CommandContext ctx) {

feat: Spike of API changes, #50 #62

feat: Spike of API changes, #50 #62

Conversation

patriknw commented Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octonato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octonato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octonato Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octonato Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patriknw commented Jul 1, 2021

patriknw commented Jul 1, 2021

patriknw commented Jun 23, 2021 •

edited

Loading

octonato Jul 1, 2021 •

edited

Loading

octonato Jul 1, 2021 •

edited

Loading