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

session: internal request execution API error types refactor #1157

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

muzarski
Copy link
Contributor

Ref: #519
Depends on: #1156

execute_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.

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 UserRequestError, 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.

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.
@muzarski muzarski requested review from Lorak-mmk and wprzytula and removed request for Lorak-mmk December 23, 2024 11:34
@muzarski muzarski self-assigned this Dec 23, 2024
Copy link

github-actions bot commented Dec 23, 2024

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

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 [  23.047s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.054s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  21.862s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.052s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.122s] 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::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 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-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: 2 major and 0 minor checks failed
    Finished [  46.553s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  10.457s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.036s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  10.844s] (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.111s] 107 checks: 107 pass, 0 skip
     Summary no semver update required
    Finished [  22.311s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Dec 23, 2024
scylla/src/transport/connection.rs Show resolved Hide resolved
scylla/src/transport/connection.rs Outdated Show resolved Hide resolved
scylla/src/transport/session.rs Outdated Show resolved Hide resolved
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.
@muzarski
Copy link
Contributor Author

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().

@muzarski muzarski requested a review from Lorak-mmk December 24, 2024 04:54
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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

2 participants