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

errors: remove ParseError #1054

Merged
merged 30 commits into from
Sep 26, 2024
Merged

Conversation

muzarski
Copy link
Contributor

@muzarski muzarski commented Aug 7, 2024

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 return ParseError, 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 from FrameError to CqlRequestSerializationError
  • FrameHeaderParseError - it corresponds to the errors that appear when Connection::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 be pub, because of scylla-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 have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • [ ] I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • [ ] I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@muzarski muzarski self-assigned this Aug 7, 2024
@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Aug 7, 2024
Copy link

github-actions bot commented Aug 7, 2024

cargo semver-checks detected some API incompatibilities in this PR.
Checked commit: 62c661b

See the following report for details:

cargo semver-checks output
./scripts/semver-checks.sh --baseline-rev ab07be6d8816df9ba53529391b65fbd63cc3e59a
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev ab07be6d8816df9ba53529391b65fbd63cc3e59a
     Cloning ab07be6d8816df9ba53529391b65fbd63cc3e59a
     Parsing scylla v0.14.0 (current)
      Parsed [  23.910s] (current)
     Parsing scylla v0.14.0 (baseline)
      Parsed [  20.701s] (baseline)
    Checking scylla v0.14.0 -> v0.14.0 (no change)
     Checked [   0.100s] 89 checks: 88 pass, 1 fail, 0 warn, 0 skip

--- failure enum_variant_missing: pub enum variant removed or renamed ---

Description:
A publicly-visible enum has at least one variant that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.35.0/src/lints/enum_variant_missing.ron

Failed in:
  variant BrokenConnectionErrorKind::FrameError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla/src/transport/errors.rs:499
  variant RequestError::FrameError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla/src/transport/errors.rs:624
  variant ConnectionSetupRequestErrorKind::FrameError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla/src/transport/errors.rs:386
  variant CqlEventHandlingError::FrameError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla/src/transport/errors.rs:552

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  44.763s] scylla
     Parsing scylla-cql v0.3.0 (current)
      Parsed [  10.120s] (current)
     Parsing scylla-cql v0.3.0 (baseline)
      Parsed [  10.017s] (baseline)
    Checking scylla-cql v0.3.0 -> v0.3.0 (no change)
     Checked [   0.096s] 89 checks: 87 pass, 2 fail, 0 warn, 0 skip

--- failure enum_missing: pub enum removed or renamed ---

Description:
A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.35.0/src/lints/enum_missing.ron

Failed in:
  enum scylla_cql::frame::frame_errors::ParseError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla-cql/src/frame/frame_errors.rs:40
  enum scylla_cql::cql_to_rust::CqlTypeError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla-cql/src/frame/response/cql_to_rust.rs:20
  enum scylla_cql::frame::response::cql_to_rust::CqlTypeError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla-cql/src/frame/response/cql_to_rust.rs:20
  enum scylla_cql::frame::frame_errors::FrameError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla-cql/src/frame/frame_errors.rs:12

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.35.0/src/lints/inherent_method_missing.ron

Failed in:
  LegacySerializedValues::new_from_frame, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ab07be6d8816df9ba53529391b65fbd63cc3e59a/789b83d096ad7d9a362060a8016a38862072c0e5/scylla-cql/src/frame/value.rs:803

     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  20.287s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

@wprzytula
Copy link
Collaborator

ref: #1056

@wprzytula wprzytula added this to the 0.14.0 milestone Aug 20, 2024
@muzarski muzarski force-pushed the serializable_request_error branch from ccb7c3d to 8855a32 Compare August 20, 2024 16:16
@muzarski muzarski marked this pull request as draft August 23, 2024 09:43
@muzarski muzarski force-pushed the serializable_request_error branch from 8855a32 to e385c26 Compare August 27, 2024 01:32
@muzarski
Copy link
Contributor Author

Rebased on #1067

@muzarski muzarski force-pushed the serializable_request_error branch from e385c26 to c56ed97 Compare August 27, 2024 01:45
@muzarski muzarski marked this pull request as ready for review August 27, 2024 01:50
@muzarski muzarski force-pushed the serializable_request_error branch from c56ed97 to 677b1cc Compare August 27, 2024 12:17
@wprzytula wprzytula modified the milestones: 0.14.0, 0.15.0 Aug 29, 2024
@muzarski muzarski force-pushed the serializable_request_error branch 2 times, most recently from ed96da7 to 5946397 Compare September 16, 2024 04:35
Copy link
Collaborator

@wprzytula wprzytula left a 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.

scylla-cql/src/frame/frame_errors.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/frame_errors.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/request/batch.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/request/auth_response.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/frame_errors.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/request/mod.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/frame_errors.rs Outdated Show resolved Hide resolved
scylla-cql/src/types/deserialize/mod.rs Outdated Show resolved Hide resolved
scylla-cql/src/types/deserialize/mod.rs Outdated Show resolved Hide resolved
scylla-cql/src/types/deserialize/mod.rs Outdated Show resolved Hide resolved
@muzarski muzarski marked this pull request as draft September 18, 2024 04:44
@muzarski muzarski force-pushed the serializable_request_error branch 2 times, most recently from 208bf72 to a983be6 Compare September 19, 2024 06:40
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.
@muzarski muzarski force-pushed the serializable_request_error branch from 4b840c9 to 62c661b Compare September 25, 2024 15:28
@muzarski
Copy link
Contributor Author

v1.2: addressed @Lorak-mmk 's comments:

  • removed snap::Error and lz4_flex::block::DecompressError types from public API. Replaced them with dynamic Arc<dyn Error> in public enum variants.
  • added docstring for FrameBodyExtensionsParseError

@muzarski muzarski requested a review from Lorak-mmk September 25, 2024 15:32
@muzarski
Copy link
Contributor Author

@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.

@muzarski
Copy link
Contributor Author

@Lorak-mmk Rerun the CI, please. A known flaky test lwt_optimisation::if_lwt_optimisation_mark_offered_then_negotiatied_and_lwt_routed_optimally failed:

`Result::unwrap()` on an `Err` value: DbError(ServerError, "Failed to apply group 0 change due to concurrent modification")

@muzarski
Copy link
Contributor Author

Can we merge?

@Lorak-mmk Lorak-mmk merged commit bcf3a6c into scylladb:main Sep 26, 2024
11 checks passed
@wprzytula wprzytula mentioned this pull request Nov 14, 2024
@muzarski muzarski deleted the serializable_request_error branch December 19, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants