errors: iterator API errors refactor #1160

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

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.

The name is more fitting, because this function is generic over CQL request kinds. Query is a name for a specific CQL request. Updated the names of variables, and some types that are directly used by this function. Adjusted the documentation of the function.

Again, query would suggest that we want to execute a CQL QUERY request. New name also clearly tells the user that provided closure can be potentially executed multiple (based on plan and retry session). Documented the function. Renamed ExecuteQueryContext -> ExecuteRequestContext

This is to prevent some unwanted implicit conversions.

This is then converted to `ProtocolError` in UserRequestError::into_query_error().

Adjusted, so the callers now convert QueryResponse to QueryResult.

Notice that into_query_result returns QueryError, and not UserRequestError (as into_non_error_query_response). It's then converted to QueryResult later. This change is required to prepare for further changes (i.e. narrowing the return type of errors passed to LBP, retry policy etc.).

In the next commit, I want to narrow the error type of `do_query` closure to UserRequestError. `do_query` closure for `query` method serializes the values after preparation.

Thanks to that, we will be able to pass the narrower error type to LBP::on_query_failure. Currently, QueryError is passed, but not all of its variants could even be constructed at this stage.

This error will be passed to LoadBalancingPolicy::on_query_failure and to RetrySession::decide_should_retry.

This narrows the error type passed to this function. Thanks to that, we do not have to match against the variants that could not ever be passed there (which was the case for QueryError).

Request is a more fitting name here.

Same as for LBP::on_query_failure, it narrows the type.

Introduced new error type and adjusted session.rs, speculative_execution module and iterator module to this type.

Introduced: - RequestTimeout(std::time::Duration) - for requests that timed out with provided client timeout - SchemaAgreementTimeout(std::time::Duration) - for schema agreement timeouts

This represents either a request failure, or a request timeout. It's created to narrow the type passed to history listener.

- `log_attempt_error` will now accept an error representing a single request failure - namely `UserRequestError` - `log_query_error` will now accept an error representing a timeoutable request run failure. Compared to `UserRequestError`, it additionally can contain information about an empty plan error, timeout error etc.

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.

The `RequestTimeout` can only be constructed in analogous session.rs code. Thus, to narrow the error type (later in this PR) even more in iterator API, we convert it safely back to RetriableRequestError (after it was passed to HistoryListener).

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.

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

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

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev 847e44d61e65eccb6205bd586ccaa808e57a6c19
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 847e44d61e65eccb6205bd586ccaa808e57a6c19
     Cloning 847e44d61e65eccb6205bd586ccaa808e57a6c19
    Building scylla v0.15.0 (current)
       Built [  24.223s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.055s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  22.867s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.053s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.119s] 107 checks: 98 pass, 9 fail, 0 warn, 0 skip

--- failure constructible_struct_adds_field: externally-constructible struct adds field ---

Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field StructuredHistory.requests in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/history.rs:257

--- 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_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::history::QueryHistoryResult, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-847e44d61e65eccb6205bd586ccaa808e57a6c19/389a3366a0a28da0298589ce4c12e851cb8631c2/scylla/src/history.rs:264

--- 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 HistoryEvent:NewRequest in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/history.rs:92
  variant HistoryEvent:RequestSuccess in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/history.rs:93
  variant HistoryEvent:RequestError in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/history.rs:94
  variant LegacyNextRowError:NextRowError in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/iterator.rs:1186

--- 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-847e44d61e65eccb6205bd586ccaa808e57a6c19/389a3366a0a28da0298589ce4c12e851cb8631c2/scylla/src/transport/iterator.rs:1165
  variant HistoryEvent::NewQuery, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-847e44d61e65eccb6205bd586ccaa808e57a6c19/389a3366a0a28da0298589ce4c12e851cb8631c2/scylla/src/history.rs:89
  variant HistoryEvent::QuerySuccess, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-847e44d61e65eccb6205bd586ccaa808e57a6c19/389a3366a0a28da0298589ce4c12e851cb8631c2/scylla/src/history.rs:90
  variant HistoryEvent::QueryError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-847e44d61e65eccb6205bd586ccaa808e57a6c19/389a3366a0a28da0298589ce4c12e851cb8631c2/scylla/src/history.rs:91

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct 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/struct_missing.ron

Failed in:
  struct scylla::history::QueryId, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-847e44d61e65eccb6205bd586ccaa808e57a6c19/389a3366a0a28da0298589ce4c12e851cb8631c2/scylla/src/history.rs:17
  struct scylla::history::QueryHistory, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-847e44d61e65eccb6205bd586ccaa808e57a6c19/389a3366a0a28da0298589ce4c12e851cb8631c2/scylla/src/history.rs:256
  struct scylla::transport::retry_policy::QueryInfo, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-847e44d61e65eccb6205bd586ccaa808e57a6c19/389a3366a0a28da0298589ce4c12e851cb8631c2/scylla/src/transport/retry_policy.rs:9
  struct scylla::retry_policy::QueryInfo, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-847e44d61e65eccb6205bd586ccaa808e57a6c19/389a3366a0a28da0298589ce4c12e851cb8631c2/scylla/src/transport/retry_policy.rs:9

--- failure struct_pub_field_missing: pub struct's pub field removed or renamed ---

Description:
A publicly-visible struct has at least one public field 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/struct_pub_field_missing.ron

Failed in:
  field queries of struct StructuredHistory, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-847e44d61e65eccb6205bd586ccaa808e57a6c19/389a3366a0a28da0298589ce4c12e851cb8631c2/scylla/src/history.rs:252

--- failure trait_method_added: pub trait method added ---

Description:
A non-sealed public trait added a new method without a default implementation, which breaks downstream implementations of the trait
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/trait_method_added.ron

Failed in:
  trait method scylla::history::HistoryListener::log_request_start in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/history.rs:42
  trait method scylla::history::HistoryListener::log_request_success in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/history.rs:45
  trait method scylla::history::HistoryListener::log_request_error in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/history.rs:48

--- failure trait_method_missing: pub trait method removed or renamed ---

Description:
A trait method is no longer callable, and may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#major-any-change-to-trait-item-signatures
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/trait_method_missing.ron

Failed in:
  method log_query_start of trait HistoryListener, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-847e44d61e65eccb6205bd586ccaa808e57a6c19/389a3366a0a28da0298589ce4c12e851cb8631c2/scylla/src/history.rs:39
  method log_query_success of trait HistoryListener, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-847e44d61e65eccb6205bd586ccaa808e57a6c19/389a3366a0a28da0298589ce4c12e851cb8631c2/scylla/src/history.rs:42
  method log_query_error of trait HistoryListener, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-847e44d61e65eccb6205bd586ccaa808e57a6c19/389a3366a0a28da0298589ce4c12e851cb8631c2/scylla/src/history.rs:45
  method on_query_success of trait LoadBalancingPolicy, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-847e44d61e65eccb6205bd586ccaa808e57a6c19/389a3366a0a28da0298589ce4c12e851cb8631c2/scylla/src/transport/load_balancing/mod.rs:83
  method on_query_failure of trait LoadBalancingPolicy, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-847e44d61e65eccb6205bd586ccaa808e57a6c19/389a3366a0a28da0298589ce4c12e851cb8631c2/scylla/src/transport/load_balancing/mod.rs:86
  method on_query_success of trait LoadBalancingPolicy, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-847e44d61e65eccb6205bd586ccaa808e57a6c19/389a3366a0a28da0298589ce4c12e851cb8631c2/scylla/src/transport/load_balancing/mod.rs:83
  method on_query_failure of trait LoadBalancingPolicy, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-847e44d61e65eccb6205bd586ccaa808e57a6c19/389a3366a0a28da0298589ce4c12e851cb8631c2/scylla/src/transport/load_balancing/mod.rs:86

     Summary semver requires new major version: 9 major and 0 minor checks failed
    Finished [  48.434s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  11.002s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.037s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  11.062s] (baseline)
     Parsing scylla-cql v0.4.0 (baseline)
      Parsed [   0.036s] (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 [  22.824s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

muzarski added 30 commits December 23, 2024 06:54

session: rename do_query -> run_request_once

bf00f8b

session: rename query_plan -> request_plan

a4e52f2

errors: replace From<> implementation with a method for UserRequestError

d6560a7

This is to prevent some unwanted implicit conversions.

batch: narrow the return error type of prepare_batch

1bed141

batch: return UserRequestError::UnexpectedResponse

28b106e

This is then converted to `ProtocolError` in UserRequestError::into_query_error().

error: Add RepreparedIdMissingInBatch variant to UserRequestError

cb118f2

batch: return UserRequestError from batch_with_consistency

9d6f717

Adjusted, so the callers now convert QueryResponse to QueryResult.

errors: Add serialization error to UserRequestError

0ecc7dd

In the next commit, I want to narrow the error type of `do_query` closure to UserRequestError. `do_query` closure for `query` method serializes the values after preparation.

run_query: narrow the return type of run_request_once closure

92046a2

Thanks to that, we will be able to pass the narrower error type to LBP::on_query_failure. Currently, QueryError is passed, but not all of its variants could even be constructed at this stage.

errors: pubify UserRequestError

792ae98

This error will be passed to LoadBalancingPolicy::on_query_failure and to RetrySession::decide_should_retry.

lbp: pass UserRequestError to LBP::on_query_failure

d16576f

This narrows the error type passed to this function. Thanks to that, we do not have to match against the variants that could not ever be passed there (which was the case for QueryError).

lbp: replace query mentions with request

6618d15

Request is a more fitting name here.

iter: narrow return error type of some internal functions

1150c10

retry_policy: decision based on UserRequestError

a86e1f6

Same as for LBP::on_query_failure, it narrows the type.

retry_policy: replace query mentions with request

22c90e4

errors: introduce RetriableRequestError

a83ff11

Introduced new error type and adjusted session.rs, speculative_execution module and iterator module to this type.

errors: replace stringified RequestTimeout variant with two new variants

4279c82

Introduced: - RequestTimeout(std::time::Duration) - for requests that timed out with provided client timeout - SchemaAgreementTimeout(std::time::Duration) - for schema agreement timeouts

errors: introduce TimeoutableRequestError

6e66fcd

This represents either a request failure, or a request timeout. It's created to narrow the type passed to history listener.

history: rename QueryId -> RequestId

036bb71

history: rename QueryHistory[Result] to Request*

c9575f2

history: adjust display for StructuredHistory to "request" terminology

de3c0e2

history: replace remaining "query" mentions with "request"

71539ec

errors: clean up errors related to pk and token extraction

370fe83

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

43e71cc

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

iterator: return NextRowError from next() methods

c4b17fe

muzarski force-pushed the iterator-errors-refactor branch from 4a964ef to c4b17fe Compare December 27, 2024 07:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors: iterator API errors refactor #1160

errors: iterator API errors refactor #1160

muzarski commented Dec 27, 2024

github-actions bot commented Dec 27, 2024 •

edited

Loading

errors: iterator API errors refactor #1160

Are you sure you want to change the base?

errors: iterator API errors refactor #1160

Conversation

muzarski commented Dec 27, 2024

Pre-review checklist

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

github-actions bot commented Dec 27, 2024 •

edited

Loading