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

errors: Narrow return error type of Session::[query/execute]_iter #1191

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

Conversation

muzarski
Copy link
Contributor

@muzarski muzarski commented Feb 3, 2025

Ref: #519

Motivation

NextPageError and NextRowError were introduced specifically for QueryPager API. Currently (before this PR), the top-level Session methods [execute/query]_iter() return a ExecutionError. This is why NextRowError variant is present in ExecutionError. To remove that, we can narrow the return error type of these methods.

There is one subtle thing to pay attention to (and potentially discuss). query_iter method potentially prepares the statement (if values are not empty). Session::prepare currently returns a ExecutionError. This is why newly introduced PagerExecutionError now depends on ExecutionError. The question is whether we should narrow the return error type of Session::prepare to some new error type (PrepareError), or leave it as is.

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 changed the title Narrow return error type of Session::[query/execute]_iter errors: Narrow return error type of Session::[query/execute]_iter Feb 3, 2025
@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Feb 3, 2025
Copy link

github-actions bot commented Feb 3, 2025

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

See the following report for details:

cargo semver-checks output
./scripts/semver-checks.sh --baseline-rev e66f2e301f1d2fa2417ea0c4253c640fdafca296
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev e66f2e301f1d2fa2417ea0c4253c640fdafca296
     Cloning e66f2e301f1d2fa2417ea0c4253c640fdafca296
    Building scylla v0.15.0 (current)
       Built [  22.314s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.049s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  22.587s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.047s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.117s] 127 checks: 126 pass, 1 fail, 0 warn, 0 skip

--- 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.39.0/src/lints/enum_variant_missing.ron

Failed in:
  variant ExecutionError::NextRowError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-e66f2e301f1d2fa2417ea0c4253c640fdafca296/7b063ee919658594438fa16b70e51f4f1e9251a1/scylla/src/errors.rs:120

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  46.079s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  11.238s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.028s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  11.329s] (baseline)
     Parsing scylla-cql v0.4.0 (baseline)
      Parsed [   0.027s] (baseline)
    Checking scylla-cql v0.4.0 -> v0.4.0 (no change)
     Checked [   0.109s] 127 checks: 127 pass, 0 skip
     Summary no semver update required
    Finished [  23.352s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

@muzarski
Copy link
Contributor Author

muzarski commented Feb 3, 2025

Flaky test_coalescing fails yet again for Cassandra (#864).

@wprzytula
Copy link
Collaborator

wprzytula commented Feb 3, 2025

There is one subtle thing to pay attention to (and potentially discuss). query_iter method potentially prepares the statement (if values are not empty). Session::prepare currently returns a QueryError. This is why newly introduced PagerExecutionError now depends on QueryError. The question is whether we should narrow the return error type of Session::prepare to some new error type (PrepareError), or leave it as is.

Now that we renamed QueryError to ExecutionError, it's even clearer that PrepareError should be introduced - because preparing a statement is different from executing a statement.

I believe PrepareError should not contain such variants of ExecutionError as:

  • MetadataError (preparing a statement never requires awaiting schema agreement),
  • RequestTimeout (preparing does not support client-side timeouts AFAIK),
  • SchemaAgreementTimeout (as said, no awaiting schema agreement after preparing),
  • UseKeyspaceError (executing USE KEYSPACE is not possible during preparation),
  • NextRowError (no rows result comes from preparation).

wprzytula
wprzytula previously approved these changes Feb 3, 2025
Copy link
Collaborator

@wprzytula wprzytula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but see my comment about introducing PrepareError.

@wprzytula wprzytula added this to the 0.16.0 milestone Feb 3, 2025
@muzarski
Copy link
Contributor Author

muzarski commented Feb 3, 2025

Now that we renamed QueryError to ExecutionError, it's even clearer that PrepareError should be introduced - because preparing a statement is different from executing a statement.

I 100% agree. New name clearly suggests that we should distinguish these two kinds of operations.

* `NextRowError` (no rows result comes from preparation).

BTW, NextRowError is not present in ExecutionError after this PR.

@muzarski
Copy link
Contributor Author

muzarski commented Feb 3, 2025

LGTM, but see my comment about introducing PrepareError.

Opened a PR #1193

@muzarski muzarski added the API-stability Part of the effort to stabilize the API label Feb 3, 2025
scylla/src/errors.rs Outdated Show resolved Hide resolved
@muzarski
Copy link
Contributor Author

muzarski commented Feb 4, 2025

Rebased on main

Last time during pager API errors refactor I missed that this return error
type can be narrowed.

There are two kinds of callers:
1. Session::query/execute_iter
In this case, we needed to replace `QueryError::NextRowError` variant
with NextPageError. However, this variant will be removed at the end of this PR as
we introduce a new error type for aforementioned Session methods.

2. Connection::query/execute_iter
This is an internal API. It's only used during metadata fetch. I decided to
map it to `NextRowError`, because it doesn't make much sense to distinguish between these two
from user's perspective. NextRowError will be returned later anyway, when we iterate over the rows.
It's an error returned by `[Caching]Session::[query/execute]_iter`.
@muzarski muzarski force-pushed the query-pager-public-error branch from 8c46971 to 4289ea3 Compare February 4, 2025 13:58
@muzarski
Copy link
Contributor Author

muzarski commented Feb 4, 2025

Rebased on main again (ssl CI fix, and removed legacy ser/deser api)

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

Successfully merging this pull request may close these issues.

3 participants