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 the ability to update the input for a WorkflowHost. #282

Merged
merged 1 commit into from
Apr 16, 2019

Conversation

zach-klippenstein
Copy link
Collaborator

Closes #247.

@zach-klippenstein zach-klippenstein requested a review from rjrjr April 13, 2019 18:32
@zach-klippenstein zach-klippenstein added this to the v0.14.0 milestone Apr 13, 2019
@zach-klippenstein
Copy link
Collaborator Author

Would it be more useful, instead of requiring an initial input immediately and having an update function, to just only accept a stream of inputs? The workflow would then just not start until the first input was emitted.

@rjrjr
Copy link
Contributor

rjrjr commented Apr 15, 2019

Would it be more useful, instead of requiring an initial input immediately and having an update function, to just only accept a stream of inputs? The workflow would then just not start until the first input was emitted.

Yes! We've been longing for that in production already.

If you're still on board with that notion I'll hold off on reviewing this for real.

@zach-klippenstein
Copy link
Collaborator Author

zach-klippenstein commented Apr 15, 2019

Totes. I'll update the PR asap.

@zach-klippenstein zach-klippenstein force-pushed the zachklipp/workflowhost-inputs branch from 6fea2ea to db24454 Compare April 15, 2019 19:38
@zach-klippenstein
Copy link
Collaborator Author

@rjrjr Updated the API. WorkflowViewModel now uses the new flatMapWorkflow operator in workflow-rx2-runtime to convert a stream of inputs into a stream of outputs. Should inputs be an Observable conveted to a Flowable with BackpressureStrategy.BUFFER internally instead?

@zach-klippenstein
Copy link
Collaborator Author

Also, maybe the stream operator is too much - could just make an extension function on Factory that takes an input stream and just returns a WorkflowHost. I just like the operator because the API surface is so tiny and it hides all the channel machinery completely.

Copy link
Contributor

@rjrjr rjrjr left a comment

Choose a reason for hiding this comment

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

Should inputs be an Observable converted to a Flowable with BackpressureStrategy.BUFFER internally instead?

You mean on WorkflowViewModel, right? Flowable certainly seems like the right thing on the runtime API. WorkflowViewModel isn't public either, though. My gut is to keep that working in terms of Flowable, and make WorkflowActivityRunner take an Observable.

Also, maybe the stream operator is too much - could just make an extension function on Factory that takes an input stream and just returns a WorkflowHost. I just like the operator because the API surface is so tiny and it hides all the channel machinery completely.

No no no, I think it's a keeper. Such a natural way to think about this.

import kotlin.DeprecationLevel.ERROR

/**
* Given a stream of [InputT] values to use as inputs for the top-level [workflow], returns a
Copy link
Contributor

Choose a reason for hiding this comment

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

This is gorgeous.

@zach-klippenstein zach-klippenstein force-pushed the zachklipp/workflowhost-inputs branch 2 times, most recently from 5cf7cb6 to 44e9d41 Compare April 16, 2019 21:16
@zach-klippenstein zach-klippenstein changed the title WIP: Add the ability to update the input for a WorkflowHost. Add the ability to update the input for a WorkflowHost. Apr 16, 2019
@zach-klippenstein zach-klippenstein marked this pull request as ready for review April 16, 2019 21:17
@zach-klippenstein zach-klippenstein requested a review from rjrjr April 16, 2019 21:17
initialSnapshot: Snapshot? = null,
dispatcher: CoroutineDispatcher = Dispatchers.Unconfined
): Flowable<Update<OutputT, RenderingT>> =
// We're ok not having a job here because the lifetime of the coroutine will be controlled by the
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This formatting is the auto-formatter being stupid.

@zach-klippenstein
Copy link
Collaborator Author

A bunch of tests broke in interesting ways because things are more asynchronous now. Added WorkflowTester.awaitFailure and .withFailure to make testing errors more reliable.

@zach-klippenstein zach-klippenstein force-pushed the zachklipp/workflowhost-inputs branch from 44e9d41 to 81aa020 Compare April 16, 2019 22:13
@zach-klippenstein zach-klippenstein force-pushed the zachklipp/workflowhost-inputs branch from 81aa020 to cf3ae7b Compare April 16, 2019 22:47
@rjrjr rjrjr merged commit 1ae90bb into master Apr 16, 2019
@zach-klippenstein zach-klippenstein added the kotlin Affects the Kotlin library. label Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kotlin Affects the Kotlin library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an input-update channel to WorkflowHost
2 participants