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.

Sign up for GitHub

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

Jump to bottom

errors: narrow error types accepted by HistoryListener #1159

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

errors: narrow error types accepted by HistoryListener #1159

wants to merge 9 commits into from
+505 −354

Conversation

muzarski
Copy link
Contributor

@muzarski muzarski commented Dec 24, 2024 •
edited
Loading

Ref: #519

New error types and variants

RequestError (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:

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

RequestError::RequestTimeout

After refactoring the timeout errors in QueryError, I added the corresponding variant to RequestError (one introduced in this PR) as well.

HistoryListener

Narrowed error types passed to log_query_error and log_attempt_error.

log_query_error (or rather log_request_error after rename) now accepts RequestError. log_attempt_error now accepts RequestAttemptError (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.

@muzarski muzarski self-assigned this Dec 24, 2024
@muzarski muzarski marked this pull request as draft December 24, 2024 06:50
@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Dec 24, 2024
@github-actions GitHub Actions
Copy link

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

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

See the following report for details:

cargo semver-checks output 
./scripts/semver-checks.sh --baseline-rev f9f0940dec71b0c6762d813af2da6b4a2578058e
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev f9f0940dec71b0c6762d813af2da6b4a2578058e
     Cloning f9f0940dec71b0c6762d813af2da6b4a2578058e
    Building scylla v0.15.0 (current)
       Built [  22.037s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.052s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  22.157s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.049s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.109s] 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/observability/history.rs:255

--- 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::observability::history::QueryHistoryResult, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-f9f0940dec71b0c6762d813af2da6b4a2578058e/2617f21fcf0fbfe17a5c5f14918ea0b21b68469e/scylla/src/observability/history.rs:265

--- 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/observability/history.rs:90
  variant HistoryEvent:RequestSuccess in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/observability/history.rs:91
  variant HistoryEvent:RequestError in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/observability/history.rs:92

--- 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-f9f0940dec71b0c6762d813af2da6b4a2578058e/2617f21fcf0fbfe17a5c5f14918ea0b21b68469e/scylla/src/observability/history.rs:90
  variant HistoryEvent::QuerySuccess, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-f9f0940dec71b0c6762d813af2da6b4a2578058e/2617f21fcf0fbfe17a5c5f14918ea0b21b68469e/scylla/src/observability/history.rs:91
  variant HistoryEvent::QueryError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-f9f0940dec71b0c6762d813af2da6b4a2578058e/2617f21fcf0fbfe17a5c5f14918ea0b21b68469e/scylla/src/observability/history.rs:92

--- 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::observability::history::QueryId, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-f9f0940dec71b0c6762d813af2da6b4a2578058e/2617f21fcf0fbfe17a5c5f14918ea0b21b68469e/scylla/src/observability/history.rs:18
  struct scylla::observability::history::QueryHistory, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-f9f0940dec71b0c6762d813af2da6b4a2578058e/2617f21fcf0fbfe17a5c5f14918ea0b21b68469e/scylla/src/observability/history.rs:257

--- 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-f9f0940dec71b0c6762d813af2da6b4a2578058e/2617f21fcf0fbfe17a5c5f14918ea0b21b68469e/scylla/src/observability/history.rs:253

--- 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::observability::history::HistoryListener::log_request_start in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/observability/history.rs:40
  trait method scylla::observability::history::HistoryListener::log_request_success in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/observability/history.rs:43
  trait method scylla::observability::history::HistoryListener::log_request_error in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/observability/history.rs:46

--- 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-f9f0940dec71b0c6762d813af2da6b4a2578058e/2617f21fcf0fbfe17a5c5f14918ea0b21b68469e/scylla/src/observability/history.rs:40
  method log_query_success of trait HistoryListener, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-f9f0940dec71b0c6762d813af2da6b4a2578058e/2617f21fcf0fbfe17a5c5f14918ea0b21b68469e/scylla/src/observability/history.rs:43
  method log_query_error of trait HistoryListener, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-f9f0940dec71b0c6762d813af2da6b4a2578058e/2617f21fcf0fbfe17a5c5f14918ea0b21b68469e/scylla/src/observability/history.rs:46

     Summary semver requires new major version: 8 major and 0 minor checks failed
    Finished [  45.327s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  11.150s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.034s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  11.250s] (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 [  23.113s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

@muzarski muzarski force-pushed the history-listener-errors branch from 6a52f4d to 71539ec Compare December 24, 2024 07:13
@muzarski muzarski marked this pull request as ready for review December 27, 2024 05:15
@muzarski muzarski requested review from Lorak-mmk and wprzytula and removed request for Lorak-mmk December 27, 2024 05:15
@muzarski muzarski force-pushed the history-listener-errors branch from 71539ec to d7c8c96 Compare December 30, 2024 12:18
@muzarski
Copy link
Contributor Author

v2:

@muzarski muzarski requested a review from wprzytula December 30, 2024 12:22
wprzytula
wprzytula previously approved these changes Dec 31, 2024
View reviewed changes
@muzarski muzarski mentioned this pull request Jan 9, 2025
Merged
7 tasks
@muzarski muzarski force-pushed the history-listener-errors branch from d7c8c96 to 4311591 Compare January 13, 2025 11:09
@muzarski
Copy link
Contributor Author

Rebased on latest #1157 (with modules refactor)

@muzarski muzarski force-pushed the history-listener-errors branch 4 times, most recently from bba3986 to 4088051 Compare January 15, 2025 14:45
@muzarski muzarski requested a review from wprzytula January 15, 2025 17:15
@muzarski muzarski force-pushed the history-listener-errors branch from 4088051 to eeef129 Compare January 15, 2025 17:59
@muzarski
Copy link
Contributor Author

v2.1: added a regression test for the scenario where the driver would unnecessarily wait for retry_interval and do the speculative retry in case of empty plan. The test's logic is explained in the (first) commit.

@muzarski
spec_exec: flatten return type of run_request_speculative_fiber
ec8a8da 
Previously, it would return `Option<Result<_, _>>`. This option
was meant to represent an empty plan. This additional layer was unnecessary
and introduced additional noise.

In addition, notice that following part in speculative_execution::execute():
```
res = async_tasks.select_next_some() => {
    if let Some(r) = res {
        if !can_be_ignored(&r) {
            return r;
        } else {
            last_error = Some(r)
        }
    }
    if async_tasks.is_empty() && retries_remaining == 0 {
        return last_error.unwrap_or({
            Err(EMPTY_PLAN_ERROR)
        });
    }
}
```
would unnecessarily try to run remaining retries if `None` was returned. This means,
we would do the retries (and sleep in between retries) with empty plan.

Now, the empty plan error is returned instead of option, and can be handled
accordinly in `can_be_ignoed` (we decide not to ignore such error - no point
in retrying on another node - there is no other node).

I also added a regression test case.
The test's timeout is set to 5s. What we try to do, is to run a request
with speculative execution policy (6s retry interval) and a LBP that always
returns an empty plan. The expected behaviour is that the test completes in
less than 5s (no timeout occurs). Otherwise, this would imply that driver
was waiting 6s for speculative retry (which was the case before this commit).
I've run the test before the change, and it indeed fails.
@muzarski
errors: re-introduce RequestError
5b845af 
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).
@muzarski
errors: replace stringified RequestTimeout variant with two new variants
de2d259 
Introduced:
- RequestTimeout(std::time::Duration) - for requests that timed out with provided
  client timeout
- SchemaAgreementTimeout(std::time::Duration) - for schema agreement timeouts
@muzarski
errors: add RequestTimeout variant to RequestError
99dc7b6 
RequestError will be passed to HistoryListener when the request either fails
or times out.
@muzarski
history: narrow the error types accepted by HistoryListener trait
a8bc046 
- `log_attempt_error` will now accept an error representing a single
   request failure - namely `RequestAttemptError`
- `log_query_error` will now accept RequestError, which represents a definite
   request failure. This is a superset of RequestAttemptError, as it also
   contains information about potential timeout, empty plan etc.
@muzarski
history: rename QueryId -> RequestId
276d5e5
@muzarski
history: rename QueryHistory[Result] to Request*
b5883a7
@muzarski
history: adjust display for StructuredHistory to "request" terminology
46da9b5
@muzarski
history: replace remaining "query" mentions with "request"
0f15e44
@muzarski muzarski force-pushed the history-listener-errors branch from eeef129 to 0f15e44 Compare January 16, 2025 14:06
@muzarski
Copy link
Contributor Author

Rebased on main

@muzarski muzarski added the API-stability Part of the effort to stabilize the API label Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lorak-mmk Lorak-mmk Awaiting requested review from Lorak-mmk

@wprzytula wprzytula Awaiting requested review from wprzytula

At least 1 approving review is required to merge this pull request.

Assignees

@muzarski muzarski

Labels
API-stability Part of the effort to stabilize the API semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@muzarski @wprzytula