errors: narrow error types accepted by `HistoryListener` #1159

muzarski · 2024-12-24T06:49:38Z

Depends on #1157. Start review from errors: rename UserRequestError -> UserRequestAttemptError.

New error types and variants

UserRequestAttemptError

It's UserRequestError, but renamed. Represents a failure of a single attempt.

UserRequestError (reintroduced)

Error type returned by run_request_spectulative_fiber. It represents either a definite request failure - last attempt error, connection pool error, or an empty plan error.

Replaced QueryError::RequestTimeout

This error variant was not good for 2 reasons:

it contains stringified error message
it was used for both user requests timeouts, and schema agreement timeouts
I decoupled this variant into two separate variants that provide more context.

TimeoutableRequestError (any idea if there is a more fitting name???)

This is a simple wrapper over new UserRequestError with additional RequestTimeout variant. This error type is only created to narrow the type of errors passed to HistoryListener::log_query_error.

HistoryListener

Narrowed error types passed to log_query_error and log_attempt_error.

log_query_error now accepts TimeoutableRequestError. log_attempt_error now accepts UserRequestAttemptError (representing an error of a single attempt).

Replaced "query" mentions with "request"

I did the replacement in trait methods, docstrings, tests etc. I tried to break the changes into multiple commits.

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-24T06:55:03Z

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

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev 57ad5ad111e371baf3610d34cd15a7f10d05df18
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 57ad5ad111e371baf3610d34cd15a7f10d05df18
     Cloning 57ad5ad111e371baf3610d34cd15a7f10d05df18
    Building scylla v0.15.0 (current)
       Built [  23.091s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.054s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  21.582s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.051s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.124s] 107 checks: 99 pass, 8 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_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-57ad5ad111e371baf3610d34cd15a7f10d05df18/111142da875b42077c7156bb27fe8441206ef0c6/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

--- 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 HistoryEvent::NewQuery, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-57ad5ad111e371baf3610d34cd15a7f10d05df18/111142da875b42077c7156bb27fe8441206ef0c6/scylla/src/history.rs:89
  variant HistoryEvent::QuerySuccess, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-57ad5ad111e371baf3610d34cd15a7f10d05df18/111142da875b42077c7156bb27fe8441206ef0c6/scylla/src/history.rs:90
  variant HistoryEvent::QueryError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-57ad5ad111e371baf3610d34cd15a7f10d05df18/111142da875b42077c7156bb27fe8441206ef0c6/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::QueryHistory, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-57ad5ad111e371baf3610d34cd15a7f10d05df18/111142da875b42077c7156bb27fe8441206ef0c6/scylla/src/history.rs:256
  struct scylla::history::QueryId, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-57ad5ad111e371baf3610d34cd15a7f10d05df18/111142da875b42077c7156bb27fe8441206ef0c6/scylla/src/history.rs:17
  struct scylla::transport::retry_policy::QueryInfo, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-57ad5ad111e371baf3610d34cd15a7f10d05df18/111142da875b42077c7156bb27fe8441206ef0c6/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-57ad5ad111e371baf3610d34cd15a7f10d05df18/111142da875b42077c7156bb27fe8441206ef0c6/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-57ad5ad111e371baf3610d34cd15a7f10d05df18/111142da875b42077c7156bb27fe8441206ef0c6/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-57ad5ad111e371baf3610d34cd15a7f10d05df18/111142da875b42077c7156bb27fe8441206ef0c6/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-57ad5ad111e371baf3610d34cd15a7f10d05df18/111142da875b42077c7156bb27fe8441206ef0c6/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-57ad5ad111e371baf3610d34cd15a7f10d05df18/111142da875b42077c7156bb27fe8441206ef0c6/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-57ad5ad111e371baf3610d34cd15a7f10d05df18/111142da875b42077c7156bb27fe8441206ef0c6/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-57ad5ad111e371baf3610d34cd15a7f10d05df18/111142da875b42077c7156bb27fe8441206ef0c6/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-57ad5ad111e371baf3610d34cd15a7f10d05df18/111142da875b42077c7156bb27fe8441206ef0c6/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-57ad5ad111e371baf3610d34cd15a7f10d05df18/111142da875b42077c7156bb27fe8441206ef0c6/scylla/src/transport/load_balancing/mod.rs:86

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

wprzytula · 2024-12-28T21:59:26Z

scylla/src/transport/errors.rs

+/// Failed to execute a retriable user request.
+#[derive(Error, Debug, Clone)]
+#[non_exhaustive]
+pub enum RetriableRequestError {


❓ What does it mean for a user request to be retriable or not? When a request is non-retriable?

From a way this error type is used, I understand that it encompasses all retry attempts of a request, whereas UserRequestError is returned from a single retry attempt. Correct?

🤔 Something is not yes with the naming of this error, or at least the docstring is too terse, because it sounds a bit vague. Unfortunately, I don't have an idea how to fix this. As this type is user-facing, we should definitely make it clearer.

I also dislike this name. I just couldn't come up with anything better. My only idea is to rename UserRequestError -> [User]RequestAttemptError (I like this name, because it suggests that it's an error of a single attempt). Then we could use UserRequestError name instead of RetriableRequestError.

What do you think of TimeoutableRequestError name (introduced in this PR)? I'm not sure if you reached the commit that introduces it - this enum simply represents either a request timeout, or a request failure (current RetriableRequestError). I wonder if there is a better name for this one as well.

My only idea is to rename UserRequestError -> [User]RequestAttemptError (I like this name, because it suggests that it's an error of a single attempt). Then we could use UserRequestError name instead of RetriableRequestError.

Sounds good, let's do that.

What do you think of TimeoutableRequestError name (introduced in this PR)? I'm not sure if you reached the commit that introduces it - this enum simply represents either a request timeout, or a request failure (current RetriableRequestError). I wonder if there is a better name for this one as well.

MaybeTimeoutedRequestError is one idea that I've had, but it's not ideal, too.

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.

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.

New name clearly suggests that this is an error that occurred during a single attempt of a request.

Introduced new error type and adjusted session.rs, speculative_execution module and iterator module to this type. This error represents a definite request failure (after potential retries).

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 `UserRequestAttemptError` - `log_query_error` will now accept an error representing a timeoutable request run failure. Compared to `UserRequestAttempt`, it additionally can contain information about an empty plan error, timeout error etc.

muzarski · 2024-12-30T12:19:41Z

v2:

rebased on most recent session: internal request execution API error types refactor #1157
renamed UserRequestError -> UserRequestAttemptError and RetriableRequestError -> UserRequestError

muzarski self-assigned this Dec 24, 2024

muzarski marked this pull request as draft December 24, 2024 06:50

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

muzarski force-pushed the history-listener-errors branch from 6a52f4d to 71539ec Compare December 24, 2024 07:13

muzarski marked this pull request as ready for review December 27, 2024 05:15

muzarski requested review from Lorak-mmk and wprzytula and removed request for Lorak-mmk December 27, 2024 05:15

wprzytula reviewed Dec 29, 2024

View reviewed changes

muzarski added 19 commits December 30, 2024 12:07

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

8247fe7

This is to prevent some unwanted implicit conversions.

batch: narrow the return error type of prepare_batch

fd1bf61

batch: return UserRequestError::UnexpectedResponse

0b96bd7

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

error: Add RepreparedIdMissingInBatch variant to UserRequestError

3bbb19d

batch: return UserRequestError from batch_with_consistency

543af77

Adjusted, so the callers now convert QueryResponse to QueryResult.

errors: Add serialization error to UserRequestError

7e56ebb

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

eba0622

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

17f5bf7

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

errors: reorder UserRequestError variants

27554a2

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

lbp: pass UserRequestError to LBP::on_query_failure

c602de5

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

f284264

Request is a more fitting name here.

iter: narrow return error type of some internal functions

f262b9b

retry_policy: decision based on UserRequestError

53d62ba

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

retry_policy: replace query mentions with request

c666b1a

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

ce2c6cc

errors: rename UserRequestError -> UserRequestAttemptError

1aab889

New name clearly suggests that this is an error that occurred during a single attempt of a request.

errors: re-introduce UserRequestError

f804226

Introduced new error type and adjusted session.rs, speculative_execution module and iterator module to this type. This error represents a definite request failure (after potential retries).

errors: replace stringified RequestTimeout variant with two new variants

457e617

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

muzarski added 6 commits December 30, 2024 13:18

errors: introduce TimeoutableRequestError

a6da895

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

a006c60

history: rename QueryHistory[Result] to Request*

26b5909

history: adjust display for StructuredHistory to "request" terminology

3e4ad30

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

d7c8c96

muzarski force-pushed the history-listener-errors branch from 71539ec to d7c8c96 Compare December 30, 2024 12:18

muzarski requested a review from wprzytula December 30, 2024 12:22

wprzytula approved these changes Dec 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

errors: narrow error types accepted by `HistoryListener` #1159

errors: narrow error types accepted by `HistoryListener` #1159

muzarski commented Dec 24, 2024 •

edited

Loading

github-actions bot commented Dec 24, 2024 •

edited

Loading

wprzytula Dec 28, 2024

wprzytula Dec 28, 2024

wprzytula Dec 28, 2024

muzarski Dec 30, 2024

wprzytula Dec 30, 2024

muzarski commented Dec 30, 2024

errors: narrow error types accepted by HistoryListener #1159

Are you sure you want to change the base?

errors: narrow error types accepted by HistoryListener #1159

Conversation

muzarski commented Dec 24, 2024 • edited Loading

New error types and variants

UserRequestAttemptError

UserRequestError (reintroduced)

Replaced QueryError::RequestTimeout

TimeoutableRequestError (any idea if there is a more fitting name???)

HistoryListener

Narrowed error types passed to log_query_error and log_attempt_error.

Replaced "query" mentions with "request"

Pre-review checklist

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

wprzytula Dec 28, 2024

Choose a reason for hiding this comment

wprzytula Dec 28, 2024

Choose a reason for hiding this comment

wprzytula Dec 28, 2024

Choose a reason for hiding this comment

muzarski Dec 30, 2024

Choose a reason for hiding this comment

wprzytula Dec 30, 2024

Choose a reason for hiding this comment

muzarski commented Dec 30, 2024

errors: narrow error types accepted by `HistoryListener` #1159

errors: narrow error types accepted by `HistoryListener` #1159

muzarski commented Dec 24, 2024 •

edited

Loading

github-actions bot commented Dec 24, 2024 •

edited

Loading