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 Workflow.newWorkflowQueue method and ZWorkflowQueue wrapper #127

Merged
merged 3 commits into from
Sep 30, 2023

Conversation

grouzen
Copy link
Contributor

@grouzen grouzen commented Sep 19, 2023

Changes being made

Add ZWorkflow.newWorkflowQueue method and ZWorkflowQueue wrapper for WorkflowQueue

Additional context

/addresses #88

import io.temporal.workflow.{QueueConsumer, WorkflowQueue}
import zio.Duration

final class ZWorkflowQueue[E](val toJava: WorkflowQueue[E]) extends ZQueueConsumer[E](toJava) with ZQueueProducer[E] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it fine for you to keep all these 3 wrappers in the same file? I'll be happy to move them into separate files if you want.

Copy link
Owner

Choose a reason for hiding this comment

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

Now, looking at the wrapper, I wonder if adding it is worth it. Do you see any possible Scala-specific extensions we can provide? I mean additional methods, supporting Scala-specific types (besides zio.Duration), etc.

Currently, this wrapper brings proper (in)variance checks and a .map method overload for Scala lambdas. What else can we do? Depending on the answer, we'd decide on what to do with those interfaces/wrappers.

Copy link
Contributor Author

@grouzen grouzen Sep 19, 2023

Choose a reason for hiding this comment

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

That's a good question. I think, it is also worth adding it because of additional null checks for some methods that return null. Also, some methods throw exceptions, and that's the question I was going to ask you. Is allowing methods to raise exceptions in a workflow code semantically correct? I mean, I think usually you want to have control over such dangerous behavior, and wrapping such methods in Try sounds good to me.

Copy link
Owner

Choose a reason for hiding this comment

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

Unfortunately, the workflow code should be built without Try, Either, or any other container. Temporal's error-handling logic can only work with Exceptions. If we wrap an error into Try, Temporal won't retry the workflow unless you get. Therefore, the existing zio-temporal functionality related to the Workflow code (such as activity invocations) doesn't wrap error-prune code but raises an exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, got it. So, deciding whether we should have a wrapper is up to you. I don't see any other benefits besides what we already mentioned.

Copy link
Owner

Choose a reason for hiding this comment

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

It sounds enough to me. We can add more methods later 😃


}

class ZQueueConsumer[E] private[zio] (toJava: QueueConsumer[E]) {
Copy link
Owner

Choose a reason for hiding this comment

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

Minor: can it be an interface as well? The ZWorkflowQueue can implement two interfaces

Copy link
Contributor Author

@grouzen grouzen Sep 19, 2023

Choose a reason for hiding this comment

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

Unfortunately, it should be a class because the original QueueConsumer.map returns QueueConsumer, which is a supertype for WorkflowQueue. That means we can't implement map method like:

def map[R](f: E => R): ZWorkflowQueue[R] =
    new ZWorkflowQueue(toJava.map(e => f(e)) // compiler error, toJava.map returns QueueConsumer but ZWorkflowQueue receives WorkflowQueue

Hope it makes sense.

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, that's strange, but we should follow it 😄

@grouzen grouzen marked this pull request as ready for review September 19, 2023 17:20
@vitaliihonta
Copy link
Owner

@grouzen, if there is any Javadoc for the Java Queue, could you copy it into our wrapper as well?
Otherwise LGTM! The only thing nice to have is usage examples. I haven't found them yet in Java SDK samples 🤔

@grouzen
Copy link
Contributor Author

grouzen commented Sep 19, 2023

@grouzen, if there is any Javadoc for the Java Queue, could you copy it into our wrapper as well? Otherwise LGTM! The only thing nice to have is usage examples. I haven't found them yet in Java SDK samples 🤔

Sure, I'll copy the docs for the methods.
That was my main struggle - no examples and docs about WorkflowQueue. I've managed to find some info on the community forum. I'll add maybe some test cases along with examples.

@codecov-commenter
Copy link

Codecov Report

Merging #127 (c3c7f4b) into main (669309f) will decrease coverage by 1.38%.
Report is 3 commits behind head on main.
The diff coverage is 7.69%.

@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
- Coverage   38.56%   37.19%   -1.38%     
==========================================
  Files         115      127      +12     
  Lines        4055     4850     +795     
==========================================
+ Hits         1564     1804     +240     
- Misses       2491     3046     +555     
Files Coverage Δ
...c/main/scala/zio/temporal/workflow/ZWorkflow.scala 39.26% <100.00%> (-3.24%) ⬇️
...n/scala/zio/temporal/workflow/ZWorkflowQueue.scala 0.00% <0.00%> (ø)

... and 39 files with indirect coverage changes

@vitaliihonta
Copy link
Owner

@grouzen I'm going to merge it into the new release. Feel free to add tests and examples in a new PR!

@vitaliihonta vitaliihonta merged commit 0bf5e61 into vitaliihonta:main Sep 30, 2023
7 checks passed
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