-
Notifications
You must be signed in to change notification settings - Fork 118
[AdminAPI] Use IngestionClient for invocation and state mgmt
#3980
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
Conversation
6fbd35c to
1b6c108
Compare
3769517 to
a25372c
Compare
960bebd to
4a40737
Compare
tillrohrmann
left a comment
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 @muhamadazmy. The changes look good to me :-)
I left a comment about the removal of the "delete an invocation" API. I think it is ok to remove the API since it was deprecated in v1.4.0 but we need to make sure that it's communicated as part of the release. The Admin API version might have to be bumped if older clients could still call the old invocation deletion API.
| /// Terminate an invocation | ||
| #[openapi( | ||
| summary = "Delete an invocation", | ||
| deprecated = true, |
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 API was deprecated in 1.4.0. If we remove it with 1.6.0, then we certainly need to add a release note to make people aware. Additionally, we need to check
| V2 = 2, |
| /// Version information endpoint |
restate/cli/src/clients/admin_client.rs
Line 34 in b9b8f28
| pub const MIN_ADMIN_API_VERSION: AdminApiVersion = AdminApiVersion::V2; |
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.
cc @slinkydeveloper for double checking whether we can remove this API.
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 actually discussed this with him before I remove it. I was confused because the was 2 endpoints to delete an invocation. So he confirmed that it's no longer used (same for the UI).
Regarding the version, thank you for pointing this out. I think we should indeed set the MIN_ADMIN_API_VERSION to V3
| warn!("Could not append state patching command to Bifrost: {err}"); | ||
| MetaApiError::Internal( | ||
| "Failed sending state patching command to the cluster.".to_owned(), |
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.
Is this always true that the patch command could not be appended to Bifrost? What if the ingress is closing after having sent the command out?
| let result = state | ||
| .ingress | ||
| .ingest( | ||
| partition_key, | ||
| IngestRecord::from_parts(envelope.record_keys(), envelope), | ||
| ) | ||
| .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.
What about rolling upgrades when there are still a few nodes that don't support the PartitionLeaderService yet?
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 think we can release all the changes up to kafka ingestion. Then release admin and shuffle changes in the following release.
6dccbcf to
e58f21d
Compare
tillrohrmann
left a comment
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 updating this PR @muhamadazmy. LGTM. +1 for merging :-)
|
|
||
| /// Min/max supported admin api versions by the server | ||
| pub const MIN_ADMIN_API_VERSION: AdminApiVersion = AdminApiVersion::V2; | ||
| pub const MIN_ADMIN_API_VERSION: AdminApiVersion = AdminApiVersion::V3; |
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 add a comment why we bumped the min admin version to V3 and in which Restate version this happened.
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.
Does this mean that V2 contained the old kill/cancel invocation endpoint and V3 does not? Maybe we can document what was included/changed with AdminApiVersion::V3 for our future selves.
b26b442 to
bbbb04e
Compare
425fe18 to
0880a6a
Compare
715b672 to
be1e987
Compare
ee02391 to
bd521a9
Compare
accb0d0 to
fb4f547
Compare
- Use IngestionClient instead of bifrost to write to partitions logs - Remove deprecated `delete_invocation`
[AdminAPI] Use
IngestionClientfor invocation and state mgmtdelete_invocationStack created with Sapling. Best reviewed with ReviewStack.
Shuffler#4024IngestionClientfor invocation and state mgmt #3980