-
Notifications
You must be signed in to change notification settings - Fork 13
feat: atomic combinator step #59
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
Conversation
| ControlFlow::Break(_) => return ControlFlow::Break(initial), | ||
| ControlFlow::Fail(error) => return ControlFlow::Fail(error), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intuition is that those two variants should return ControlFlow::Ok(initial). Though this is a matter of the intention behind this step.
| let result = OneStep::<P>::new(composite).run().await?; | ||
| assert!(matches!(result, ControlFlow::Fail(_))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should ControlFlow::Fail in Atomic when one of its sub-steps fail or should we return ControlFlow::Ok(_) of the initial payload?
My thoughts are that if Atomic propagates ControlFlow::Fail and ControlFlow::Break then it is no different than pipeline.step(S1).step(S2).step(S3).
As a user of this API I would imagine Atomic enabling behavior that is not available out of the box in pipeline syntax and behave somehow similar to CAS semantics in atomic operations:
- Either all sub-steps succeed with
ControlFlow::Ok(_), in which case the resulting payload would be equivalent to fold. - Or if any of the sub-steps returns non-
ControlFlow::Ok(_), then the original input payload is returned asControlFlow::Ok(_). - I think that people using
atomic!()already expect some of the steps to fail but they don't necessarily want those steps failures to propagate to pipeline failures.
What are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this makes sense, we should go with what you described. I didn't think through the whole logic of Atomic too much at first
|
I will close this and move it directly to #58 |
Stacked on #58
This PR introduce a first
atomiccombinator step where all steps must succeed (ControlFlow::Ok) in order to mutate payload.It uses the
CombinatorStepwith a newAtomicMode, and a macro that allows to quickly define atomic combinator steps like: