Custom timestamps as a per-execution setting #1164
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`scylla::retries` is not an intuitive name for a path. `scylla::execution::retries` should be the way to reference the module with retry policies.
This is a preparation commit for introducing new methods on Session that will allow setting custom timestamps on statements upon executing them. For now, the user-facing API still does not allow setting a custom timestamp, but the internal functions do pass the optional timestamp. The timestamp still does not reach the connection layer, but this will be adjusted in a next commit.
New methods are introduced that are fully analogous to the existing execution methods but accept a custom timestamp to be passed in the request.
New methods are introduced that are fully analogous to the existing execution methods but accept a custom timestamp to be passed in the request.
The timestamp is still not set on the statement on CQL protocol layer, but this will be adjusted in a next commit.
The connection layer now executes requests with a custom timestamp provided as an argument to a proper execution method on Session. The timestamp set directly on a statement is ignored, and will be removed in the next commit. Session's and CachingSession's tests are migrated to the new API.
A timestamp is something related to a single execution of a statement, not to the statement itself. Also, setting a timestamp on a statement inside the Batch would not set a timestamp on a Batch itself, which has shown to be a pitfall. This commit finishes the migration to the new per-execution timestamp API by removing timestamp settings from statements.
github-actions
bot
added
the
semver-checks-breaking
|
See the following report for details: cargo semver-checks output |
|
To be discussed after 1.0. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note: this is based on #1163 not to incur the rebasing effort when #1163 gets merged.
Please start review with the commit
session: pass custom timestamp in request execution APIs.Motivation
As noted in #262, a timestamp is something related to a single execution of a statement, not to the statement itself. Also, setting a timestamp on a statement inside a Batch would not set a timestamp on the Batch itself, which has shown to be a pitfall.
What's done
This PR introduces a new per-execution timestamp APIs on
SessionandCachingSession, by duplicating all execution methods and making the second version accept a custom timeout. For example, we hadSession::query_single_page(query, values), so now we haveSession::query_single_page_with_timestamp(query, values, timestamp), too.The old API involving having timestamp on statements is removed. Statements can no longer hold a timestamp.
Non-viable alternatives
The original idea of @Lorak-mmk to leverage the
IntoFuturetrait to avoid API duplication showed to not be an option for us, becauseIntoFuturerequires specifying (i.e., naming) the type of the future, which in our case is an anonymous type (being a future driving theasyncblock of executing the statement).Fixes: #262
Pre-review checklist
[ ] I added relevant tests for new features and bug fixes.[ ] I have adjusted the documentation in./docs/source/.Fixes:annotations to PR description.