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.

Sign up for GitHub

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

Jump to bottom

errors: refactor errors on USE KEYSPACE execution path and clean up NewSessionError #1180

Merged
merged 5 commits into from
Jan 30, 2025
Merged

errors: refactor errors on USE KEYSPACE execution path and clean up NewSessionError #1180

merged 5 commits into from
Jan 30, 2025

Conversation

muzarski
Copy link
Contributor

@muzarski muzarski commented Jan 27, 2025 •
edited
Loading

Ref: #519

Motivation

NewSessionError has a lot of variants that can't even be constructed. This is all because of From<QueryError> for NewSessionError implementation. The only remaining execution path where we make use of this conversion is USE KEYSPACE request execution. (Previously, there was also MetadataReader::read_metadata that returned QueryError, but this was addressed in previous PR).

This is why we narrow the return error types in USE KEYSPACE execution path - from QueryError to new UseKeyspaceError.

  • 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 Jan 27, 2025
@muzarski muzarski force-pushed the use-keyspace-errors-refactor branch from 80b8a51 to 424cb76 Compare January 27, 2025 16:49
@muzarski muzarski added this to the 0.16.0 milestone Jan 27, 2025
@muzarski muzarski added the API-stability Part of the effort to stabilize the API label Jan 27, 2025
@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Jan 27, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 27, 2025 •
edited
Loading

cargo semver-checks detected some API incompatibilities in this PR.
Checked commit: 32f5a21

See the following report for details:

cargo semver-checks output 
./scripts/semver-checks.sh --baseline-rev 973084717fce8142f2279c5264ae1e809d09318b
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 973084717fce8142f2279c5264ae1e809d09318b
     Cloning 973084717fce8142f2279c5264ae1e809d09318b
    Building scylla v0.15.0 (current)
       Built [  24.815s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.051s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  22.787s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.049s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.125s] 127 checks: 125 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.39.0/src/lints/enum_missing.ron

Failed in:
  enum scylla::errors::UseKeyspaceProtocolError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:348

--- 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.39.0/src/lints/enum_variant_missing.ron

Failed in:
  variant NewSessionError::DbError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:202
  variant NewSessionError::BadQuery, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:206
  variant NewSessionError::CqlRequestSerialization, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:210
  variant NewSessionError::EmptyPlan, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:219
  variant NewSessionError::BodyExtensionsParseError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:223
  variant NewSessionError::CqlResultParseError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:231
  variant NewSessionError::CqlErrorParseError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:235
  variant NewSessionError::ConnectionPoolError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:239
  variant NewSessionError::ProtocolError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:243
  variant NewSessionError::TimeoutError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:247
  variant NewSessionError::BrokenConnection, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:251
  variant NewSessionError::UnableToAllocStreamId, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:255
  variant NewSessionError::RequestTimeout, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:262
  variant NewSessionError::SchemaAgreementTimeout, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:266
  variant NewSessionError::NextRowError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:274
  variant NewSessionError::IntoLegacyQueryResultError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:284
  variant QueryError::TimeoutError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:94
  variant BadQuery::BadKeyspaceName, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:597
  variant ProtocolError::UseKeyspace, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-973084717fce8142f2279c5264ae1e809d09318b/bcf3f1eb8497da70111aa45423a530376ad95ec9/scylla/src/errors.rs:321

     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  48.841s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  11.363s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.035s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  11.545s] (baseline)
     Parsing scylla-cql v0.4.0 (baseline)
      Parsed [   0.034s] (baseline)
    Checking scylla-cql v0.4.0 -> v0.4.0 (no change)
     Checked [   0.126s] 127 checks: 127 pass, 0 skip
     Summary no semver update required
    Finished [  23.896s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

@muzarski
Copy link
Contributor Author

group 0 test failure...

wprzytula
wprzytula previously approved these changes Jan 27, 2025
View reviewed changes
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.

🥳 How wonderful to see NewSessionError finally free of those redundant variants!

scylla/src/network/connection_pool.rs Outdated Show resolved Hide resolved
@muzarski muzarski force-pushed the use-keyspace-errors-refactor branch from 424cb76 to f43a3c1 Compare January 29, 2025 17:09
@muzarski
Copy link
Contributor Author

Rebased on main

wprzytula
wprzytula previously approved these changes Jan 29, 2025
View reviewed changes
@muzarski
errors: introduce UseKeyspaceError
977ba36 
Note: I wasn't able to split it into multiple commits. It's always tricky
to split the changes into multiple commits when there are some tokio channels
we use to send a `Result<_, SomeError>` where SomeError is the error type we
adjust.

Major changes:
- UseKeyspaceProtocolError is now renamed to UseKeyspaceError. New variants are introduced.
- Removed ProtocolError::UseKeyspace variant
- Added UseKeyspaceError variant to QueryError and NewSessionError
- Replaced `QueryError` with `UseKeyspacError` in USE KEYSPACE execution path
  in `Session`, `Cluster`, `ClusterWorker`, `Node`, `PoolRefiller` and `Connection` methods.

There is a potential FIXME I introduced. Pay attention to this during review.
@muzarski
errors: remove BadQuery::BadKeyspaceName variant
0956d7f 
It was moved to UseKeyspaceError
@muzarski
errors: remove [Query/NewSession]Error::TimeoutError variant
f741194 
It's not used anymore
@muzarski
errors: remove From<QueryError> for NewSessionError
ca2a425
@muzarski
errors: remove unused NewSessionError variants
32f5a21
@muzarski muzarski force-pushed the use-keyspace-errors-refactor branch from f43a3c1 to 32f5a21 Compare January 30, 2025 13:12
@muzarski
Copy link
Contributor Author

v1.1: Adjusted the FIXME related to USE KEYSPACE request timeout

@wprzytula wprzytula merged commit 4c464b6 into scylladb:main Jan 30, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wprzytula wprzytula wprzytula approved these changes

@Lorak-mmk Lorak-mmk Lorak-mmk approved these changes

Assignees

@muzarski muzarski

Labels
API-stability Part of the effort to stabilize the API semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes
Projects
None yet
Milestone
0.16.0
Development

Successfully merging this pull request may close these issues.
3 participants
@muzarski @Lorak-mmk @wprzytula