Split invocation status in invocation status/service status, Change keying of journal to invocation id #1169
Conversation
slinkydeveloper
force-pushed
the
issues/reorganize-status-pp
branch
2 times, most recently
from
211a3a6
to
c453646
Compare
slinkydeveloper
force-pushed
the
issues/reorganize-status-pp
branch
from
c453646
to
23e62ca
Compare
slinkydeveloper
changed the title
slinkydeveloper
force-pushed
the
issues/reorganize-status-pp
branch
3 times, most recently
from
20dbe55
to
53ae351
Compare
|
The last commit is optional, it's a refactoring i did which i figured would be useful now. |
slinkydeveloper
force-pushed
the
issues/reorganize-status-pp
branch
from
53ae351
to
670213b
Compare
There was a problem hiding this comment.
Thanks for creating this impressive piece of work @slinkydeveloper. The changes look good to me. I mainly had minor comments. The one thing that we should address before merging is making sure that the CLI still works. I think the renaming of some of the datafusion tables might cause problems for the CLI. The documentation update for the datafusion tables will be done as a follow-up, right?
|
|
||
| #[derive(Debug, Default, Clone, PartialEq)] | ||
| pub enum ServiceStatus { | ||
| Locked(InvocationUuid), |
There was a problem hiding this comment.
Should this maybe return an InvocationId?
There was a problem hiding this comment.
Storing InvocationId is redudant here, because partition key is already part of the key.
There was a problem hiding this comment.
This is the Rust type that the upper layers will interact with, right? How things are stored by storage can be different. I am mainly wondering whether we want the upper layers to know about the uuid part and then doing the right stitching with the right partition key or whether this is something that is done by the layers that return this type.
There was a problem hiding this comment.
I agree, although due to how our code is organized, this is not easy to plug in those try/from implementations. Will do this change anyway as you proposed, and perhaps revisit later how to improve those try/from impls.
crates/worker/src/partition/state_machine/command_interpreter/mod.rs
Outdated
Show resolved
Hide resolved
| @@ -27,6 +27,8 @@ pub enum InvokeInputJournal { | |||
| CachedJournal(JournalMetadata, Vec<PlainRawEntry>), | |||
| } | |||
|
|
|||
| // TODO We can remove FullInvocationId awareness from the invoker completely | |||
There was a problem hiding this comment.
Where are we gonna retrieve the ServiceId from if we don't provide the FullInvocationId?
There was a problem hiding this comment.
It can come straight from the journal reader (JournalMetadata/InvocationMetadata effectively).
| effects.drop_journal_and_pop_inbox( | ||
| FullInvocationId::with_service_id( | ||
| invocation_status | ||
| .service_id() | ||
| .expect("InvocationStatus should be available"), | ||
| invocation_id.invocation_uuid(), | ||
| ), | ||
| journal_length, | ||
| ); |
There was a problem hiding this comment.
Was it a mistake to pop the inbox before?
There was a problem hiding this comment.
Yes. inbox don't make sense for virtual journal remote context :)
|
Ah one more thing, I think we need to bump the storage format version to 2. Depending on which PR gets merged first, this might already be done via #1210. |
…eying of journal to invocation id
slinkydeveloper
force-pushed
the
issues/reorganize-status-pp
branch
from
670213b
to
17d8170
Compare
slinkydeveloper
force-pushed
the
issues/reorganize-status-pp
branch
from
62b4512
to
acf48fe
Compare
There was a problem hiding this comment.
Great work @slinkydeveloper. The changes look good to me. +1 for merging.
| FROM sys_invocation_status ss | ||
| LEFT JOIN sys_invocation_state sis ON ss.id = sis.id |
There was a problem hiding this comment.
I have to admit that I find sys_invocation_status and sys_invocation_state a bit too close in Levenshtein distance. I always find myself thinking about what each table represents. Maybe as a follow-up, can we try to find more distinct names?
There was a problem hiding this comment.
Same here, we'll have to fix this at a later point.
This PR has some followups:
FullInvocationIdin a couple of places, such as within the invoker and in theServiceInvocationResponseSink. I'll open 2 followup issues for those.InvocationIdinJournalId? Because that's what it is now.