errors: pager API errors refactor #1160

muzarski · 2024-12-27T07:23:50Z

Ref: #519

Motivation

There are three main objectives:

Purge pager.rs module of QueryError

It's yet another module where usage of QueryError was abused. It's taken care of in this PR

Narrow return error type of `[poll_]next()` methods on iterators/streams

They now return a self-contained NextRow error. It's independent of QueryError.

Step forward narrowing error type of metadata related functions

Currently all top-level metadata related functions/methods (e.g. Session::refresh_metadata) return QueryError. This is because low-level methods use QueryPager API under the hood. Before this PR, the pager API would return QueryError. In the follow-up PR we will continue on purging the metadata methods of QueryError. It will be replaced with an error type designated for these operations - namely 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.

github-actions · 2024-12-27T07:29:14Z

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

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev 918e52267e5ce9c7dc17dbcd2ff26b4e6c15d52f
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 918e52267e5ce9c7dc17dbcd2ff26b4e6c15d52f
     Cloning 918e52267e5ce9c7dc17dbcd2ff26b4e6c15d52f
    Building scylla v0.15.0 (current)
       Built [  27.777s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.053s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  24.867s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.050s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.118s] 107 checks: 104 pass, 3 fail, 0 warn, 0 skip

--- failure enum_marked_non_exhaustive: enum marked #[non_exhaustive] ---

Description:
A public enum has been marked #[non_exhaustive]. Pattern-matching on it outside of its crate must now include a wildcard pattern like `_`, or it will fail to compile.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#attr-adding-non-exhaustive
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/enum_marked_non_exhaustive.ron

Failed in:
  enum PartitionKeyError in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/statement/prepared_statement.rs:482
  enum PartitionKeyError in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/statement/prepared_statement.rs:482

--- failure enum_variant_added: enum variant added on exhaustive enum ---

Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/enum_variant_added.ron

Failed in:
  variant LegacyNextRowError:NextRowError in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/client/pager.rs:1180

--- 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 LegacyNextRowError::QueryError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-918e52267e5ce9c7dc17dbcd2ff26b4e6c15d52f/6aa2ec538060aafde22a82a3fb8ca7912d17f498/scylla/src/client/pager.rs:1175

     Summary semver requires new major version: 3 major and 0 minor checks failed
    Finished [  53.841s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  12.181s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.035s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  12.122s] (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.116s] 107 checks: 107 pass, 0 skip
     Summary no semver update required
    Finished [  25.234s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

Lorak-mmk · 2025-01-23T13:21:31Z

scylla/src/client/pager.rs

    /// Failed to deserialize result metadata associated with next page response.
    #[error("Failed to deserialize result metadata associated with next page response: {0}")]
    ResultMetadataParseError(#[from] ResultMetadataAndRowsCountParseError),
-    // TODO: This should also include a variant representing an error that occurred during
-    // query that fetches the next page. However, as of now, it would require that we include QueryError here.
-    // This would introduce a cyclic dependency: QueryError -> NextRowError -> NextPageError -> QueryError.


Commit: "iterator: narrow error type of internal items "
❓ You removed this comment, and included "RequestFailure". This does not introduce a cycle because you used "RequestError" instead of "QueryError", right?

Lorak-mmk · 2025-01-23T13:25:23Z

scylla/src/cluster/metadata.rs

+        // FIXME 2: The specific error we expect here should appear in QueryError::NextRowError. Currently
+        // leaving match against both variants. This will be fixed, once `MetadataError` is further adjusted
+        // in a follow-up PR. The goal is to return MetadataError from all functions related to metadata fetch.
+        Err(QueryError::DbError(DbError::Invalid, _))
+        | Err(QueryError::NextRowError(NextRowError::NextPageError(
+            NextPageError::RequestFailure(RequestError::LastAttemptError(
+                RequestAttemptError::DbError(DbError::Invalid, _),
+            )),
+        ))) => Ok(HashMap::new()),


❓ Why can't you just move this commit before the previous one? Then all the commits would compile and pass tests, right?

True, I'll do that.

Done, but I lied in the PR cover letter. I mentioned that penultimate commit breaks things. I moved the commit with the fix, before iterator: narrow error type of internal items.

I marked `PartitionKeyError` as non_exhaustive. Narrowed the return type of remaining funtions that returned `QueryError` to `PartitionKeyError`. Added an explicit conversion function PartitionKeyError -> QueryError.

This change is required for the tests to pass after next commit. The next commit causes the DbError in topology code to be placed under QueryError::NextRowError. Without this change, tests fail, because we propagate the error for Cassandra clusters, instead of ignoring it.

Narrowed the error types in multiple places in internal API of iterator module. Now the error type we manipulate on mainly is `NextPageError` (instead of `QueryError`). I did not change the return type of public methods yet. I want to do it in a separate commit.

muzarski · 2025-01-23T16:06:24Z

v1.1:

Rebased on main
Reordered commits so tests pass for all of them

muzarski marked this pull request as draft December 27, 2024 07:23

muzarski self-assigned this Dec 27, 2024

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

muzarski force-pushed the iterator-errors-refactor branch 2 times, most recently from c4b17fe to 3cfdcc7 Compare December 30, 2024 13:16

muzarski force-pushed the iterator-errors-refactor branch from 3cfdcc7 to dc72120 Compare January 16, 2025 14:44

muzarski marked this pull request as ready for review January 16, 2025 15:40

muzarski requested review from wprzytula and Lorak-mmk and removed request for wprzytula January 16, 2025 15:41

muzarski mentioned this pull request Jan 16, 2025

errors: metadata errors refactor #1170

Draft

6 tasks

muzarski changed the title ~~errors: iterator API errors refactor~~ errors: pager API errors refactor Jan 16, 2025

muzarski added this to the 0.16.0 milestone Jan 16, 2025

muzarski added the API-stability Part of the effort to stabilize the API label Jan 16, 2025

wprzytula previously approved these changes Jan 17, 2025

View reviewed changes

muzarski dismissed wprzytula’s stale review via 3047435 January 23, 2025 11:44

muzarski force-pushed the iterator-errors-refactor branch from dc72120 to 3047435 Compare January 23, 2025 11:44

Lorak-mmk reviewed Jan 23, 2025

View reviewed changes

muzarski added 5 commits January 23, 2025 16:58

errors: clean up errors related to pk and token extraction

aa2a3e2

I marked `PartitionKeyError` as non_exhaustive. Narrowed the return type of remaining funtions that returned `QueryError` to `PartitionKeyError`. Added an explicit conversion function PartitionKeyError -> QueryError.

iter: narrow return error type of SingleConnectionPagerWorker::do_work

f3b10ef

iterator: return NextRowError from next() methods

04c7eec

muzarski force-pushed the iterator-errors-refactor branch from 3047435 to 04c7eec Compare January 23, 2025 16:05

muzarski requested review from Lorak-mmk and wprzytula January 23, 2025 16:08

wprzytula approved these changes Jan 23, 2025

View reviewed changes

Lorak-mmk approved these changes Jan 23, 2025

View reviewed changes

wprzytula merged commit d40e184 into scylladb:main Jan 23, 2025
9 checks passed

muzarski deleted the iterator-errors-refactor branch January 23, 2025 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors: pager API errors refactor #1160

errors: pager API errors refactor #1160

muzarski commented Dec 27, 2024 •

edited by wprzytula

Loading

github-actions bot commented Dec 27, 2024 •

edited

Loading

Lorak-mmk Jan 23, 2025

muzarski Jan 23, 2025

Lorak-mmk Jan 23, 2025

muzarski Jan 23, 2025

muzarski Jan 23, 2025

muzarski commented Jan 23, 2025

errors: pager API errors refactor #1160

errors: pager API errors refactor #1160

Conversation

muzarski commented Dec 27, 2024 • edited by wprzytula Loading

Motivation

Purge pager.rs module of QueryError

Narrow return error type of [poll_]next() methods on iterators/streams

Step forward narrowing error type of metadata related functions

Pre-review checklist

github-actions bot commented Dec 27, 2024 • edited Loading

Lorak-mmk Jan 23, 2025

Choose a reason for hiding this comment

muzarski Jan 23, 2025

Choose a reason for hiding this comment

Lorak-mmk Jan 23, 2025

Choose a reason for hiding this comment

muzarski Jan 23, 2025

Choose a reason for hiding this comment

muzarski Jan 23, 2025

Choose a reason for hiding this comment

muzarski commented Jan 23, 2025

muzarski commented Dec 27, 2024 •

edited by wprzytula

Loading

Narrow return error type of `[poll_]next()` methods on iterators/streams

github-actions bot commented Dec 27, 2024 •

edited

Loading