[WIP] Decoupling concurrency guarantees/behavior on existing invocation id in invocation request #2393
Conversation
slinkydeveloper
changed the title
slinkydeveloper
changed the title
crates/types/src/invocation.rs
Outdated
| MaxParallelism, | ||
| /// Use the default from the target semantics | ||
| #[default] | ||
| UseTargetDefault, |
There was a problem hiding this comment.
It's unclear what the default means here. It might be worth adding a couple of words to explain that it'll use the default behavior based on the target type (virtual object, service, etc.)
Also, see my note below.
crates/types/src/invocation.rs
Outdated
| inbox_key: ByteString, | ||
| }, | ||
| /// No queueing, just execute the request | ||
| MaxParallelism, |
There was a problem hiding this comment.
If the previous one becomes Sequential, then this can be Concurrent
crates/types/src/invocation.rs
Outdated
|
|
||
| /// Behavior when sending an invocation request and the request already exists. | ||
| #[derive(Debug, Default, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] | ||
| enum BehaviorOnExistingInvocationId { |
There was a problem hiding this comment.
suggestion: IfExists might be a reasonable option if you like it. I can already imagine the call-site being invocation::IfExists::Attach
crates/types/src/invocation.rs
Outdated
| #[default] | ||
| UseTargetDefault, |
There was a problem hiding this comment.
I'm not sure I like having the default variant. It adds cognitive overhead. It sounds like "it depends". An alternative strategy is to always set it on the caller-side, I suppose.
There was a problem hiding this comment.
it is "it depends" really :D
The reason i was thinking about introducing UseTargetDefault is for backward compatibility, and eventually i guess we remove it. The idea is that next release will act on this new two types and their variants, but we never write it, while the release afterward will also write those fields.
Also i tried to make clear the "it depends" adding the method below "infer target default"
|
|
||
| /// Key to use for idempotent request. If none, this request is not idempotent, or it's a workflow call. See [`InvocationRequestHeader::is_idempotent`]. | ||
| /// This value is propagated only for observability purposes, as the invocation id is already deterministic given the invocation id. | ||
| pub idempotency_key: Option<ByteString>, |
There was a problem hiding this comment.
does that need serde(default)?
There was a problem hiding this comment.
this field is not new, i just moved it to the bottom here.
Implement PartitionKey assignment logic
Proposal for #2326