session: internal request execution API error types refactor #1157

muzarski · 2024-12-23T11:31:13Z

run_request_once closure

To narrow the error return types of some important API functions (e.g. RS::decide_should_retry and LBP::on_query_failure), the error type of execute_request_once (previously known as do_query) needs to be adjusted as well. A couple of commits at the beginning of this PR do just that. What we achieve, is that we narrow the return error type of the closure from QueryError to UserRequestError.

UserRequestError

This error is renamed to RequestAttemptError - as the name suggests, it represents a failure of a single attempt of execution.

LBP and RetrySession

Both of these traits accept some error and do something based on the error. Before this PR, they would accept a QueryError, resulting in matches against variants that could never even be constructed before the call. In this PR, I pub-ified the RequestAttemptError, and made LBP::on_query_failure and RetrySession::decide_should_retry accept it instead of QueryError.

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-23T11:36:34Z

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

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev 36a677594c5f546626cf0aea05fb3b62f2a23d54
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 36a677594c5f546626cf0aea05fb3b62f2a23d54
     Cloning 36a677594c5f546626cf0aea05fb3b62f2a23d54
    Building scylla v0.15.0 (current)
       Built [  23.089s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.055s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  23.516s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.050s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.105s] 107 checks: 105 pass, 2 fail, 0 warn, 0 skip

--- 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::policies::retry::QueryInfo, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-36a677594c5f546626cf0aea05fb3b62f2a23d54/144950dce43fd3f7e432f041e0295ea0699f4a63/scylla/src/policies/retry/retry_policy.rs:9

--- 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 on_query_success of trait LoadBalancingPolicy, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-36a677594c5f546626cf0aea05fb3b62f2a23d54/144950dce43fd3f7e432f041e0295ea0699f4a63/scylla/src/policies/load_balancing/mod.rs:86
  method on_query_failure of trait LoadBalancingPolicy, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-36a677594c5f546626cf0aea05fb3b62f2a23d54/144950dce43fd3f7e432f041e0295ea0699f4a63/scylla/src/policies/load_balancing/mod.rs:89

     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  47.908s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  21.959s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.124s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  12.124s] (baseline)
     Parsing scylla-cql v0.4.0 (baseline)
      Parsed [   0.034s] (baseline)
    Checking scylla-cql v0.4.0 -> v0.4.0 (no change)
     Checked [   0.111s] 107 checks: 107 pass, 0 skip
     Summary no semver update required
    Finished [  35.051s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

scylla/src/transport/connection.rs

scylla/src/transport/session.rs

muzarski · 2024-12-24T04:54:24Z

v1.1: Rebased on latest #1156 and addressed @Lorak-mmk comments. Most important change is that we replaced From<UserRequestError> for QueryError with explicit conversion method UserRequestError::into_query_error().

Lorak-mmk

Looks good.

A suggestion for the future (because this PR is probably not a good place for this): could you re-order variants in UserRequestError so that they match the order in QueryError? Changing the QueryError instead would be fine too - I just think that the variants present in both should appear in the same order in both.

scylla/src/transport/errors.rs

scylla/src/transport/load_balancing/mod.rs

scylla/src/transport/session.rs

scylla/src/transport/iterator.rs

scylla/src/transport/retry_policy.rs

muzarski · 2024-12-30T08:24:05Z

Addressed @wprzytula comments

The merge-base changed after approval.

muzarski · 2024-12-30T11:07:50Z

Rebased on main

scylla/src/transport/errors.rs

scylla/src/transport/downgrading_consistency_retry_policy.rs

muzarski · 2025-01-13T10:49:20Z

Rebased on main (modules refactor)

muzarski · 2025-01-13T11:03:27Z

Flaky test_coalescing failed

mykaul · 2025-01-13T11:06:06Z

Flaky test_coalescing failed

Is there an open issue about it? Reference it please (or open one)

muzarski · 2025-01-13T11:09:27Z

Flaky test_coalescing failed

Is there an open issue about it? Reference it please (or open one)

#864

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.

Reordered the variants with respect to the moment corresponding error can arise at during request execution.

muzarski · 2025-01-15T10:19:27Z

Rebased on main

New name clearly suggests that this is a failure of a SINGLE attempt of request execution.

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.

muzarski · 2025-01-15T11:23:43Z

v1.2: Renamed UserRequestError -> RequestAttemptError (commit taken from follow-up PR)

muzarski requested review from Lorak-mmk and wprzytula and removed request for Lorak-mmk December 23, 2024 11:34

muzarski self-assigned this Dec 23, 2024

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

Lorak-mmk requested changes Dec 23, 2024

View reviewed changes

scylla/src/transport/connection.rs Outdated Show resolved Hide resolved

scylla/src/transport/connection.rs Outdated Show resolved Hide resolved

scylla/src/transport/session.rs Outdated Show resolved Hide resolved

muzarski force-pushed the run_query_errors branch from cc87b86 to 22c90e4 Compare December 24, 2024 04:50

muzarski requested a review from Lorak-mmk December 24, 2024 04:54

Lorak-mmk previously approved these changes Dec 24, 2024

View reviewed changes

muzarski mentioned this pull request Dec 27, 2024

errors: narrow error types accepted by HistoryListener #1159

Open

6 tasks

wprzytula requested changes Dec 28, 2024

View reviewed changes

muzarski dismissed Lorak-mmk’s stale review via fe5e8c5 December 30, 2024 08:21

muzarski force-pushed the run_query_errors branch from 22c90e4 to fe5e8c5 Compare December 30, 2024 08:21

wprzytula previously approved these changes Dec 30, 2024

View reviewed changes

muzarski force-pushed the run_query_errors branch from b0d77b4 to ce2c6cc Compare December 30, 2024 11:07

muzarski requested review from wprzytula and Lorak-mmk December 30, 2024 12:13

wprzytula previously approved these changes Dec 31, 2024

View reviewed changes

Lorak-mmk requested changes Jan 2, 2025

View reviewed changes

muzarski force-pushed the run_query_errors branch from ce2c6cc to 67692e0 Compare January 9, 2025 10:11

muzarski requested a review from Lorak-mmk January 9, 2025 10:12

muzarski dismissed wprzytula’s stale review via 629f112 January 13, 2025 10:29

muzarski force-pushed the run_query_errors branch 2 times, most recently from 629f112 to 0e3dc7e Compare January 13, 2025 10:45

muzarski requested a review from wprzytula January 13, 2025 10:49

muzarski force-pushed the run_query_errors branch from 0e3dc7e to cdea322 Compare January 14, 2025 12:37

wprzytula added this to the 0.16.0 milestone Jan 15, 2025

muzarski added 10 commits January 15, 2025 11:19

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

b4f14ff

This is to prevent some unwanted implicit conversions.

batch: narrow the return error type of prepare_batch

05b7fce

batch: return UserRequestError::UnexpectedResponse

45bacd6

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

error: Add RepreparedIdMissingInBatch variant to UserRequestError

292ee77

batch: return UserRequestError from batch_with_consistency

ac646b7

Adjusted, so the callers now convert QueryResponse to QueryResult.

errors: Add serialization error to UserRequestError

9cc2188

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

1add181

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

5dff058

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

errors: reorder UserRequestError variants

0ccb5f5

Reordered the variants with respect to the moment corresponding error can arise at during request execution.

muzarski force-pushed the run_query_errors branch from cdea322 to 8f05eb2 Compare January 15, 2025 10:19

muzarski added 7 commits January 15, 2025 12:09

errors: rename UserRequestError to RequestAttemptError

f147840

New name clearly suggests that this is a failure of a SINGLE attempt of request execution.

lbp: pass RequestAttemptError to LBP::on_query_failure

7052e16

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

3e0c903

Request is a more fitting name here.

iter: narrow return error type of some internal functions

8b73ff2

retry_policy: decision based on RequestAttemptError

b94f6b5

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

retry_policy: replace query mentions with request

5485130

iterator: replace "query" with "request" in logs

e4bb929

muzarski force-pushed the run_query_errors branch from 8f05eb2 to e4bb929 Compare January 15, 2025 11:21

wprzytula approved these changes Jan 15, 2025

View reviewed changes

wprzytula merged commit 54402c7 into scylladb:main Jan 15, 2025
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

session: internal request execution API error types refactor #1157

session: internal request execution API error types refactor #1157

muzarski commented Dec 23, 2024 •

edited

Loading

github-actions bot commented Dec 23, 2024 •

edited

Loading

muzarski commented Dec 24, 2024

Lorak-mmk left a comment

muzarski commented Dec 30, 2024

muzarski commented Dec 30, 2024

muzarski commented Jan 13, 2025

muzarski commented Jan 13, 2025

mykaul commented Jan 13, 2025

muzarski commented Jan 13, 2025

muzarski commented Jan 15, 2025

muzarski commented Jan 15, 2025

session: internal request execution API error types refactor #1157

session: internal request execution API error types refactor #1157

Conversation

muzarski commented Dec 23, 2024 • edited Loading

run_request_once closure

UserRequestError

LBP and RetrySession

Pre-review checklist

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

muzarski commented Dec 24, 2024

Lorak-mmk left a comment

Choose a reason for hiding this comment

muzarski commented Dec 30, 2024

muzarski commented Dec 30, 2024

muzarski commented Jan 13, 2025

muzarski commented Jan 13, 2025

mykaul commented Jan 13, 2025

muzarski commented Jan 13, 2025

muzarski commented Jan 15, 2025

muzarski commented Jan 15, 2025

muzarski commented Dec 23, 2024 •

edited

Loading

github-actions bot commented Dec 23, 2024 •

edited

Loading