-
Notifications
You must be signed in to change notification settings - Fork 111
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
errors: remove ParseError #1054
errors: remove ParseError #1054
Conversation
See the following report for details: cargo semver-checks output
|
ref: #1056 |
ccb7c3d
to
8855a32
Compare
8855a32
to
e385c26
Compare
Rebased on #1067 |
e385c26
to
c56ed97
Compare
c56ed97
to
677b1cc
Compare
ed96da7
to
5946397
Compare
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.
Mostly good direction, mainly nits.
208bf72
to
a983be6
Compare
Introduced an error type returned by QueryParameters::serialize.
Introduced an error type to give more context for errors returned by Query::serialize.
Introduced an error type to give more context for errors returned by Prepare::serialize.
Introduced an error type to give more context for errors returned by Batch::serialize.
Introduced an error type to give more context for errors returned by AuthResponse::serialize.
Introduced an error type to give more context for errors returned by Register::serialize.
Introduced an error type to give more context for errors returned by Startup::serialize.
Introduced an error type to be returned by an implementations of SerializableRequest trait.
lz4 compression cannot fail in our case, and so we never return the error variant with lz4 compression failure.
The most important thing is that we narrow the return error type of SerializedRequest::make from FrameError to CqlRequestSerializationError. We remove both `CqlRequestSerializationError` and `SnapCompressError` variants from FrameError. `SnapCompressError` variant is now moved to `CqlRequestSerializationError`.
Replaced FrameError::Parse with multiple variants which provide more context.
Added more context to std::io::Errors in FrameDeserializationError.
This is a new error type returned from frame::read_response_frame. We extracted FrameError variants that were originally constructed there, and moved them to new error type.
lz4_flex crate is unstable - we need to replace the direct usage of this type with `Arc<dyn Error>`.
We ultimately want to include FrameBodyExtensionsParseError in QueryError, which needs to be clonable.
Until now, the FrameErrors were stringified. This commit changes that.
This error type is not constructed anywhere. It can be removed.
Because of the previous changes, there were some unused ParseError variants which we can get rid of. This cleans up a bit, and helps us to see what variants are actually used and need to be removed later.
ParseError was abused in testing as well. DeserializableRequest is a trait mainly used by `scylla-proxy`. Previously, it returned `ParseError`. This commit introduces a new error type, designed for this specific case. It does not provide much context, as it is not intended to be used by the users.
This function is not used anywhere. It contains the last usage of `std::io::Error` -> `ParseError` mapping. Let's get rid of that.
It is not used anymore. We got rid of yet another variant from ParseError.
We removed last usages of this conversion from scylla-proxy. This allows us to remove the From impl.
We remove last usage (an impl) of this variant. This variant was the most awful, since it stringified many errors. We fortunately can remove it now.
Removed last usages of ParseError from scylla-proxy. This allows us to remove LowLevelDeserializationError variant from ParseError.
4b840c9
to
62c661b
Compare
v1.2: addressed @Lorak-mmk 's comments:
|
@Lorak-mmk I also checked if we use any of these libraries' types in other parts of public API, but didn't find anything. You can double check that, if possible. |
@Lorak-mmk Rerun the CI, please. A known flaky test
|
Can we merge? |
Ref: #519
This PR solves multiple issues:
No context for request serialization
I introduced a
CqlRequestSerializationError
, with the corresponding variants for each request type. Previously, the request serialization module would returnParseError
, which did not provide much context.Distinguishing between frame ser and deser errors
Decoupled
FrameError
(which originally was overloaded with variants corresponding to serialization and deserialization) into:CqlRequestSerializationError
- in fact, I only moved the variant regarding frame body compression fromFrameError
toCqlRequestSerializationError
FrameHeaderParseError
- it corresponds to the errors that appear whenConnection::reader()
async task receives a frame, and tries to deserialize its header. It happens before the actual response handler is chosen (based on the stream id).FrameBodyExtensionsParseError
- as the name suggests, it is returned when the driver fails to deserialize body extensions (e.g. frame warnings, or custom payload).Test utilities making use of ParseError
There were a couple of test utilities used by
scylla-proxy
that would depend on ParseError. In some cases, if needed, I introduced new error types (e.g.RequestDeserializationError
). I provided a docstring, noting that this is intended for driver's internal use and testing (it has to bepub
, because ofscylla-proxy
crate).Removing ParseError
With this PR, we could finally remove
ParseError
altogether! I believe that there are still some error-related issues to be addressed, but this is a huge step forward, as this was the most problematic error type.Pre-review checklist
[ ] I added relevant tests for new features and bug fixes.[ ] I have adjusted the documentation in./docs/source/
.Fixes:
annotations to PR description.