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

Effect API+impl for actions #258

Merged
merged 7 commits into from
Sep 2, 2021
Merged

Conversation

johanandren
Copy link
Contributor

@johanandren johanandren commented Aug 31, 2021

Refs #52

Effects API for actions and implementation plus codegen.

Old Reply type left in place even though Actions does not use it anymore since replicated entities still does.

@johanandren johanandren marked this pull request as ready for review September 1, 2021 13:40
@johanandren
Copy link
Contributor Author

Done for final review

Copy link
Member

@octonato octonato left a comment

Choose a reason for hiding this comment

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

LGTM

There are a few FIXME left. Are you planning to create issues and address them on upcoming PRs or do you want to include here?

Comment on lines +118 to +125
/**
* Create a reply from an async operation result returning an effect.
*
* @param futureEffect The future effect to reply with.
* @return A reply, the actual type depends on the nested Effect.
* @param <S> The type of the message that must be returned by this call.
*/
<S> Effect<S> asyncEffect(CompletionStage<Effect<S>> futureEffect);
Copy link
Member

Choose a reason for hiding this comment

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

Not clear to me in which case we would need this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look at the TCK Action. I thought it felt superficial at first but I think it makes sense that any effect and its side effects, not only messages, can be created as a consequence of a future completing.

Comment on lines 51 to 52
// FIXME async + closing over stateful action context will be a pain for users
ActionContext actionContext = actionContext();
Copy link
Member

Choose a reason for hiding this comment

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

that will be a concern indeed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johanandren
Copy link
Contributor Author

The two FIXMEs in ActionsImpl I should do something about, let's create an issue for closing over the context and solve separately.

Copy link
Member

@octonato octonato left a comment

Choose a reason for hiding this comment

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

re-approved

Ready for merge?

@johanandren johanandren merged commit c7134b3 into sdk-codegen-dev Sep 2, 2021
@johanandren johanandren deleted the wip-action-effect-impl branch September 2, 2021 12:49
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.

None yet

3 participants