[service-protocol] Add support for suspension message V7#4601
Open
muhamadazmy wants to merge 2 commits intorestatedev:mainfrom
Open
[service-protocol] Add support for suspension message V7#4601muhamadazmy wants to merge 2 commits intorestatedev:mainfrom
muhamadazmy wants to merge 2 commits intorestatedev:mainfrom
Conversation
muhamadazmy
force-pushed
the
pr4601
branch
2 times, most recently
from
0a01200 to
66d91b0
Compare
muhamadazmy
force-pushed
the
pr4601
branch
4 times, most recently
from
6dba403 to
8e71340
Compare
Contributor
slinkydeveloper
left a comment
slinkydeveloper
left a comment
There was a problem hiding this comment.
followup here is to enable via env the new protocol version negotiation in types service_protocol.rs, like was done back in this PR 2ce3607#diff-6442aa1f30aedc9c7fc4b6536f57b25d548f18508fc1e27195fe53eb001c8927R48
This way we can test the handling of the message already.
muhamadazmy
changed the title
muhamadazmy
force-pushed
the
pr4601
branch
2 times, most recently
from
1249780 to
acb54e7
Compare
muhamadazmy
changed the title
muhamadazmy
changed the title
muhamadazmy
force-pushed
the
pr4601
branch
2 times, most recently
from
3307bc9 to
477efa7
Compare
slinkydeveloper
requested changes
Apr 20, 2026
| } | ||
| InvocationTaskOutputInner::SuspendedV2(notification_ids) => { | ||
| self.handle_invocation_task_suspended_v2(partition, invocation_id, notification_ids).await | ||
| InvocationTaskOutputInner::SuspendedV2(combinator) => { |
Contributor
There was a problem hiding this comment.
here and everywhere you use the word combinator for future, replace it with future :)
| partition: PartitionLeaderEpoch, | ||
| invocation_id: InvocationId, | ||
| waiting_for_notifications: HashSet<NotificationId>, | ||
| combinator: UnresolvedFuture, |
| // service protocol messages. Otherwise, crates depending only on this feature fail clippy. | ||
| #[allow(dead_code)] | ||
| mod proto { | ||
| pub mod proto { |
| // If set, ignore the awaiting_for_* notifications above (17, 23, and 24) | ||
| // the awaiting_for_* are written as a flattened version of awaiting_on | ||
| // for Restate <= v1.7 | ||
| optional dev.restate.service.protocol.Future awaiting_on = 32; |
Contributor
There was a problem hiding this comment.
optional is not needed here, it's ignored for non-primitive fields.
slinkydeveloper
requested changes
Apr 21, 2026
muhamadazmy
force-pushed
the
pr4601
branch
4 times, most recently
from
e41bdbf to
418ba05
Compare
slinkydeveloper
requested changes
Apr 21, 2026
Contributor
slinkydeveloper
left a comment
slinkydeveloper
left a comment
There was a problem hiding this comment.
TODOs as followup to complete this:
- we need to make sure both sys_invocation_status and sys_invocation_state propagate the new unresolved future info.
| Suspended { | ||
| metadata: InFlightInvocationMetadata, | ||
| waiting_for_notifications: HashSet<NotificationId>, | ||
| awaiting_on: UnresolvedFuture, |
slinkydeveloper
requested changes
Apr 21, 2026
muhamadazmy
force-pushed
the
pr4601
branch
2 times, most recently
from
ab5f4ae to
807d98b
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
slinkydeveloper
left a comment
slinkydeveloper
left a comment
There was a problem hiding this comment.
another thing that we're missing is normalization of the future before proposing it in the invoker effects. we can assume the sdk behaves fine, but we must solve this before release to be defensive against sdk bugs.
muhamadazmy
force-pushed
the
pr4601
branch
6 times, most recently
from
0acf62d to
2e9934f
Compare
…e/decode Summary: This PR allows the messages (MessageType) to know the current service-protocol-version and then decide how to decode/encode to work the support version for the deployment. Note: For opaque messages, these are written as is to bifrost, and hence can't be verified against the select protocol-version.
Summary: This adds support to handle which suspension message to decode based on the protocol version. It also introduce the Suspension message V7 based on the new protocol.proto schema Update the invocation Suspended state to store and handle the awaiting on field
Comment on lines
+49
to
+53
| impl Default for UnresolvedFuture { | ||
| fn default() -> Self { | ||
| Self::Unknown(Vec::default()) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
imo you should remove this too, as this is at all effects an invalid future.
| // Should be treated as FirstCompleted, | ||
| // Used with Suspension V2 to indicate that | ||
| // the sdk did not provide a combinator kind | ||
| #[default] |
Contributor
There was a problem hiding this comment.
let's remove this default too, i think it doesn't make a lot of sense
Comment on lines
+25
to
+26
| // Used with Suspension V2 to indicate that | ||
| // the sdk did not provide a combinator kind |
Contributor
There was a problem hiding this comment.
yes, but this is also used by the sdk too sometimes.
Contributor
There was a problem hiding this comment.
the Unknown has a subtle semantic difference with FirstCompleted:
FirstCompletedcompletes with success if the first completed child future was successful, or completes with failure if the first completed child future failed.- For
Unknown, if the first completed child future succeeds, we can't tell ifUnknownitself will be marked as success or failure. The SDK can flip the result from success to failure, or viceversa.
| self | ||
| } | ||
|
|
||
| pub fn build(self) -> UnresolvedFuture { |
Contributor
There was a problem hiding this comment.
shall we fail if there's no childs here?
| } | ||
| } | ||
|
|
||
| pub struct UnresolvedFutureBuilder { |
Contributor
There was a problem hiding this comment.
given you use this only when doing tryfrom/into for storage protobuf conversions, maybe just move it there to keep this file slimmer?
Contributor
There was a problem hiding this comment.
in tests you made those factory methods, you can just use those 🤷
Contributor
Author
There was a problem hiding this comment.
The tryfrom/into implementation is in 2 different palce (one for the protocol and the other for the storage) so it's better to have it in a common place
| pub enum RawNotificationResultVariant { | ||
| /// `Unknown` indicates no result is stored, which is the case | ||
| /// for notifications created before Restate v1.7. | ||
| #[default] |
Contributor
There was a problem hiding this comment.
Not a big fan of having these defaults, especially for these unknown situations, they're a recipe for misuse :D
Contributor
Author
There was a problem hiding this comment.
I think this one is useful with serde. I think we should keep it. this way older notification entries with no result is set to unknown
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[service-protocol] Add support for suspension message V7
Summary:
This adds support to handle which suspension message to decode
based on the protocol version. It also introduce the Suspension message V7
based on the new protocol.proto schema
Update the invocation Suspended state to store and handle
the awaiting on field
Stack created with Sapling. Best reviewed with ReviewStack.