Skip to content

retry_policy: replace all usages of Box<dyn RetryPolicy> with Arc<...>#1103

Merged
Lorak-mmk merged 2 commits intoscylladb:mainfrom
muzarski:retry-policy-behind-arc
Oct 24, 2024
Merged

retry_policy: replace all usages of Box<dyn RetryPolicy> with Arc<...>#1103
Lorak-mmk merged 2 commits intoscylladb:mainfrom
muzarski:retry-policy-behind-arc

Conversation

@muzarski
Copy link
Contributor

@muzarski muzarski commented Oct 24, 2024

This way, when user wants to reuse a retry policy in multiple places, there is no need for an additional allocation for Box (RetryPolicy::clone_boxed).

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.

@muzarski muzarski self-assigned this Oct 24, 2024
@github-actions
Copy link

github-actions bot commented Oct 24, 2024

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

See the following report for details:

cargo semver-checks output
./scripts/semver-checks.sh --baseline-rev aef0cf5c8dea75d878f0ae8c687213fd9fa287d7
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev aef0cf5c8dea75d878f0ae8c687213fd9fa287d7
     Cloning aef0cf5c8dea75d878f0ae8c687213fd9fa287d7
     Parsing scylla v0.14.0 (current)
      Parsed [  20.439s] (current)
     Parsing scylla v0.14.0 (baseline)
      Parsed [  19.951s] (baseline)
    Checking scylla v0.14.0 -> v0.14.0 (no change)
     Checked [   0.107s] 94 checks: 93 pass, 1 fail, 0 warn, 0 skip

--- failure trait_method_missing: pub trait method removed or renamed ---

Description:
A trait method is no longer callable, and may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#major-any-change-to-trait-item-signatures
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/trait_method_missing.ron

Failed in:
  method clone_boxed of trait RetryPolicy, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-aef0cf5c8dea75d878f0ae8c687213fd9fa287d7/28587ea95124515726f41c658488682589e689eb/scylla/src/transport/retry_policy.rs:34
  method clone_boxed of trait RetryPolicy, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-aef0cf5c8dea75d878f0ae8c687213fd9fa287d7/28587ea95124515726f41c658488682589e689eb/scylla/src/transport/retry_policy.rs:34

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  40.550s] scylla
     Parsing scylla-cql v0.3.0 (current)
      Parsed [   9.944s] (current)
     Parsing scylla-cql v0.3.0 (baseline)
      Parsed [  10.046s] (baseline)
    Checking scylla-cql v0.3.0 -> v0.3.0 (no change)
     Checked [   0.106s] 94 checks: 94 pass, 0 skip
     Summary no semver update required
    Finished [  20.143s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Oct 24, 2024
@muzarski muzarski force-pushed the retry-policy-behind-arc branch 2 times, most recently from e2fbbf2 to de6fde4 Compare October 24, 2024 14:55
This way, it's easier to reuse retry policy for multiple
execution profiles/statement configs - there is no need
for additional allocation of Box.
Again, from now on we will prefer storing dyn RetryPolicy behind
an Arc. There is no need for `clone_boxed` method anymore.

Removed `impl Clone for Box<dyn RetryPolicy>`
Comment on lines +194 to +195
pub(crate) fn retry_policy() -> Arc<dyn RetryPolicy> {
Arc::new(DefaultRetryPolicy::new())
Copy link
Collaborator

Choose a reason for hiding this comment

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

We do lose something. Namely, Box has an optimisation which makes it not allocate anything for ZSTs, and Arc allocates always (its control block). However, I don't know if the optimisation is viable for Box<dyn Trait> with ZST inside.

@Lorak-mmk Lorak-mmk merged commit 9aaf25a into scylladb:main Oct 24, 2024
@wprzytula wprzytula mentioned this pull request Nov 14, 2024
@muzarski muzarski deleted the retry-policy-behind-arc branch December 19, 2024 16:34
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.

3 participants