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

Partitioner visibility #1223

Merged
merged 4 commits into from
Feb 11, 2025

Conversation

wprzytula
Copy link
Collaborator

Clone of #1222.

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.

There are two reasons to do so.
First, users may be interested which partitioner is used for a specific
PreparedStatement.
Second, tests in session_test.rs call `get_partitioner_name()` to assert
some things. If we want to extract `session_test.rs` to integration
tests directory - out of `scylla` crate - we must therefore expose
`get_partitioner_name().

NOTE: PartitionerHasherAny is made `pub` in this commit. This is a
temporary measure. This is needed because otherwise the compiler errors
out that we leak a crate-private type in public interface (the
associated type in `impl Partitioner for PartitionerName`).
We will be able to revert this once we unpub `Partitioner(Hasher)`.
As `PartitionerName` is now public and retrievable from
`PreparedStatement` using `get_partitioner_name()`, it makes sense to
accept it in `calculate_token_for_partition_key` instead of an
`impl Partitioner` generic parameter.
We do not expect users to implement their own Partitioners nor use their
methods. Therefore, `Partitioner` trait and its implementors,
`Murmur3Partitioner` and `CDCPartitioner`, are made `pub(crate)`.
Same with `PartitionerHasher` trait and its implementors.
@wprzytula wprzytula requested a review from Lorak-mmk February 11, 2025 11:41
@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Feb 11, 2025
Copy link

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

See the following report for details:

cargo semver-checks output
./scripts/semver-checks.sh --baseline-rev f6d37a628c2cef393fe7e13f8b2fc4220c0154fd
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev f6d37a628c2cef393fe7e13f8b2fc4220c0154fd
     Cloning f6d37a628c2cef393fe7e13f8b2fc4220c0154fd
    Building scylla v0.15.0 (current)
       Built [  24.655s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.048s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  21.985s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.046s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.124s] 127 checks: 124 pass, 3 fail, 0 warn, 0 skip

--- failure function_requires_different_generic_type_params: function now requires a different number of generic type parameters ---

Description:
A function now requires a different number of generic type parameters than it used to. Uses of this function that supplied the previous number of generic types will be broken.
        ref: https://doc.rust-lang.org/reference/items/generics.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.39.0/src/lints/function_requires_different_generic_type_params.ron

Failed in:
  function calculate_token_for_partition_key (1 -> 0 generic types) in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/routing/partitioner.rs:351

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

Failed in:
  struct scylla::routing::partitioner::Murmur3Partitioner, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-f6d37a628c2cef393fe7e13f8b2fc4220c0154fd/09415b9adb445d3b3772cc894263c395b09e4230/scylla/src/routing/partitioner.rs:100
  struct scylla::routing::partitioner::Murmur3PartitionerHasher, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-f6d37a628c2cef393fe7e13f8b2fc4220c0154fd/09415b9adb445d3b3772cc894263c395b09e4230/scylla/src/routing/partitioner.rs:115
  struct scylla::routing::partitioner::CDCPartitionerHasher, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-f6d37a628c2cef393fe7e13f8b2fc4220c0154fd/09415b9adb445d3b3772cc894263c395b09e4230/scylla/src/routing/partitioner.rs:283
  struct scylla::routing::partitioner::CDCPartitioner, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-f6d37a628c2cef393fe7e13f8b2fc4220c0154fd/09415b9adb445d3b3772cc894263c395b09e4230/scylla/src/routing/partitioner.rs:281

--- failure trait_missing: pub trait removed or renamed ---

Description:
A publicly-visible trait cannot be imported by its prior path. A `pub use` may have been removed, or the trait 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.39.0/src/lints/trait_missing.ron

Failed in:
  trait scylla::routing::partitioner::Partitioner, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-f6d37a628c2cef393fe7e13f8b2fc4220c0154fd/09415b9adb445d3b3772cc894263c395b09e4230/scylla/src/routing/partitioner.rs:78
  trait scylla::routing::partitioner::PartitionerHasher, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-f6d37a628c2cef393fe7e13f8b2fc4220c0154fd/09415b9adb445d3b3772cc894263c395b09e4230/scylla/src/routing/partitioner.rs:95

     Summary semver requires new major version: 3 major and 0 minor checks failed
    Finished [  47.848s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  11.068s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.028s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  10.817s] (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.110s] 127 checks: 127 pass, 0 skip
     Summary no semver update required
    Finished [  22.554s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

@wprzytula wprzytula self-assigned this Feb 11, 2025
@wprzytula wprzytula merged commit a86d390 into scylladb:branch-hackathon Feb 11, 2025
11 checks passed
@wprzytula wprzytula deleted the partitioner-visibility branch February 11, 2025 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

1 participant