errors: protocol error refactor #1080

muzarski · 2024-09-25T10:54:55Z

QueryError::ProtocolError

In this PR we do a refactor of the QueryError::ProtocolError variant, which was represented simply as &'static str. In this PR, we create a new type ProtocolError which will represent all posible kinds of protocol errors. We simply replace all occurrences of QueryError::ProtocolError with a corresponding ProtocolError variant.

QueryError::InvalidMessage

This variant is no longer use after the fixes. Previously, this variant was constructed for some protocol errors due to limitations of the QueryError::ProtocolError variant type (&'static str, when String was needed). After introducing ProtocolError type, this is no longer an issue. This variant can be safely removed.

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.

github-actions · 2024-09-25T10:59:51Z

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

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev ea7c464bf1a3d10fadc1d67827dd17e1ee191b7d
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev ea7c464bf1a3d10fadc1d67827dd17e1ee191b7d
     Cloning ea7c464bf1a3d10fadc1d67827dd17e1ee191b7d
     Parsing scylla v0.14.0 (current)
      Parsed [  22.762s] (current)
     Parsing scylla v0.14.0 (baseline)
      Parsed [  20.625s] (baseline)
    Checking scylla v0.14.0 -> v0.14.0 (no change)
     Checked [   0.108s] 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 QueryError::InvalidMessage, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ea7c464bf1a3d10fadc1d67827dd17e1ee191b7d/e6f2e7f1733f2bdab932241d6890de3ef2b96662/scylla/src/transport/errors.rs:71
  variant NewSessionError::InvalidMessage, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-ea7c464bf1a3d10fadc1d67827dd17e1ee191b7d/e6f2e7f1733f2bdab932241d6890de3ef2b96662/scylla/src/transport/errors.rs:208

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  43.554s] scylla
     Parsing scylla-cql v0.3.0 (current)
      Parsed [  10.518s] (current)
     Parsing scylla-cql v0.3.0 (baseline)
      Parsed [  10.331s] (baseline)
    Checking scylla-cql v0.3.0 -> v0.3.0 (no change)
     Checked [   0.101s] 89 checks: 89 pass, 0 skip
     Summary no semver update required
    Finished [  21.009s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

muzarski · 2024-09-26T10:40:49Z

Rebased on main

scylla/src/transport/errors.rs

scylla/src/transport/connection.rs

scylla/src/transport/errors.rs

scylla/src/transport/topology.rs

scylla/src/transport/errors.rs

scylla/src/statement/prepared_statement.rs

scylla/src/transport/retry_policy.rs

scylla/src/transport/connection.rs

scylla/src/transport/errors.rs

Lorak-mmk · 2024-10-01T09:54:28Z

scylla/src/transport/errors.rs

+    /// A protocol error appeared during cluster metadata fetch.
+    #[error("Cluster metadata fetch protocol error: {0}")]
+    Metadata(#[from] MetadataProtocolError),
+


Why this variant gets #[from] but others dont?

Since I think it makes sense to do it for this variant. However, IMO it's not true for variants such as UnexpectedResponse(CqlResponseKind) - here, CqlResponseKind is not an error type itself, it's there only to provide more context. The error is that we received an unexpected response - I don't think From<CqlResponseKind> impl makes sense here.

scylla/src/transport/topology.rs

scylla/src/transport/errors.rs

scylla/src/transport/topology.rs

muzarski · 2024-10-03T07:04:33Z

v2: addressed review comments

Most important change is that I removed Protocol infix from errors related to metadata (e.g. MetadataProtocolError -> MetadataError). I also moved MetadaError out of ProtocolError, directly under [Query/NS]Error.

scylla/src/transport/connection.rs

scylla/src/transport/session.rs

scylla/src/transport/errors.rs

In the following commits, we will be replacing `QueryError::ProtocolError` variants usages with QueryError::ProtocolErrorTyped. Once every usage of QueryError::ProtocolError is replaced, we will remove this variant and rename ProtocolErrorTyped to ProtocolError. ProtocolErrorTyped is a temporary variant name.

Instead of converting keyspace strings to lowercase (which implies an allocation), we can make use of a comparator that does not allocate additional memory.

Added new error type: UseKeyspaceProtocolError and a corresponding variant to ProtocolError.

Wrapped `SingleRowTypedError` returned during schema version fetch. Previously, we would simply drop this error and provide some custom error messages - which, in my opinion were not really insightful.

Added a variant for nonfinished paging state in response to unpaged query.

Previously, this would be a ProtocolError. However, an empty plan received from LBP does not really represent a protocol violation.

Added an error variant that corresponds to pk extraction error. It indicates that prepared statement's metadata provided by the server is invalid.

muzarski · 2024-10-04T10:39:48Z

v2.1: addressed @Lorak-mmk comments

Replaced last QueryError::ProtocolError variant usages with QueryError::ProtocolErrorTyped (via From impl).

This type represents an error that occurred during cluster metadata fetch (peers and keyspaces).

From now on, the driver will only expect 'class' and replication factor per dc options for NetworkTopologyStrategy. If some other option is detected, an error is returned.

We removed last usages of this variant in previous commit. We can finally get rid of yet another variant that stringified errors.

muzarski marked this pull request as draft September 25, 2024 10:55

muzarski self-assigned this Sep 25, 2024

github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Sep 25, 2024

muzarski marked this pull request as ready for review September 25, 2024 11:06

muzarski requested review from Lorak-mmk and wprzytula September 25, 2024 11:06

muzarski force-pushed the protocol-error-refactor branch from 09bc8ce to d75ef64 Compare September 26, 2024 10:40

wprzytula requested changes Oct 1, 2024

View reviewed changes

Lorak-mmk requested changes Oct 1, 2024

View reviewed changes

muzarski force-pushed the protocol-error-refactor branch 2 times, most recently from 92311fc to f2f43d9 Compare October 2, 2024 11:47

muzarski requested review from Lorak-mmk and wprzytula October 3, 2024 07:04

wprzytula previously approved these changes Oct 3, 2024

View reviewed changes

Lorak-mmk requested changes Oct 3, 2024

View reviewed changes

muzarski added 12 commits October 4, 2024 12:21

use_keyspace: make use of eq_ignore_ascii_case

a530a3f

Instead of converting keyspace strings to lowercase (which implies an allocation), we can make use of a comparator that does not allocate additional memory.

errors: UseKeyspaceProtocolError

1aae3a2

Added new error type: UseKeyspaceProtocolError and a corresponding variant to ProtocolError.

errors: schema version fetch protocol error

dc824ab

Wrapped `SingleRowTypedError` returned during schema version fetch. Previously, we would simply drop this error and provide some custom error messages - which, in my opinion were not really insightful.

errors: nonfinished paging state protocol error

70c9ca2

Added a variant for nonfinished paging state in response to unpaged query.

errors: LBP empty plan error

5b180f1

Previously, this would be a ProtocolError. However, an empty plan received from LBP does not really represent a protocol violation.

errors: prepared statement id mismatch protocol error

5fc5768

errors: reprepared stmt id changed protocol error

997eb7a

errors: fix InvalidCqlType -> QueryError conversion

06b68ae

errors: partition key extraction error

18319d9

Added an error variant that corresponds to pk extraction error. It indicates that prepared statement's metadata provided by the server is invalid.

errors: TracingProtocolError

0a2a1ca

errors: reprepared statement's id missing in batch

e904495

errors: unexpected response protocol error

8e4a392

muzarski force-pushed the protocol-error-refactor branch from f2f43d9 to 1c11070 Compare October 4, 2024 10:37

muzarski added 12 commits October 4, 2024 12:44

errors: replace QueryError::ProtocolError usage in tests

cc7169e

Replaced last QueryError::ProtocolError variant usages with QueryError::ProtocolErrorTyped (via From impl).

errors: introduce MetadataError

00bca6c

This type represents an error that occurred during cluster metadata fetch (peers and keyspaces).

errors: PeersMetadataError

37cd965

errors: KeyspacesMetadataError

0d9e43f

errors: KeyspaceStrategyError

2064774

topology: return error on unexpected NTS option

2bc17a2

From now on, the driver will only expect 'class' and replication factor per dc options for NetworkTopologyStrategy. If some other option is detected, an error is returned.

errors: UdtMetadataError

77626d5

errors: TablesMetadataError

7059593

errors: ViewsMetadataError

3e6e348

errors: remove QueryError::InvalidMessage variant

3514dc2

We removed last usages of this variant in previous commit. We can finally get rid of yet another variant that stringified errors.

errors: remove QueryError::ProtocolError variant

e1b3f84

errors: rename variant ProtocolErrorTyped -> ProtocolError

204f911

muzarski force-pushed the protocol-error-refactor branch from 1c11070 to 204f911 Compare October 4, 2024 10:44

muzarski requested review from wprzytula and Lorak-mmk October 4, 2024 10:55

Lorak-mmk approved these changes Oct 4, 2024

View reviewed changes

muzarski dismissed wprzytula’s stale review via 204f911 October 4, 2024 11:37

wprzytula approved these changes Oct 4, 2024

View reviewed changes

wprzytula merged commit 2ac20a9 into scylladb:main Oct 4, 2024
11 checks passed

wprzytula mentioned this pull request Nov 14, 2024

Release 0.15 #1125

Merged

muzarski deleted the protocol-error-refactor branch December 19, 2024 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors: protocol error refactor #1080

errors: protocol error refactor #1080

muzarski commented Sep 25, 2024 •

edited

Loading

github-actions bot commented Sep 25, 2024 •

edited

Loading

muzarski commented Sep 26, 2024

Lorak-mmk Oct 1, 2024

muzarski Oct 3, 2024 •

edited

Loading

muzarski commented Oct 3, 2024

muzarski commented Oct 4, 2024

errors: protocol error refactor #1080

errors: protocol error refactor #1080

Conversation

muzarski commented Sep 25, 2024 • edited Loading

QueryError::ProtocolError

QueryError::InvalidMessage

Pre-review checklist

github-actions bot commented Sep 25, 2024 • edited Loading

muzarski commented Sep 26, 2024

Lorak-mmk Oct 1, 2024

Choose a reason for hiding this comment

muzarski Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

muzarski commented Oct 3, 2024

muzarski commented Oct 4, 2024

muzarski commented Sep 25, 2024 •

edited

Loading

github-actions bot commented Sep 25, 2024 •

edited

Loading

muzarski Oct 3, 2024 •

edited

Loading