-
Notifications
You must be signed in to change notification settings - Fork 108
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
Aid in migration to the new deserialization API #1121
Merged
wprzytula
merged 4 commits into
scylladb:main
from
wprzytula:deserialization-api-migration-aid
Nov 13, 2024
Merged
Aid in migration to the new deserialization API #1121
wprzytula
merged 4 commits into
scylladb:main
from
wprzytula:deserialization-api-migration-aid
Nov 13, 2024
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
wprzytula
force-pushed
the
deserialization-api-migration-aid
branch
from
November 12, 2024 19:46
a747219
to
5a5298f
Compare
wprzytula
force-pushed
the
deserialization-api-migration-aid
branch
from
November 12, 2024 19:50
5a5298f
to
dad84ac
Compare
github-actions
bot
added
the
semver-checks-breaking
cargo-semver-checks reports that this PR introduces breaking API changes
label
Nov 12, 2024
See the following report for details: cargo semver-checks output
|
Lorak-mmk
requested changes
Nov 13, 2024
wprzytula
force-pushed
the
deserialization-api-migration-aid
branch
from
November 13, 2024 14:43
dad84ac
to
7b3838e
Compare
Lorak-mmk
previously approved these changes
Nov 13, 2024
muzarski
reviewed
Nov 13, 2024
The Cluster struct by itself only serves as a facade for the ClusterWorker, i.e. it has channels that allow sending requests to the worker, receives the ClusterData via the `data` field etc. Apart from the `_worker_handle` field, all other fields are cloneable. Two tasks working on two copies of the same Cluster object should behave the same as if they shared and operated on a single Cluster object (e.g. via Arc<Cluster>). This commit makes the Cluster object cloneable - the `_worker_handle` is shared via an Arc. This will be very useful in the next commit - we will do a similar thing for the GenericSession object. Co-authored-by: Wojciech Przytuła <[email protected]>
In order to make migration from the old API easier and allow doing it gradually, some components of the client programs would probably like to use the old API while the new components will use the new API. However, in the current design, Session and LegacySession are two different types and it's not possible to "cast" one to another - even though they have nearly the same fields and implementations. The previous commit made Cluster cloneable, based on the observation that it's perfectly fine to clone Cluster's fields, construct a new one and treat it as a shared facade, handle to the same "semantic" cluster. The same applies to Session, actually - cloning a session would have similar effect (though we encourage users to keep Session in an Arc so that cloning is cheaper). Instead of making GenericSession cloneable, we introduce methods which, in reality, perform a clone but change the kind of session's API. This allows to have two session objects which share the same resources but have different APIs. This should be very useful when migrating large projects to the new API - components that need to use the new API can just "convert" the session to the new interface and use that.
muzarski
approved these changes
Nov 13, 2024
As we want to phase out the legacy deserialization API, let's yell at users still using it, by use of the amazing #[deprecated] annotation, the greatest friend of a Rust lib maintainer.
Rebased on main after #1117 was merged. |
wprzytula
force-pushed
the
deserialization-api-migration-aid
branch
from
November 13, 2024 15:44
7b3838e
to
85534c5
Compare
Lorak-mmk
previously approved these changes
Nov 13, 2024
Adds a document that should help adjust users to the new deserialization API. Co-authored-by: Wojciech Przytuła <[email protected]>
wprzytula
force-pushed
the
deserialization-api-migration-aid
branch
from
November 13, 2024 20:24
85534c5
to
b7d1dfe
Compare
Lorak-mmk
approved these changes
Nov 13, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
area/deserialization
semver-checks-breaking
cargo-semver-checks reports that this PR introduces breaking API changes
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.
Motivation
Let's reduce friction when migrating to the new deserialization API.
What's done
Deprecations
Entities (traits and structs) belonging to the old deserialization API, which we plan to remove soon, are labeled with
#[deprecated]
attribute. This way they will trigger warnings when used, urging users to consider migration.Convenient switching between the legacy & the new Session's API
There already are
Session
andLegacySession
. However, a viable migration strategy involves incremental migration from the old API to the new API. In such case, it makes sense to have cheap conversions between Session APIs, using the same data underneath. This is done:LegacySession
is convertible toSession
and vice versa (and they share the sameCluster
through anArc
).Migration guide
A migration guide is added. It briefly describes the old and the new deserialization traits and compares them. Then, it summarises what changes are needed to adjust to the new deserialization framework entities.
Fixes: #965
Pre-review checklist
./docs/source/
.Fixes:
annotations to PR description.