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 and extract TracingError #1194

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

muzarski
Copy link
Contributor

@muzarski muzarski commented Feb 3, 2025

Ref: #519

It was previously called TracingProtocolError and was contained in ExecutionError::ProtocolError.

Since we now have a big picture of errors hierarchy, it's much clearer that TracingError should be a separate error type returned by tracing methods. ExecutionError should NOT depend on it.

This PR renames TracingProtocolError to TracingError, and changes the return type of Session::get_tracing_info(). New variant is introduced to TracingError - it contains an ExecutionError, which makes sense since we try to execute queries to system tracing tables.

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 Feb 3, 2025
@muzarski muzarski added the API-stability Part of the effort to stabilize the API label Feb 3, 2025
@muzarski muzarski added this to the 0.16.0 milestone Feb 3, 2025
@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Feb 3, 2025
Copy link

github-actions bot commented Feb 3, 2025

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

See the following report for details:

cargo semver-checks output
./scripts/semver-checks.sh --baseline-rev 699e3895ea9fde34fb250e5581207ceee53dd2aa
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 699e3895ea9fde34fb250e5581207ceee53dd2aa
     Cloning 699e3895ea9fde34fb250e5581207ceee53dd2aa
    Building scylla v0.15.0 (current)
       Built [  21.936s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.049s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  22.230s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.047s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.118s] 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::TracingProtocolError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-699e3895ea9fde34fb250e5581207ceee53dd2aa/4eda4016a3500346a2fafc3e0edc84d78aee3428/scylla/src/errors.rs:267

--- 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 ProtocolError::Tracing, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-699e3895ea9fde34fb250e5581207ceee53dd2aa/4eda4016a3500346a2fafc3e0edc84d78aee3428/scylla/src/errors.rs:216

     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  45.349s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  11.335s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.027s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  10.938s] (baseline)
     Parsing scylla-cql v0.4.0 (baseline)
      Parsed [   0.026s] (baseline)
    Checking scylla-cql v0.4.0 -> v0.4.0 (no change)
     Checked [   0.112s] 127 checks: 127 pass, 0 skip
     Summary no semver update required
    Finished [  23.313s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

wprzytula
wprzytula previously approved these changes Feb 3, 2025
Lorak-mmk
Lorak-mmk previously approved these changes Feb 3, 2025
@wprzytula
Copy link
Collaborator

@muzarski a rebase is needed.

@muzarski muzarski dismissed stale reviews from Lorak-mmk and wprzytula via b1b4e9b February 4, 2025 13:02
@muzarski
Copy link
Contributor Author

muzarski commented Feb 4, 2025

Rebased on main

It was previously called `TracingProtocolError` and was contained in ExecutionError::ProtocolError.

Since we now have a big picture of errors hierarchy, it's much clearer that `TracingError` should
be a separate error type returned by tracing methods. ExecutionError should NOT depend on it.

This commit renamed TracingProtocolError to TracingError, and changes the return type
of Session::get_tracing_info(). New variant is introduced to `TracingError` - it contains
an `ExecutionError`, which makes sense since we try to execute queries to system tracing
tables.
@muzarski
Copy link
Contributor Author

muzarski commented Feb 4, 2025

Rebased on main again (ssl CI fix, and removed legacy ser/deser api)

@Lorak-mmk Lorak-mmk merged commit e23e316 into scylladb:main Feb 5, 2025
11 checks passed
@Lorak-mmk Lorak-mmk mentioned this pull request Mar 4, 2025
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

3 participants