Add Workflow service type and handler type #1506
Conversation
Also merge ServiceType and HandlerType in InvocationTargetType. This is what the ingress and invoker will use from now on.
slinkydeveloper
force-pushed
the
issues/1505
branch
from
69f2a9d
to
9a87d17
Compare
…tency. This was already the case internally in InvocationStatus, this change just modified the ServiceInvocation struct.
…be set only for workflow methods.
…r to do that I added the completion_retention to the call enriched entry.
slinkydeveloper
changed the title
There was a problem hiding this comment.
Thanks for creating this PR @slinkydeveloper. The changes look good to me. I guess the e2e are failing because of the changes to the admin API. +1 for merging after resolving my minor comments.
| oneof idempotency_key { | ||
| google.protobuf.Empty idempotency_key_none = 12; | ||
| string idempotency_key_value = 13; | ||
| } | ||
| optional string idempotency_key = 12; |
There was a problem hiding this comment.
Here we are gonna have some nice merge conflicts with one of my PRs 😅
| effects.unlock_service_id( | ||
| invocation_target | ||
| .as_keyed_service_id() | ||
| .expect("Workflow methods must have keyed service id"), | ||
| ); |
There was a problem hiding this comment.
As a side note: Introducing special cases makes it a tad bit harder to follow what is happening in the state machine. For example, I was stumbling upon this call because I thought that we will unlock the service when the inbox gets popped and there are no more invocations. However, this is not happening because we are only doing it for exclusive virtual objects.
There was a problem hiding this comment.
there is not much i can act on for this comment i think. I hope once we get rid of the effect thing, it will be much easier to track all the state machine read/writes and all those special casings, because at least i'll be able to follow the full flow.
There was a problem hiding this comment.
I think it is not necessarily the effects but the different cases that we handle slightly differently. If we could unify the cases, then things would become simpler, I believe.
There was a problem hiding this comment.
If we could unify the cases, then things would become simpler, I believe.
As much as I agree with you, right now I unfortunately don't see this happening. The different semantics we want to support are inherently different, and this will only get worse in future... I think we should instead strive to make the "capabilities" of the state machine more and more modular and mixable between each other, so that the state machine reasons more about "This invocation needs feature x, y, z" rather than "this invocation needs these behaviours because it's a workflow call". A bit in a rule engine fashion, if you get what i mean. Testability could also gain from this design approach a lot.
There was a problem hiding this comment.
as a side note, i do like a lot the approach of ECS engines, it's a bit of a mix of rule engine where the input is wildly dynamic. We don't necessarily need that, but still some rule engine of some sort could be interesting here...
…NG_IF OPTION::IS_NONE
slinkydeveloper
force-pushed
the
issues/1505
branch
from
680a7c9
to
bce19b1
Compare
Fix #1505
This PR: