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: rename RequestError and make it private #1168

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

muzarski
Copy link
Contributor

Ref: #519

Motivation

Current RequestError is a low-level (connection layer) error that represents a failure of ANY CQL request. Before this PR it was public. Why? There was only one place it was used in public API - namely BrokenConnectionErrorKind::KeepaliveQueryError. This is a very rare error.

I decided to make this error type pub(crate) for multiple reasons:

  • As mentioned previously - this is a very rare low-level error kind. I don't think there is a point in exposing it to the user
  • Thanks to that, we can rename it to InternalRequestError (since it becomes an error used only in driver's internals)
  • RequestError name is now available :). It can be used for some more meaningful error returned from user-facing API. For example, an error representing a definite request failure that occurs in a session layer after possible retries, speculative execution etc.

I've hidden this type in BrokenConnectionErrorKind::KeepaliveRequestError (variant got renamed as well) behind an Arc. It's OK to do so, because this specific case is very rare. I think that not providing any additional context (i.e., ignoring the underlying error cause) is a valid alternative solution.

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.

This is the only PUBLIC occurrence of RequestError. Since we want to
ultimately make it private, let's hide it behind an Arc.

Why is it OK to do so?
1. This is a very rare error case.
2. I don't think users would be interested in checking the exact error cause via matches.
3. We no longer need to expose RequestError in the public API - this is a really low-level
   error that occurs on a connection level.
@muzarski muzarski force-pushed the internal-request-error branch from 5252aa7 to f480b0e Compare January 13, 2025 17:58
@muzarski muzarski self-assigned this Jan 13, 2025
@muzarski muzarski added this to the 0.16.0 milestone Jan 13, 2025
@muzarski muzarski requested a review from wprzytula January 13, 2025 18:01
Copy link

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

See the following report for details:

cargo semver-checks output
./scripts/semver-checks.sh --baseline-rev 7ac4d4556053b335bf0d6439a1ac128545dc00b8
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 7ac4d4556053b335bf0d6439a1ac128545dc00b8
     Cloning 7ac4d4556053b335bf0d6439a1ac128545dc00b8
    Building scylla v0.15.0 (current)
       Built [  22.752s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.051s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  22.605s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.049s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.105s] 107 checks: 105 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.38.0/src/lints/enum_missing.ron

Failed in:
  enum scylla::errors::RequestError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-7ac4d4556053b335bf0d6439a1ac128545dc00b8/693dee0d209335f6347c6207669a6b08fbe5fbb8/scylla/src/errors.rs:972

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

Failed in:
  variant BrokenConnectionErrorKind::KeepaliveQueryError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-7ac4d4556053b335bf0d6439a1ac128545dc00b8/693dee0d209335f6347c6207669a6b08fbe5fbb8/scylla/src/errors.rs:836

     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  46.658s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  11.128s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.034s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  11.035s] (baseline)
     Parsing scylla-cql v0.4.0 (baseline)
      Parsed [   0.033s] (baseline)
    Checking scylla-cql v0.4.0 -> v0.4.0 (no change)
     Checked [   0.107s] 107 checks: 107 pass, 0 skip
     Summary no semver update required
    Finished [  22.994s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Jan 13, 2025
@wprzytula wprzytula merged commit 36a6775 into scylladb:main Jan 15, 2025
11 checks passed
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.

2 participants