-
Notifications
You must be signed in to change notification settings - Fork 33
Mj/file store err flip #1074
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
Open
michaeldjeffrey
wants to merge
13
commits into
main
Choose a base branch
from
mj/file-store-err-flip
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Mj/file store err flip #1074
Conversation
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
7020c14 to
e859bbf
Compare
`TimestampDecodeError` provides more explicit errors for what type of
field we are attempting to decode, and an extra if we cannot parse the
incoming number.
`TimestampDecodeResult` is provided as a convenience for implementing
MsgTimestamp.
```
impl MsgTimeStamp<Result<DateTime<Utc>, TimestampDecodeError>> for Type {}
```
is super cumbersome to read.
This will cuase the compiler to err if you try to implement MsgDecoe for a type without also having the TryFrom impl. Before, that error would only be thrown if someone tried to call ::decode() on a msg. We also provide a custom decode error, this allows us to tell the different between prost failing to decode a protobuf vs. our own conversion code failing to transform into a domain struct. This also allows us to provide our own error for conversions instead of being tied to `file_store::Error` thta had become a grab bag of domain specific err variants.
Remove the constraint on MsgDecode implementers returning a `file_store::Error` in lieu of their own domain error. The parser will pass them back their own error if something fails, and they can be more explicit about the errors they care about. Rather than trying to decide between putting a new err variant in file-store, or using ExternalError, or putting a specific string in the wrong err type for what they actual are trying to communicate.
Use the member field type to figure out what enum we’re casting into. Provide an err variant that wraps a `prost::UnknownEnumValue` for propogating unsupported variants in TryFrom impls.
… from file-store lib code All domain specific errors are going to move into the err variant of TryFrom for the type that is being converted to.
Includes the normal `TimestampDecodeError` provided by file-store. MissingField when trying to access nested types to convert. And wrapping `prost::UnknownEnumValue` errors to better know which enum we could not map, and have a consistent way of treating enum conversions. Before we had a smattering of using `Enum::try_from(proto.enum_field)` which allowed is to catch errs. And `proto.enum_field()` using the provided getter that will panic if the value is unknown. Now we have a `prost_enum(proto.enum_field, Error::EnumField)` mapper being provided that can figure out what type we’re trying to convert into and forward any errors back to us for bubbling.
You may notice an `h3o::error::InvalidCellIndex` error in usage counts. One of the benefits for flipping error logic over to the implementer is now file-store has no more reason to know about h3o.
These 2 files are essentially the same and are good candidates for moving into networ_common in file-store-oracles. That is outside the scope of this PR.
Many of the streams in file-store are still heavily tied to `file_store::Error`. Passing reward manifest files still requires us mapping into a proper lib error so we can pass it to a lib function.
e859bbf to
b8fcfeb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Move Domain-Specific Errors from file-store to Implementers
Summary
This PR restructures error handling in the file-store library by moving domain-specific errors to the types implementing TryFrom conversions. This separation of concerns keeps file-store focused on library-level errors while allowing each domain to handle its own conversion errors appropriately.
Changes
file_store::Error Cleanup
The
file_store::Errorenum has been significantly reduced to contain only errors that can be returned from file-store library code:Errortype of their respectiveTryFromimplementationsCustom Domain Error Types
Created domain-specific error types in file-store-oracles.
Each file has it's own Error type that maps things that can fail during those types conversions.
Some common errors include
MsgDecode Trait Enhancement
The
MsgDecodetrait now includes a custom error variant:This allows implementers to return domain-specific errors from the
TryFromconversion without coupling to file-store's error types.MsgTimestamp Trait Relocation
The
MsgTimestamptrait has been removed from file-store due to Rust's orphan rules. When implementing this trait for both proto and domain types, you need to own the trait definition. The trait definition and usage examples are documented in the timestamp.rs file with guidance for implementers.Timestamp Handling Improvements
TimestampDecodetrait andTimestampDecodeErrorremain in file-store for shared timestamp logicTimestampEncodetrait added for encoding DateTime to various timestamp formatsEnum Conversion Utility
Added
prost_enumhelper function in file-store-oracles:This provides a consistent way to convert prost i32 values to enums while properly propagating errors. The mapper automatically determines the enum type from context and allows custom error handling.
FileInfoPollerParser Changes
Updated to allow user errors to pass through from TryFrom implementations, enabling better error context propagation from domain-specific conversions.
IoT Beacon Conversion
The conversion from
IotBeaconReportto a regular beacon no longer returns a Result as it's a pure transformation.Code Organization