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

Query paging API: hardening, robustness, explicitness #1061

Merged
merged 19 commits into from
Aug 27, 2024

Conversation

wprzytula
Copy link
Collaborator

@wprzytula wprzytula commented Aug 20, 2024

Motivation

Query paging API has shown to be unclear for users:

  • {query,execute}_paged had misleading name (query_paged is a misleading name #940),
  • paging_state was a magic Bytes instance (Make paging state strongly typed #987),
  • {query,execute} would silently return only the first page if page size was specified on a statement,
  • users would naturally resort to {query,execute} as the methods with the simplest names, even though they should be used with SELECTs with caution:
  • with no page size set, an unpaged SELECT query would have high latency and memory footprint, and could event time out,
  • with page size set, only first page would be returned (see the previous big point).
  • it was easy to forget about iterating over pages based on paging_state, as it was just a field on QueryResult,
  • there was lack of examples of a loop together with {query,execute}_paged.

This PR seeks to address those problems before we attack the Giant Request Execution Refactor™ (GRER™) (#978).

What's done

The core idea incorporated:

Shift responsibility about whether to page or not from statements (by setting page_size to Some or None) to Session's methods.

Separate paged and unpaged API promotes conscious choice by users and thus prevents API misuse.

Detailed changes

  1. Strongly typed PageSize is introduced, which restricts the inner i32's range to positive integers. This is important, because passing nonpositive integer as a page size silently makes the query unpaged. All APIs now accept PageSize, and it's user's responsibility to create a PageSize, handling possible errors thrown upon its construction.
  2. Paging state is made strongly typed. Two new types are introduced: PagingState and PagingStateResponse. PagingState is, underneath, just an Option<Arc> over the raw bytes from the DB. The idea is that PagingState is passed as an argument to request execution API to ask for resuming the query (continuing it) from the point specified by given PagingState. PagingStateResponse is returned always by the DB from paged requests - with information about either more or no more pages being available. PagingStateResponse is easily convertible into PagingState, so that user can use it in in a loop.
  3. PageSize is made mandatory on statements. Henceforth, paged API will always use it and unpaged API will always ignore it.
  4. {query,execute} methods have _unpaged prefix appended and they always perform an unpaged query (ignoring the PageSize specified on a statement).
  5. {query,execute}_paged are renamed to {query,execute}_single_page. The name %_paged has long confused users. The new name indicates explicitly that only a single page is fetched with a single call to those methods.
  6. {query,execute}_single_page pass page state explicitly. As an attempt to make paged queries' API more explicit, robust and self-explanatory, PagingStateResponse is decoupled from QueryResult. Instead, it's returned from {query,execute}_single_page as a second field of a pair, helping users not to forget about it.
    Moreover, internal and external APIs which expect that the query is not paged now warn if a "more pages" page state is returned from a query.
  7. select-paging example is modified to showcase the suggested way to use {query,execute}_single_page.
  8. Documentation is adjusted to the introduced changes.

Fixes: #940
Fixes: #987

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.

@wprzytula wprzytula self-assigned this Aug 20, 2024
@wprzytula wprzytula added area/statement-execution area/documentation Improvements or additions to documentation labels Aug 20, 2024
@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Aug 20, 2024
Copy link

github-actions bot commented Aug 20, 2024

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

See the following report for details:

cargo semver-checks output
./scripts/semver-checks.sh --baseline-rev 78286c3f7cbe5067d16c70597b4c87c582e0c5d9
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 78286c3f7cbe5067d16c70597b4c87c582e0c5d9
     Cloning 78286c3f7cbe5067d16c70597b4c87c582e0c5d9
     Parsing scylla v0.13.0 (current)
      Parsed [  21.567s] (current)
     Parsing scylla v0.13.0 (baseline)
      Parsed [  20.000s] (baseline)
    Checking scylla v0.13.0 -> v0.13.0 (no change)
     Checked [   0.076s] 79 checks: 77 pass, 2 fail, 0 warn, 0 skip

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn 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.34.0/src/lints/inherent_method_missing.ron

Failed in:
  Session::query, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/session.rs:620
  Session::query_paged, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/session.rs:639
  Session::execute, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/session.rs:973
  Session::execute_paged, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/session.rs:987
  Session::query, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/session.rs:620
  Session::query_paged, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/session.rs:639
  Session::execute, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/session.rs:973
  Session::execute_paged, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/session.rs:987
  PreparedStatement::disable_paging, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/statement/prepared_statement.rs:161
  PreparedStatement::disable_paging, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/statement/prepared_statement.rs:161
  CachingSession::execute, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/caching_session.rs:72
  CachingSession::execute_paged, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/caching_session.rs:94
  Query::disable_paging, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/statement/query.rs:43
  Query::disable_paging, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/statement/query.rs:43

--- 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.34.0/src/lints/struct_pub_field_missing.ron

Failed in:
  field paging_state of struct QueryResult, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/query_result.rs:23
  field paging_state of struct QueryResult, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-78286c3f7cbe5067d16c70597b4c87c582e0c5d9/7fd5bb76cdb30117cc4c1f6bc25a636a39d2cb6e/scylla/src/transport/query_result.rs:23

     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  41.689s] scylla
     Parsing scylla-cql v0.2.0 (current)
      Parsed [   9.910s] (current)
     Parsing scylla-cql v0.2.0 (baseline)
      Parsed [   9.974s] (baseline)
    Checking scylla-cql v0.2.0 -> v0.2.0 (no change)
     Checked [   0.081s] 79 checks: 79 pass, 0 skip
     Summary no semver update required
    Finished [  20.014s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

@wprzytula wprzytula added this to the 0.14.0 milestone Aug 20, 2024
scylla/src/transport/query_result.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/request/query.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
scylla/src/transport/session.rs Outdated Show resolved Hide resolved
@wprzytula wprzytula force-pushed the paging-api-hardening branch from 0966edf to 76b1fae Compare August 22, 2024 16:58
@wprzytula wprzytula marked this pull request as draft August 22, 2024 16:58
@wprzytula wprzytula force-pushed the paging-api-hardening branch 4 times, most recently from 009283e to ee73b22 Compare August 23, 2024 09:39
@wprzytula wprzytula marked this pull request as ready for review August 23, 2024 09:39
@wprzytula wprzytula requested a review from Lorak-mmk August 23, 2024 09:39
Copy link
Contributor

@muzarski muzarski left a comment

Choose a reason for hiding this comment

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

Some nits, otherwise LGTM

scylla/src/statement/mod.rs Outdated Show resolved Hide resolved
docs/source/queries/result.md Show resolved Hide resolved
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.

I don't like the first commit. Using invalid values for page size was never shown to be a problem / footgun for our users, so advantages from strong typing are quite limited. Otoh it introduces unnecessary complications in each call to set_page_size.
Imo in case of such obscure error, which also implies a bug in user code, a panic wuld be better - we still catch the error, but don't introduce complications for users.
So let's drop PageSize type and perform panicking check in set_page_size methods.

PR description is out of date - it mentions PagingContinuation.

In checklist you marked that All commits compile, pass static checks and pass test. - is that really the case? I would expect commit session: make {query,execute} methods unpaged to break something if it really prevents usage of unpaged queries. If it doesn't break any test then it sounds like our tests are lacking.

examples/select-paging.rs Outdated Show resolved Hide resolved
scylla-cql/src/frame/request/query.rs Outdated Show resolved Hide resolved
scylla/src/transport/connection.rs Outdated Show resolved Hide resolved
scylla/src/transport/connection.rs Outdated Show resolved Hide resolved
scylla/src/transport/connection.rs Outdated Show resolved Hide resolved
@wprzytula
Copy link
Collaborator Author

So let's drop PageSize type and perform panicking check in set_page_size methods.

I suggest a slightly different thing: let's make PageSize not occur in the public API, but still be passed around in the internal codebase. This way we make sure that we won't forget about a panic anywhere - because we must handle an error on try_into() - I suggest unwrapping it.

@wprzytula
Copy link
Collaborator Author

In checklist you marked that All commits compile, pass static checks and pass test. - is that really the case? I would expect commit session: make {query,execute} methods unpaged to break something if it really prevents usage of unpaged queries. If it doesn't break any test then it sounds like our tests are lacking.

Good catch. Let's see if any test breaks.

@Lorak-mmk
Copy link
Collaborator

So let's drop PageSize type and perform panicking check in set_page_size methods.

I suggest a slightly different thing: let's make PageSize not occur in the public API, but still be passed around in the internal codebase. This way we make sure that we won't forget about a panic anywhere - because we must handle an error on try_into() - I suggest unwrapping it.

Makes sense

@wprzytula
Copy link
Collaborator Author

wprzytula commented Aug 26, 2024

In checklist you marked that All commits compile, pass static checks and pass test. - is that really the case? I would expect commit session: make {query,execute} methods unpaged to break something if it really prevents usage of unpaged queries. If it doesn't break any test then it sounds like our tests are lacking.

Good catch. Let's see if any test breaks.

Sad news... everything has passed.

On a second thought, it's not strange that we haven't been testing improper behaviour. After all, query and execute should have never been used for paged queries, as they don't accept a non-start paging state. The only place that could potentially break is where query/execute was used as the first query before {query/execute}_paged loop, but apparently nothing broke.

@wprzytula wprzytula force-pushed the paging-api-hardening branch from a7aa56f to 6248a1c Compare August 26, 2024 13:52
@wprzytula
Copy link
Collaborator Author

v1.1: addressed all Karol's comments apart from PageSize panics, which is TODO.

@wprzytula wprzytula force-pushed the paging-api-hardening branch from 6248a1c to 0fa3118 Compare August 26, 2024 13:55
@wprzytula wprzytula force-pushed the paging-api-hardening branch from f5b1839 to a139ca5 Compare August 26, 2024 14:31
@wprzytula
Copy link
Collaborator Author

v 1.2.1: mentioned that setters panic with non-positive numbers given.

Before, a bare i32 was passed. Some API methods would accept nonpositive
integers, while others would panic on such input. Both those kinds of
behaviour are bad, and the inconsistency between them is even worse.
For robustness the popular Rust tactics is employed, namely strong
typing. PageSize's public constructor returns error for nonpositive
integers, ensuring that is an instance of that type exists, its validity
is guaranteed.
I originally wanted to put burden of creating a correct PageSize and
handle possible errors on user, but is would introduce a boilerplate
(`try_into().unwrap()`) that is deemed to be unacceptable. Therefore,
PageSize is kept pub(crate) and it's constructed by driver's code,
panicking on invalid user's input. As page size is always chosen by user
explicitly (not by some convoluted logic), such panics should appear
early and be clear to users.
The value of everything being empty does not seem to be a reasonable
default value for both QueryResult and ResultMetadata. It's better
to have a mock_empty() constructor for the specific case when such
special value is needed.
As an effort of more robust paging API, paging state is now made typed.
Two new types are introduced: PagingState and PagingStateResponse.
PagingState is, underneath, just an Arc over the raw bytes from
the DB. The idea is that PagingState is passed as an argument to
request execution API to ask for resuming the query (_continue it_) from
the given PagingState. PagingStateResponse is returned always by the DB
from paged requests - with information about either more or no more
pages available. PagingState can be easily retrieved from
PagingStateResponse (if it contains some), so that user can use it in
a loop.
The select_paging example is modernised to showcase the idea.
As users' experience showed that it optional page size on statements
is error-prone, it is made mandatory.
This means that at the moment no unpaged queries are possible. However,
in the next commits it is going to be brought back again.
Some methods on Connection return QueryResponse instead of QueryResult.
To make those methods stand out, they get "_raw" particle put into their
names.
Without a clear reason, Connection::execute_raw would take
PreparedStatement by value, involving a clone. The signature was changed
to accept a shared reference and its usages are adjusted.
These methods are analogous to `Session`'s `{query,execute}`. Similarly,
they don't accept a non-start PagingState.
The logic of {query,execute} is extracted to {query_inner,execute_inner}
pub(crate) methods, respectively. {query,execute}_paged are unchanged.
{query,execute} now are unpaged unconditionally, i.e. they ignore the
page size set on a statement and pass None to the connection layer.
In the next commit, both {query,execute} are appended the `_unpaged`
suffix to explicitly state that they perform unpaged queries.
In order to make it explicit that the requests using those methods are
performed without paging, their names are adjusted.
As these methods are analogous to those on Session, they are made
unpaged in a manner similar to those. Inthe next commit, both all of
them are appended the `_unpaged` suffix to explicitly state that they
perform unpaged queries.
In order to make it explicit that the requests using those methods are
performed without paging, their names are adjusted.
In no case for internal queries should we fetch only one page and ignore
possible further ones. As both queries that were using query_single_page
would return only one row anyway, the change should not affect semantics
after all.
The name %_paged has long confused users. The new name indicates
explicitly that only a single page is fetched with a single call to
those methods.
As an attempt to make paged queries' API more explicit, robust and
self-explanatory, PagingState is decoupled from QueryResult. Instead,
it's returned from {query,execute}_single_page as a second field of a
pair, helping users not to forget about it.
Moreover, internal and external APIs which expect that the query is not
paged now issue an error tracing message and return ProtocolError error
if a "more pages" page state is returned from the unpaged query.
It appears that query_single_page's previous semantics was to only fetch
the first page and ignore others. It makes much more sense, though, to
have its semantics consistent with Session::query_single_page: to take
PagingState and query a single page pointed by that state. This allows
using it in a loop, similarly to Session::query_single_page.
As {query,execute} methods got the _unpaged prefix, now the inner
methods can drop the _inner prefix. This gives us one subtle advantage:
users now see "method {query,execute} is private" instead of
"{query,execute} method not found", which makes them more likely to
think that the API was changed on purpose. Assuming that users see
docstrings of private methods when they hover a call to them, this also
lets them read the new docstrings of {query,execute} that explain the
reasons behind the change and point to proper public methods:
{query,execute}_{unpaged,single_page,iter}.
After the API changes regarding paging, docstrings are updated with
those changes in mind.
@wprzytula wprzytula force-pushed the paging-api-hardening branch from a139ca5 to a1286de Compare August 27, 2024 09:55
@wprzytula
Copy link
Collaborator Author

v1.2.2: fully removed PageSize from public API - I forgot about getters - and made it pub(crate).

@wprzytula wprzytula merged commit 5752af4 into scylladb:main Aug 27, 2024
12 checks passed
@wprzytula wprzytula deleted the paging-api-hardening branch August 27, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation area/statement-execution 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.

Make paging state strongly typed query_paged is a misleading name
4 participants