Flatten effects #1837
Flatten effects #1837
Conversation
slinkydeveloper
force-pushed
the
flatten-effects
branch
from
14a2236 to
2f64c22
Compare
There was a problem hiding this comment.
Thanks for unifying the EffectInterpreter and the CommandInterpreter @slinkydeveloper. The changes look good to me. I only had a few minor comments/questions. I think it would be great to unify the different StateMachine impl blocks that are spread across a few files now as a follow-up. Apart from this, +1 for merging.
| pub(crate) struct EffectInterpreter<Codec> { | ||
| _codec: PhantomData<Codec>, | ||
| } | ||
| impl<Codec: RawEntryCodec> StateMachine<Codec> { |
There was a problem hiding this comment.
I understand that this is a temporary state and thus this comment is for when we move the code: Let's not split the impl block across multiple files. In some parts of our code we did it like this and I always find it very surprising finding some more code in some seemingly unrelated files.
There was a problem hiding this comment.
I'm gonna merge this all together in this PR now and merge.
| Ok(()) | ||
| } | ||
|
|
||
| #[test(tokio::test(flavor = "multi_thread", worker_threads = 2))] |
There was a problem hiding this comment.
Is there a specific reason why the number of worker threads is 2 and we are not using the current thread runtime?
There was a problem hiding this comment.
IDK about this, this change was brought in by @AhmedSoliman, I'm simply following the way the other tests were changed.
There was a problem hiding this comment.
Not needed anymore. There was a time during a transition when that was required. Feel free to change it back if needed.
| } | ||
|
|
||
| #[test(tokio::test)] | ||
| async fn send_response_using_invocation_id() { |
There was a problem hiding this comment.
Which test covers this test case?
There was a problem hiding this comment.
Basically every test where we send completions, which is a good part of the other tests...
…the Command interpreter and squashes together apply and effect interpret. on_apply is implemented in the next commits.
… flattening of the debug/trace operation for each effect + the actual interpretation (which was previously in interpret_effect). Some of those methods can be further simplified improved, but for the sake of simplicity to review the PR I tried to keep this refactoring as mechanical as possible, without any business logic changes. Over the next PRs I'll start moving some of the business logic within the rest of the state machine.
…e do_ methods directly. As the previous commit, some of the business logic can be further simplified/improved, but for the sake of simplicity to review the PR I tried to keep this refactoring as mechanical as possible, without any business logic changes. The only bit that was changes is the code are the loops in kill_child_invocations and cancel_journal_leaves, as i can't pin anymore the stream b/c of the mutable refs to ctx.storage
The only test that was skipped is send_response_using_invocation_id, as it was redundant.
slinkydeveloper
force-pushed
the
flatten-effects
branch
from
2f64c22 to
275c1a2
Compare
Depends on #1828, replaces #1620
This PR flattens the effect logic straight into the business logic of the state machine. Check the individual commits for more info.
No business logic was changed in any way, and there's room to do more cleanup. There's also no logical reason for having separate files
command_handler.rsandeffect_interpreter.rs, I simply tried to reduce the number of changes. I think going forward we should instead reorganize the business logic on a "command basis", that is all code related to the invoke command goes inhandle_command.rsand stuff like this (and related tests too). But we can do this iteratively.As followup we can also do #276, which is probably trivial at this point.