Token: add constructor and normalization #948

Lorak-mmk · 2024-03-05T15:21:05Z

i64::MIN turns out to be an invalid value for a token: https://github.com/scylladb/scylladb/blob/4be70bfc2bc7f133cab492b4aac7bab9c790a48c/dht/token.hh#L32
Hashing algorithm should change i64::MIN result to i64::MAX.
In order to have this check in one place and make sure we always perform it I made value field of Token non-public and created constructor and getter for it.

Fixes: #947

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.

github-actions · 2024-03-05T15:25:58Z

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

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev edfb28a04c609a0ebf1813797966b39bbbb3029c
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev edfb28a04c609a0ebf1813797966b39bbbb3029c
     Cloning edfb28a04c609a0ebf1813797966b39bbbb3029c
     Parsing scylla v0.12.0 (current)
      Parsed [  17.163s] (current)
     Parsing scylla v0.12.0 (baseline)
      Parsed [  17.329s] (baseline)
    Checking scylla v0.12.0 -> v0.12.0 (no change)
     Checked [   0.477s] 67 checks; 65 passed, 2 failed, 0 unnecessary

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

Failed in:
  field value of struct Token, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-edfb28a04c609a0ebf1813797966b39bbbb3029c/f589045e3f86a4a0964789e2de12a7e48f112b11/scylla/src/routing.rs:9

--- failure struct_pub_field_now_doc_hidden: pub struct field is now #[doc(hidden)] ---

Description:
A pub field of a pub struct is now marked #[doc(hidden)] and is no longer part of the public API.
        ref: https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html#hidden
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.30.0/src/lints/struct_pub_field_now_doc_hidden.ron

Failed in:
  field Token.value in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/routing.rs:20
     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  35.012s] scylla
     Parsing scylla-cql v0.1.0 (current)
      Parsed [   9.350s] (current)
     Parsing scylla-cql v0.1.0 (baseline)
      Parsed [   9.545s] (baseline)
    Checking scylla-cql v0.1.0 -> v0.1.0 (no change)
     Checked [   0.245s] 67 checks; 67 passed, 0 unnecessary
    Finished [  19.176s] scylla-cql
make: *** [Makefile:53: semver-rev] Error 1

scylla/src/routing.rs

github-actions · 2024-03-06T11:36:24Z

cargo semver-checks detected some API incompatibilities in this PR.
See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev da49a7dedd48a0a2dd7d24e8d1578deeb96735b2
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev da49a7dedd48a0a2dd7d24e8d1578deeb96735b2
     Cloning da49a7dedd48a0a2dd7d24e8d1578deeb96735b2
     Parsing scylla v0.12.0 (current)
      Parsed [  18.389s] (current)
     Parsing scylla v0.12.0 (baseline)
      Parsed [  17.686s] (baseline)
    Checking scylla v0.12.0 -> v0.12.0 (no change)
     Checked [   0.060s] 62 checks; 60 passed, 2 failed, 0 unnecessary

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

Failed in:
  field value of struct Token, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-da49a7dedd48a0a2dd7d24e8d1578deeb96735b2/d6c3d8eb772e78bdbd5766c1009f259929e1fe1c/scylla/src/routing.rs:9

--- failure struct_pub_field_now_doc_hidden: pub struct field is now #[doc(hidden)] ---

Description:
A pub field of a pub struct is now marked #[doc(hidden)] and is no longer part of the public API.
        ref: https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html#hidden
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.29.1/src/lints/struct_pub_field_now_doc_hidden.ron

Failed in:
  field Token.value in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/routing.rs:8
     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  36.179s] scylla
     Parsing scylla-cql v0.1.0 (current)
      Parsed [   9.485s] (current)
     Parsing scylla-cql v0.1.0 (baseline)
      Parsed [   9.621s] (baseline)
    Checking scylla-cql v0.1.0 -> v0.1.0 (no change)
     Checked [   0.054s] 62 checks; 62 passed, 0 unnecessary
    Finished [  19.197s] scylla-cql
make: *** [Makefile:53: semver-rev] Error 1

scylla/src/routing.rs

wprzytula · 2024-03-06T12:56:03Z

scylla/src/routing.rs

+    #[inline]
+    pub fn new_denormalized(value: i64) -> Self {
+        Self { value }
+    }


Do we really want to expose new_denormalized?
By default, I'm against exposing functions unless we have a strong reason to do so.

I exposed all methods because before the change the field itself was public. We could restrict this method (maybe new() too?), but I'd like to know what users need Token for. Is there any plausible scenario where they need to create a new token?

I can't think of any such scenario. WDYT @piodul?

Looking at the code below, it seems there is one case where you call new_denormalize() but only for the MIN value itself. So I think instead of new_denormalized(i64) you should have just a new_invalid() which returns an invalid token - it it64:min.

That's right, we either want to create a valid token (everywhere else) or specifically an invalid token (i64::MIN).

I'm worried that a year from now, nobody will remember what a "denormalized" token means. So you'll either need to write a comment explaining what this constructor means and when to use it - or just not have this constructor and have a separate new_invalid() just for the special case of min_token which is easy to understand what it means. If it's enough...
BTW, If I understand correctly, an "invalid token" (min_int) will never be part of any token range.

I'm not sure anything comes to my mind as far as real use cases for users go for new_denormalized. For our internal use at least we will have to represent token ranges somehow, specifically their left ends. I don't know whether this concern for users.

What I meant is: are there are scenarios where user needs to create a new token from integer? I'm not talking just about invalid tokens, but any tokens at all. @piodul
I'm asking because I'm wondering if we can make all constructors private.

I don't know, but I'd err on the more cautious side and allow creating tokens from integers.

Do we have any APIs in the driver that accept Token?

I don't know, but I'd err on the more cautious side and allow creating tokens from integers.

Do we have any APIs in the driver that accept Token?

There are some:

https://docs.rs/scylla/latest/scylla/routing/struct.Sharder.html#method.shard_of
https://docs.rs/scylla/latest/scylla/transport/locator/struct.TokenRing.html#method.get_elem_for_token
https://docs.rs/scylla/latest/scylla/transport/locator/struct.TokenRing.html#method.ring_range
https://docs.rs/scylla/latest/scylla/transport/locator/struct.TokenRing.html#method.ring_range_full
https://docs.rs/scylla/latest/scylla/transport/locator/struct.ReplicaLocator.html#method.replicas_for_token
https://docs.rs/scylla/latest/scylla/transport/struct.ClusterData.html#method.get_token_endpoints

I found them by search Token on docs.rs and going to "In Parameters" tab - judging by the results I got I think those should be exhaustive.

So the constructor needs to remain public I think. Not sure about INVALID constant (previously new_unchecked). We can probably make it private?

github-actions · 2024-03-06T20:11:05Z

cargo semver-checks detected some API incompatibilities in this PR.
See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev da49a7dedd48a0a2dd7d24e8d1578deeb96735b2
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev da49a7dedd48a0a2dd7d24e8d1578deeb96735b2
     Cloning da49a7dedd48a0a2dd7d24e8d1578deeb96735b2
     Parsing scylla v0.12.0 (current)
      Parsed [  20.334s] (current)
     Parsing scylla v0.12.0 (baseline)
      Parsed [  19.228s] (baseline)
    Checking scylla v0.12.0 -> v0.12.0 (no change)
     Checked [   0.063s] 62 checks; 60 passed, 2 failed, 0 unnecessary

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

Failed in:
  field value of struct Token, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-da49a7dedd48a0a2dd7d24e8d1578deeb96735b2/d6c3d8eb772e78bdbd5766c1009f259929e1fe1c/scylla/src/routing.rs:9

--- failure struct_pub_field_now_doc_hidden: pub struct field is now #[doc(hidden)] ---

Description:
A pub field of a pub struct is now marked #[doc(hidden)] and is no longer part of the public API.
        ref: https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html#hidden
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.29.1/src/lints/struct_pub_field_now_doc_hidden.ron

Failed in:
  field Token.value in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/routing.rs:20
     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  39.679s] scylla
     Parsing scylla-cql v0.1.0 (current)
      Parsed [  10.468s] (current)
     Parsing scylla-cql v0.1.0 (baseline)
      Parsed [  10.571s] (baseline)
    Checking scylla-cql v0.1.0 -> v0.1.0 (no change)
     Checked [   0.057s] 62 checks; 62 passed, 0 unnecessary
    Finished [  21.141s] scylla-cql
make: *** [Makefile:53: semver-rev] Error 1

Lorak-mmk · 2024-03-06T21:38:42Z

I added doc comments for Token and it's new methods.

CI fails because of #950 . After #951 is merged I'll rebase on main.

github-actions · 2024-03-07T15:07:15Z

cargo semver-checks detected some API incompatibilities in this PR.
See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev 44785bd9782427051ef0a39c33bed46a1a43520a
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 44785bd9782427051ef0a39c33bed46a1a43520a
     Cloning 44785bd9782427051ef0a39c33bed46a1a43520a
     Parsing scylla v0.12.0 (current)
      Parsed [  20.340s] (current)
     Parsing scylla v0.12.0 (baseline)
      Parsed [  18.598s] (baseline)
    Checking scylla v0.12.0 -> v0.12.0 (no change)
     Checked [   0.063s] 62 checks; 60 passed, 2 failed, 0 unnecessary

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

Failed in:
  field value of struct Token, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-44785bd9782427051ef0a39c33bed46a1a43520a/10030a018fc40eeb43bb587316f1e047062ac65c/scylla/src/routing.rs:9

--- failure struct_pub_field_now_doc_hidden: pub struct field is now #[doc(hidden)] ---

Description:
A pub field of a pub struct is now marked #[doc(hidden)] and is no longer part of the public API.
        ref: https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html#hidden
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.29.1/src/lints/struct_pub_field_now_doc_hidden.ron

Failed in:
  field Token.value in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/routing.rs:20
     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  39.054s] scylla
     Parsing scylla-cql v0.1.0 (current)
      Parsed [  10.531s] (current)
     Parsing scylla-cql v0.1.0 (baseline)
      Parsed [  10.463s] (baseline)
    Checking scylla-cql v0.1.0 -> v0.1.0 (no change)
     Checked [   0.054s] 62 checks; 62 passed, 0 unnecessary
    Finished [  21.087s] scylla-cql
make: *** [Makefile:53: semver-rev] Error 1

github-actions · 2024-03-09T21:25:05Z

cargo semver-checks detected some API incompatibilities in this PR.
See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev 7648b1b4108e1f44babfdf13da872b2680e3d7dd
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 7648b1b4108e1f44babfdf13da872b2680e3d7dd
     Cloning 7648b1b4108e1f44babfdf13da872b2680e3d7dd
     Parsing scylla v0.12.0 (current)
      Parsed [  19.235s] (current)
     Parsing scylla v0.12.0 (baseline)
      Parsed [  17.819s] (baseline)
    Checking scylla v0.12.0 -> v0.12.0 (no change)
     Checked [   0.062s] 62 checks; 60 passed, 2 failed, 0 unnecessary

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

Failed in:
  field value of struct Token, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-7648b1b4108e1f44babfdf13da872b2680e3d7dd/00aea99dfa0cca5f6de2c2351fb89cc1b6194e45/scylla/src/routing.rs:9

--- failure struct_pub_field_now_doc_hidden: pub struct field is now #[doc(hidden)] ---

Description:
A pub field of a pub struct is now marked #[doc(hidden)] and is no longer part of the public API.
        ref: https://doc.rust-lang.org/rustdoc/write-documentation/the-doc-attribute.html#hidden
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.29.1/src/lints/struct_pub_field_now_doc_hidden.ron

Failed in:
  field Token.value in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/routing.rs:20
     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  37.161s] scylla
     Parsing scylla-cql v0.1.0 (current)
      Parsed [   9.890s] (current)
     Parsing scylla-cql v0.1.0 (baseline)
      Parsed [   9.855s] (baseline)
    Checking scylla-cql v0.1.0 -> v0.1.0 (no change)
     Checked [   0.055s] 62 checks; 62 passed, 0 unnecessary
    Finished [  19.839s] scylla-cql
make: *** [Makefile:53: semver-rev] Error 1

Lorak-mmk · 2024-03-25T19:23:17Z

Rebased on main
Changed new_denormalized constructor to INVALID associated constant

wprzytula

LGTM

scylla/src/routing.rs

`i64::MIN` turns out to be an invalid value for a token: https://github.com/scylladb/scylladb/blob/4be70bfc2bc7f133cab492b4aac7bab9c790a48c/dht/token.hh#L32 Hashing algorithm should change `i64::MIN` result to `i64::MAX`. In order to have this check in one place and make sure we always perform it I made `value` field of `Token` non-public and created constructor and getter for it.

Lorak-mmk requested review from piodul and wprzytula March 5, 2024 15:21

Lorak-mmk self-assigned this Mar 5, 2024

github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Mar 5, 2024

wprzytula reviewed Mar 6, 2024

View reviewed changes

scylla/src/routing.rs Outdated Show resolved Hide resolved

Lorak-mmk force-pushed the fix-min-token branch from ad1ebed to 500d1ca Compare March 6, 2024 11:31

wprzytula requested changes Mar 6, 2024

View reviewed changes

Lorak-mmk force-pushed the fix-min-token branch 3 times, most recently from ca43793 to 3425851 Compare March 6, 2024 20:05

Lorak-mmk force-pushed the fix-min-token branch from 3425851 to 2c88d14 Compare March 7, 2024 15:02

Lorak-mmk force-pushed the fix-min-token branch from 2c88d14 to 813f529 Compare March 9, 2024 21:20

Lorak-mmk added the waiting-on-author Waiting for a response from issue/PR author label Mar 14, 2024

Lorak-mmk force-pushed the fix-min-token branch from 813f529 to 9f2c5b6 Compare March 25, 2024 19:22

Lorak-mmk removed the waiting-on-author Waiting for a response from issue/PR author label Mar 25, 2024

wprzytula self-requested a review March 26, 2024 20:25

Lorak-mmk force-pushed the fix-min-token branch from 9f2c5b6 to 049dc35 Compare March 26, 2024 21:35

wprzytula approved these changes Mar 27, 2024

View reviewed changes

scylla/src/routing.rs Outdated Show resolved Hide resolved

Lorak-mmk force-pushed the fix-min-token branch from 049dc35 to 7b7592d Compare March 27, 2024 09:49

Lorak-mmk requested a review from wprzytula March 27, 2024 09:49

wprzytula approved these changes Mar 27, 2024

View reviewed changes

wprzytula merged commit a092845 into scylladb:main Mar 27, 2024
11 checks passed

Lorak-mmk mentioned this pull request May 9, 2024

Release 0.13 #995

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Token: add constructor and normalization #948

Token: add constructor and normalization #948

Lorak-mmk commented Mar 5, 2024 •

edited

Loading

github-actions bot commented Mar 5, 2024 •

edited

Loading

github-actions bot commented Mar 6, 2024

wprzytula Mar 6, 2024

Lorak-mmk Mar 6, 2024

wprzytula Mar 7, 2024

nyh Mar 10, 2024

wprzytula Mar 10, 2024

nyh Mar 10, 2024

Lorak-mmk Mar 18, 2024

piodul Mar 18, 2024

Lorak-mmk Mar 25, 2024

Lorak-mmk Mar 25, 2024 •

edited

Loading

github-actions bot commented Mar 6, 2024

Lorak-mmk commented Mar 6, 2024 •

edited

Loading

github-actions bot commented Mar 7, 2024

github-actions bot commented Mar 9, 2024

Lorak-mmk commented Mar 25, 2024

wprzytula left a comment

Token: add constructor and normalization #948

Token: add constructor and normalization #948

Conversation

Lorak-mmk commented Mar 5, 2024 • edited Loading

Pre-review checklist

github-actions bot commented Mar 5, 2024 • edited Loading

github-actions bot commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lorak-mmk Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Mar 6, 2024

Lorak-mmk commented Mar 6, 2024 • edited Loading

github-actions bot commented Mar 7, 2024

github-actions bot commented Mar 9, 2024

Lorak-mmk commented Mar 25, 2024

wprzytula left a comment

Choose a reason for hiding this comment

Lorak-mmk commented Mar 5, 2024 •

edited

Loading

github-actions bot commented Mar 5, 2024 •

edited

Loading

Lorak-mmk Mar 25, 2024 •

edited

Loading

Lorak-mmk commented Mar 6, 2024 •

edited

Loading