Skip to content
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

Update InvocationStatus v2 to use "iS" key prefix #1877

Merged

Conversation

pcholakov
Copy link
Contributor

@pcholakov pcholakov commented Aug 21, 2024

Let's use InvocationStatusV2 since both key prefixes will likely live on in the Restate codebase for a long time, even after is becomes officially unsupported sometime down the line.

I've also renamed the storage protobuf message NeoInvocationStatus to InvocationStatusV2 without doing a mass search-and-replace; this PR only covers the long-lived mentions of the updated V2 invocation message and keys.

@pcholakov pcholakov marked this pull request as draft August 21, 2024 11:21
@pcholakov pcholakov marked this pull request as ready for review August 21, 2024 11:56
Copy link
Contributor

@muhamadazmy muhamadazmy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Disclaimer: I definitely don't know the impact of changing the key kind from ni to iS . The only problem I can think of is backward compatibility. If ni is already used or is only recently added but not released.

@pcholakov
Copy link
Contributor Author

@muhamadazmy yes, this was added very recently indeed in #1828, and has never been a part of a tagged release.

Copy link
Contributor

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this renaming, thanks for tackling it @pcholakov !

Copy link
Contributor

@AhmedSoliman AhmedSoliman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks for initiating and addressing this @pcholakov.

@pcholakov
Copy link
Contributor Author

Thanks folks!

@pcholakov pcholakov merged commit b0d5870 into restatedev:main Aug 22, 2024
9 checks passed
@pcholakov pcholakov deleted the update-invocationstatus-v2-key-prefix branch September 21, 2024 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants