Add TCK model test for Actions #468
Conversation
nice! Best applied usage of a TCK :) |
| import java.util.concurrent.CompletionStage; | ||
|
|
||
| @Action | ||
| public class ActionTckModelBehavior { |
There was a problem hiding this comment.
Is this Entity different to the eventsourced TCK entities as it is named Behaviour(I think this is an Akka/Actor thing? I don't know Akka) as the eventsourced Entity is not named with this pattern EventSourcedTckModelEntity.
There was a problem hiding this comment.
Actions are not considered to be Entities, in the definitions we've been using. Where an Entity is primarily defined by having a distinct identity, associated with its id or key. I think the usual pattern will be to name actions with Action, and I originally had ActionTckModelAction and ActionTwoAction here, and then tried naming it with Behaviour instead to avoid the awkwardness. Happy to find a naming convention and use it here in the TCK too.
| private ServiceCall serviceTwoRequest(Context context, String id) { | ||
| return context | ||
| .serviceCallFactory() | ||
| .lookup("cloudstate.tck.model.action.ActionTwo", "Call", OtherRequest.class) |
There was a problem hiding this comment.
No issue here, but I'm surprised to see the Java gRPC service descriptors same as, the ones of the Go prototbuf plugin, do not export typesafe identifiers for services and service names.
There was a problem hiding this comment.
… there is work underway to change that for the go gRPC plugin grpc/grpc-go#3762
There was a problem hiding this comment.
The Scala service descriptors do have the identifiers. Maybe they've been added for Java as well now. Good that there will be a better API for Go.
There was a problem hiding this comment.
Akka gRPC for Java does of course generate a bunch of things, including static service names that can be used here. The method descriptors in the client all look to be private, otherwise they could be used for a lookup here. Agree that it would be good to have typesafe APIs for Cloudstate service calls, and we should otherwise look at generating these. The Java support is very reflection and runtime-based currently.
There was a problem hiding this comment.
I've updated this to use the generated service name at least, from Akka gRPC (the generation was actually disabled, as only the java protoc generation is needed).
|
|
||
| public ActionTckModelBehavior() {} | ||
|
|
||
| @CallHandler |
There was a problem hiding this comment.
Just a non-PR comment. Why did we choose CallHandler instead of CommandHandler? I don't understand why we need to change the semantics since the behavior is implicit in the type of Entity used, we shouldn't have to call it CallHandler just because it is a stateless entity. This is definitely the case with "pattern is having no pattern".
There was a problem hiding this comment.
For Actions or Functions (which are not Entities) I expect EventHandler may be the most commonly used in other similar solutions, but I think that was seen as confusing with event-sourced entities having a more specific use of event handling. In some discussions, just using Handler for actions was discussed. I'll let @jroper comment on why it's CallHandler currently. Sticking to CommandHandler for both entities and actions seems okay to me too.
There was a problem hiding this comment.
@sleipnir, do you want to create an issue for the CallHandler naming? So we track discussion there and remember to make changes.
|
As it's been a couple of weeks, I'll merge this now, and make it simpler for the next TCK extensions. Post-merge reviews still welcome of course. |
More on evolving the TCK: #316. Add simple TCK model tests for Actions.
Currently only implemented for java-support, which is set up for TCK model tests. This did uncover a couple of things in the java-support implementation: side effects were not being returned to the proxy, and there was no support for failure replies.