Add promises support #1515
Add promises support #1515
Conversation
slinkydeveloper
force-pushed
the
issues/1513
branch
from
3b7e8f2 to
e3175d2
Compare
There was a problem hiding this comment.
Thanks a lot for creating this PR @slinkydeveloper. The changes look good to me. The one question I have is about the lifecycle of promises. I couldn't find how these promises are being cleaned up. If they are kept around forever, how are we gonna make handlers work that repeatedly create promises? I guess in the first version only workflows will use this feature. After a workflow is cleaned up I assume that a new run will try to create the same promise again.
| #[derive(Debug, Clone, PartialEq)] | ||
| pub enum PromiseState { | ||
| Completed(EntryResult), | ||
| NotCompleted(Vec<JournalEntryId>), |
There was a problem hiding this comment.
What are the JournalEntryIds being used for? Maybe add a bit of documentation for this variant because it does not seem super obvious.
There was a problem hiding this comment.
Note to myself: Those are the entries of another invocation's journal of the same service instance which are awaiting the promise and want to be notified in case it completes.
| ); | ||
|
|
||
| if let Some(service_id) = | ||
| invocation_metadata.invocation_target.as_keyed_service_id() |
There was a problem hiding this comment.
Do I understand it correctly that the promise feature only allows a service instance to access promises that were created by itself? I guess if we wanted to do it generally, one would have to introduce a new message that can retrieve the result from a different partition.
There was a problem hiding this comment.
Do I understand it correctly that the promise feature only allows a service instance to access promises that were created by itself?
Yes this is how it's modeled in the service protocol entry message too (it doesn't have the service name/service key of the promise holder). The idea is that people should not expose promises to foreign services as api surface, but should rely on shared handlers for that.
| warn!( | ||
| "Trying to process entry {} for a target that has no promises", | ||
| journal_entry.header().as_entry_type() | ||
| ); |
There was a problem hiding this comment.
A bit unrelated. I am not sure about the warn level here because technically this situation can easily happen when a user writes an incorrect handler. However, this does not mean that the runtime is in anyway in danger which I would usually associate with a log message on warn level (e.g. disk space is running full, not able to connect to the cluster controller, etc.).
There was a problem hiding this comment.
A bit unrelated. I am not sure about the warn level here because technically this situation can easily happen when a user writes an incorrect handler.
This can happen only in case the SDK is buggy and exposes the promise apis to non-workflow Context, this is a similar case to creating GetState inside a regular service.
| state: PromiseState::Completed(_), | ||
| }) => { | ||
| // Conflict! | ||
| (&CONFLICT_INVOCATION_ERROR).into() |
There was a problem hiding this comment.
Out of curiosity: How will this error surface to the user? Will it be a terminal error or returned as false on the complete call?
There was a problem hiding this comment.
For now it seems reasonable to return a terminal error. There might be other situations where we could fail to complete the promise that were discussed as well, e.g. fail completing a promise if the workflow didn't started yet.
crates/worker/src/partition/state_machine/command_interpreter/mod.rs
Outdated
Show resolved
Hide resolved
This is exactly the reason why these promises will be for the foreseable future a workflow only feature, because the lifecycle question is still open for virtual objects. As soon as I rebase this PR, I'll add the command to cleanup the promises when the workflow is cleaned up (if you remember, there was a todo for that in the other workflow PR) |
slinkydeveloper
force-pushed
the
issues/1513
branch
2 times, most recently
from
f310dea to
a1da606
Compare
|
I updated this PR with the feedback + integrated with the previous workflow PR @tillrohrmann |
| pub enum PromiseState { | ||
| Completed(EntryResult), | ||
| NotCompleted( | ||
| // Journal entries listening for this promise to be completed |
There was a problem hiding this comment.
Is there a limitation for which journals/invocations can listen for a promise (invocations to the same service instance vs. an arbitrary invocation)?
There was a problem hiding this comment.
In principle not, the implementation of this table is built such that any invocation from any partition could listen to any promise. But the restriction currently exists in the service protocol entry, and transitively in the PP commands.
slinkydeveloper
force-pushed
the
issues/1513
branch
from
425c278 to
faccc9e
Compare
Fix #1513. This implements the new promises api in the service-protocol