-
Notifications
You must be signed in to change notification settings - Fork 49
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
Move ConversionError
from restate_common
to restate_storage_api
#532
Move ConversionError
from restate_common
to restate_storage_api
#532
Conversation
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 @slinkydeveloper. The changes look good to me. I only had two minor comments.
src/storage_proto/src/lib.rs
Outdated
impl From<ConversionError> for StorageError { | ||
fn from(value: ConversionError) -> Self { | ||
StorageError::Conversion(value.into()) | ||
} | ||
} |
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.
If it weren't for this conversion, then we could avoid the dependency on restate_storage_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 don't think it makes sense to avoid that, on the contrary it makes sense that this package depends on storage_proto, because this package is part of the implementation of the storage interface. Also see #536
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.
Look at the last commit, i aligned what i did here with what's in #536
29f3981
to
ee76055
Compare
ee76055
to
a1b1f92
Compare
…on `restate_storage_api` only for the conversions.
a1b1f92
to
4c96d34
Compare
Part of #420