Skip to content

Improve retry logic and update unmaintained dependencies for Rust lint CI#2673

Merged
barshaul merged 8 commits intovalkey-io:release-1.2from
barshaul:fix_rust_ci
Nov 12, 2024
Merged

Improve retry logic and update unmaintained dependencies for Rust lint CI#2673
barshaul merged 8 commits intovalkey-io:release-1.2from
barshaul:fix_rust_ci

Conversation

@barshaul
Copy link
Copy Markdown
Collaborator

@barshaul barshaul commented Nov 11, 2024

PR Description:

This PR enhances the retry strategy and addresses dependency maintenance issues in the Rust lint CI:

  • Replaced Unmaintained Crates: Removed the backoff, tokio-retry, futures-time and Derivative crates due to limited functionality and lack of maintenance. Previously, the project used both backoff (unmaintained) and tokio-retry (last updated 3 years ago). These have been replaced with tokio-retry2, which offers enhanced features for exponential backoff. Additionally, Derivative was removed as it is no longer maintained.

  • Improved Retry Functionality: The new tokio-retry2 crate includes better jitter control, ensuring consistent exponential backoff intervals without erratic timings. The previous implementation produced inconsistent retry intervals (e.g., 192ms, 110ms, 900ms). With tokio-retry2, the jitter range is controlled, maintaining a steady exponential backoff. For slot refresh retries, the strategy has been updated to ExponentialFactorBackoff, resulting in attempts with intervals such as [0ms, 400-600ms, 800-1200ms].

Issue link

This Pull Request is linked to issue (URL): closes #2669

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one issue.
  • Commit message has a detailed description of what changed and why.
  • Tests are added or updated.
  • CHANGELOG.md and documentation files are updated.
  • Destination branch is correct - main or release
  • Commits will be squashed upon merging.

Signed-off-by: barshaul <barshaul@amazon.com>
Signed-off-by: barshaul <barshaul@amazon.com>
…ed "tokio-retry"

Signed-off-by: barshaul <barshaul@amazon.com>
Signed-off-by: barshaul <barshaul@amazon.com>
@barshaul barshaul changed the title Fix rust failing DENY linter in CI Improve retry logic and update unmaintained dependencies for Rust lint CI Nov 11, 2024
Signed-off-by: barshaul <barshaul@amazon.com>
Signed-off-by: barshaul <barshaul@amazon.com>
Signed-off-by: barshaul <barshaul@amazon.com>
Signed-off-by: barshaul <barshaul@amazon.com>
ExponentialBackoff::from_millis(self.exponent_base as u64)
.factor(self.factor as u64)
.map(jitter)
.map(jitter_range(0.8, 1.2))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How did you come up with these numbers?

Copy link
Copy Markdown
Collaborator Author

@barshaul barshaul Nov 12, 2024

Choose a reason for hiding this comment

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

Till now, the jitter range was (0,1). This change reduces the range to 20% above and below the interval, which is sufficient to spread requests across different time slots from various hosts, while maintaining increasing intervals

)
.map(jitter_range(0.8, 1.2))
.take(DEFAULT_NUMBER_OF_REFRESH_SLOTS_RETRIES);
let retries_counter = AtomicUsize::new(0);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we need to use the counter and not the built in limiter? Is there a reason we need to use the number down the tree?
If it can be changed and managed in one place it will reduce complexity.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We use the retry number internally to calculate topology to know if we're at the last try - and if we are, we have less restrictions on choosing the topology view

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Note the we don't use it as a limit anymore, only for the sake of choosing the topology.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see I tried to see if there's a way to call something different on last try. It appears that the only callback is notify, which can be used, but it's not the reason it exists, so probably not a good idea.
In general, I would prefer the called function to have one logic, and the caller to call functions base on needs, so we keep functions shorter and more readable. That's why I want to avoid the counter logic.
But couldn't find a replacement currently.

@barshaul barshaul marked this pull request as ready for review November 12, 2024 08:07
@barshaul barshaul requested a review from a team as a code owner November 12, 2024 08:07
@barshaul barshaul merged commit 0e52ac9 into valkey-io:release-1.2 Nov 12, 2024
mo-amzn pushed a commit to mo-amzn/valkey-glide that referenced this pull request Nov 13, 2024
…t CI (valkey-io#2673)

Signed-off-by: Muhammad Awawdi <mawawdi@amazon.com>
mo-amzn pushed a commit to mo-amzn/valkey-glide that referenced this pull request Nov 13, 2024
mo-amzn pushed a commit to mo-amzn/valkey-glide that referenced this pull request Nov 13, 2024
…t CI (valkey-io#2673)

Signed-off-by: Muhammad Awawdi <mawawdi@amazon.com>
mo-amzn pushed a commit to mo-amzn/valkey-glide that referenced this pull request Nov 13, 2024
mo-amzn pushed a commit to mo-amzn/valkey-glide that referenced this pull request Nov 13, 2024
…t CI (valkey-io#2673)

Signed-off-by: Muhammad Awawdi <mawawdi@amazon.com>
mo-amzn pushed a commit to mo-amzn/valkey-glide that referenced this pull request Nov 13, 2024
…t CI (valkey-io#2673)

Signed-off-by: Muhammad Awawdi <mawawdi@amazon.com>
@Yury-Fridlyand Yury-Fridlyand mentioned this pull request Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dependencies in redis-rs and Go lib.rs: Unmainted dependencies are used in redis-rs need to be replaced

3 participants