-
Notifications
You must be signed in to change notification settings - Fork 34
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
InvocationStatus::Inboxed #1334
InvocationStatus::Inboxed #1334
Conversation
5c18189
to
2742f7c
Compare
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.
I can see this work. So from my side +1.
55fe2e6
to
cb1bcd8
Compare
cb1bcd8
to
2c2adb5
Compare
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.
Thanks for creating this PR @slinkydeveloper. The changes look good to me :-) +1 for merging.
@@ -48,11 +53,11 @@ impl SequenceNumberInboxEntry { | |||
} | |||
pub fn from_invocation( |
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.
maybe rename into invocation
?
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.
I prefer from_invocation
to stay consistent with below method from_state_mutation
.
match caller { | ||
Source::Service(caller) => { | ||
row.invoked_by("component"); | ||
row.invoked_by_component(&caller.service_id.service_name); | ||
if row.is_invoked_by_id_defined() { | ||
row.invoked_by_id(format_using(output, &caller)); | ||
} | ||
} | ||
Source::Ingress => { | ||
row.invoked_by("ingress"); | ||
} | ||
Source::Internal => { | ||
row.invoked_by("restate"); | ||
} | ||
} | ||
if row.is_trace_id_defined() { | ||
let tid = span_context.trace_id(); | ||
if tid != TraceId::INVALID { | ||
row.trace_id(format_using(output, &tid)); | ||
} | ||
} |
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.
How will removing these columns affect the CLI?
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.
The CLI won't need to query the sys_inbox table anymore now. I defer those changes to followups.
crates/worker/src/partition/state_machine/command_interpreter/mod.rs
Outdated
Show resolved
Hide resolved
crates/worker/src/partition/state_machine/effect_interpreter.rs
Outdated
Show resolved
Hide resolved
let inboxed_status = | ||
state_storage.get_invocation_status(&invocation_id).await?; |
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.
I guess this is one of the additional reads we are introducing by having the InvocationStatus::Inboxed
.
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.
this is the only one i think
Don't store the ServiceInvocation anymore in the inbox entry.
727a7d6
to
40e9dfd
Compare
Store the metadata about the inboxed invocation in the invocation status table.
This simplifies a bunch of problems, such as terminating inboxed invocations and the upcoming issue of inboxed idempoten invocations (described in #1326)