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

Merge from redis-rs and fixes for rust 1.80.0 #2029

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

barshaul
Copy link
Collaborator

@barshaul barshaul commented Jul 27, 2024

Fixes for rust 1.80.0:
Including:

Merge from upstream redis-rs:
Including:

  • Removed IP constrains
  • Changed non-key based commands not to be routed to a random node
  • Rust 1.80.0 linter fixes

@barshaul barshaul requested a review from a team as a code owner July 27, 2024 12:52
@barshaul barshaul requested a review from avifenesh July 27, 2024 12:53
@@ -13,7 +13,7 @@ test-env-helpers = "0.2.2"

[dependencies]
tracing = "0.1"
tracing-appender = "0.2.2"
tracing-appender = { version = "0.2.3", default-features = false }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you check if it has any implication to the behavior, does the features connected to our usage in some way?
Generally for faster builds we should disable default feature everywhere.
We'll not go all over the packages now (for redis-rs ill do it at some point) but let's try to make sure we do that when adding new packages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -30,6 +30,7 @@ nanoid = "0.4.0"

[features]
socket-layer = ["directories", "integer-encoding", "num_cpus", "protobuf", "tokio-util"]
standalone_heartbeat = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the Idea of this feature?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's a standalone feature to run heartbeats on the connections which we decided not to enable yet, this is why it's behind a feature flag.

@avifenesh
Copy link
Collaborator

Can you please mention what's the upstream merge include?
And it's better to separate.
If something break after PR merge we know the scope - when it mixed, and not for a typo fix or doc it make it problematic to trace.

@barshaul
Copy link
Collaborator Author

Can you please mention what's the upstream merge include?

It's mentioned on the PR descriptionn

And it's better to separate.
If something break after PR merge we know the scope - when it mixed, and not for a typo fix or doc it make it problematic to trace.

It's already separated by commits

@barshaul barshaul merged commit d89a9de into valkey-io:main Jul 29, 2024
27 checks passed
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.

2 participants