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

Add support for Kotlin in Async #319

Merged
merged 1 commit into from
Aug 14, 2021

Conversation

Spikhalskiy
Copy link
Contributor

@Spikhalskiy Spikhalskiy commented Jan 21, 2021

Previous behavior

Previously Async didn't handle method references on Temporal stubs coming from Kotlin correctly.
As the result, method references on Temporal stubs were treated as regular functions and executed as a new child thread of the workflow instead of just inline calls to an async stub.
There is a relevant post about this issue in an unrelated discussion: #537 (comment)

#Change
temporal-kotlin module has been added that provides support for correct temporal stubs detection inside Async if the method referenced is passed from kotlin code.
After merging this PR Kotlin users will have to add io.temporal:temporal-kotlin module to the classpath or they will get an exception trying to use Async.

@CLAassistant
Copy link

CLAassistant commented Jan 21, 2021

CLA assistant check
All committers have signed the CLA.

@Spikhalskiy Spikhalskiy marked this pull request as draft January 21, 2021 07:04
@mfateev
Copy link
Member

mfateev commented Jan 22, 2021

Awesome! Thanks for driving this!
My dream is to get a full-blown Kotlin SDK implemented on top of Java SDK. Especially I'm excited to use coroutines to implement workflows.

@mfateev
Copy link
Member

mfateev commented Feb 10, 2021

Are you planning to convert this PR to "Ready to review status"?

@Spikhalskiy
Copy link
Contributor Author

@mfateev Yes. My current priority is opentracing though. After preparing opentracing PR, I will polish this PR one more time and will open it for review.

* Functions.Func} implementation
* @return promise on the return value of the asynchronous invocation of {@code func}
*/
private static <R> Promise<R> execute(boolean temporalStub, Functions.Func<R> func) {
Copy link
Member

Choose a reason for hiding this comment

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

Can temporalStub not be determined by inspecting func ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can! but the point of this function is that we expected the func already earlier and provide this as an input. It's a private function, not a part of the class API.

if (methodReferenceDisassemblyService == null) {
throw new IllegalStateException(
"Kotlin method reference is used with async. "
+ "For Temporal to correctly support async invocation kotlin method references add temporal-kotlin to classpath");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
+ "For Temporal to correctly support async invocation kotlin method references add temporal-kotlin to classpath");
+ "For Temporal to correctly support async invocation kotlin method references, add temporal-kotlin to classpath");

Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me but I can't really claim to fully track all the weird reflection stuff going on. Adding some brief docstring explainers might be nice.

@Spikhalskiy Spikhalskiy force-pushed the async-kotlin branch 14 times, most recently from 791974a to 22d83c1 Compare August 14, 2021 03:59
@Spikhalskiy Spikhalskiy force-pushed the async-kotlin branch 5 times, most recently from cc8f368 to 369c809 Compare August 14, 2021 17:22
@Spikhalskiy Spikhalskiy merged commit 759526e into temporalio:master Aug 14, 2021
@Spikhalskiy Spikhalskiy deleted the async-kotlin branch March 8, 2022 01:20
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.

4 participants