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: metadata errors refactor #1170

Merged
merged 15 commits into from
Jan 29, 2025

Conversation

muzarski
Copy link
Contributor

@muzarski muzarski commented Jan 16, 2025

Ref: #519

Motivation

The goal of this PR is to purge metadata.rs of QueryError, and replace it with MetadataError. Thanks to that metadata related functions are now independent of QueryError.

I tried to split this PR into commits, so each commit handles one function/method from metadata module. New error variants are introduced where necessary.

When it comes to public API - after this PR Session::refresh_metadata() will now return MetadataError.

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 marked this pull request as draft January 16, 2025 17:45
@muzarski muzarski force-pushed the topology-errors-refactor branch from 3e1aa4e to 6283bee Compare January 16, 2025 17:46
@muzarski muzarski self-assigned this Jan 16, 2025
@muzarski muzarski added this to the 0.16.0 milestone Jan 16, 2025
@muzarski muzarski added the API-stability Part of the effort to stabilize the API label Jan 16, 2025
@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Jan 16, 2025
Copy link

github-actions bot commented Jan 16, 2025

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

See the following report for details:

cargo semver-checks output
./scripts/semver-checks.sh --baseline-rev 9441b0593385e6aef6af482334b3ad81fcb7c047
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 9441b0593385e6aef6af482334b3ad81fcb7c047
     Cloning 9441b0593385e6aef6af482334b3ad81fcb7c047
    Building scylla v0.15.0 (current)
       Built [  25.547s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.049s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  22.642s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.049s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.127s] 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::ViewsMetadataError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-9441b0593385e6aef6af482334b3ad81fcb7c047/62fca62783f2893603c618574acc5e36f91a0e69/scylla/src/errors.rs:546

--- 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 PeersMetadataError::SystemPeersInvalidColumnType, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-9441b0593385e6aef6af482334b3ad81fcb7c047/62fca62783f2893603c618574acc5e36f91a0e69/scylla/src/errors.rs:455
  variant PeersMetadataError::SystemLocalInvalidColumnType, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-9441b0593385e6aef6af482334b3ad81fcb7c047/62fca62783f2893603c618574acc5e36f91a0e69/scylla/src/errors.rs:459
  variant MetadataError::Views, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-9441b0593385e6aef6af482334b3ad81fcb7c047/62fca62783f2893603c618574acc5e36f91a0e69/scylla/src/errors.rs:446
  variant ProtocolError::InvalidCqlType, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-9441b0593385e6aef6af482334b3ad81fcb7c047/62fca62783f2893603c618574acc5e36f91a0e69/scylla/src/errors.rs:333
  variant TablesMetadataError::SchemaTablesInvalidColumnType, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-9441b0593385e6aef6af482334b3ad81fcb7c047/62fca62783f2893603c618574acc5e36f91a0e69/scylla/src/errors.rs:527
  variant TablesMetadataError::SchemaColumnsInvalidColumnType, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-9441b0593385e6aef6af482334b3ad81fcb7c047/62fca62783f2893603c618574acc5e36f91a0e69/scylla/src/errors.rs:531
  variant UdtMetadataError::SchemaTypesInvalidColumnType, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-9441b0593385e6aef6af482334b3ad81fcb7c047/62fca62783f2893603c618574acc5e36f91a0e69/scylla/src/errors.rs:514
  variant KeyspacesMetadataError::SchemaKeyspacesInvalidColumnType, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-9441b0593385e6aef6af482334b3ad81fcb7c047/62fca62783f2893603c618574acc5e36f91a0e69/scylla/src/errors.rs:476

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

@muzarski muzarski force-pushed the topology-errors-refactor branch 2 times, most recently from 54f07fb to 898b1f8 Compare January 24, 2025 12:32
@muzarski muzarski marked this pull request as ready for review January 24, 2025 12:32
scylla/src/cluster/metadata.rs Outdated Show resolved Hide resolved
scylla/src/cluster/metadata.rs Outdated Show resolved Hide resolved
@muzarski muzarski force-pushed the topology-errors-refactor branch from 898b1f8 to 43d9989 Compare January 27, 2025 13:22
@muzarski
Copy link
Contributor Author

v1.1: addressed comments

@muzarski muzarski requested a review from wprzytula January 27, 2025 13:35
@muzarski muzarski force-pushed the topology-errors-refactor branch from 43d9989 to d51d4f2 Compare January 27, 2025 15:26
@muzarski
Copy link
Contributor Author

Rebased on main

wprzytula
wprzytula previously approved these changes Jan 27, 2025
@muzarski
Copy link
Contributor Author

v2: applied @Lorak-mmk suggestion. Introduced MetadataFetchError. I managed to edit existing commits.

wprzytula
wprzytula previously approved these changes Jan 28, 2025
This error type will contain information about why metadata fetch failed.
Existing enums (UdtMetadataError etc) will now contain only logical errors
related to the metadata verification - for example, if we detect the circular
dependency in UDT definitions.

This commit does not do any changes to the logic yet. It only introduces new error
type and adjust the documentation of MetadataError.
This also allows us to remove the FIXME regarding DbError returned
from cassandra clusters during metadata fetch.
@muzarski
Copy link
Contributor Author

Rebased on main

@muzarski muzarski requested a review from wprzytula January 29, 2025 15:20
Copy link
Collaborator

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the new errors here!

@wprzytula wprzytula merged commit 9730847 into scylladb:main Jan 29, 2025
9 checks passed
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