-
Notifications
You must be signed in to change notification settings - Fork 203
Replace Fred with redis-rs #2076
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
base: main
Are you sure you want to change the base?
Conversation
|
this will need to be released as 0.8.0 the last version, excluding bug fixes, before |
| futures = { version = "0.3.31", default-features = false } | ||
| futures = { version = "0.3.31", features = [ | ||
| "async-await", | ||
| ], default-features = false } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing fred pulled this in before, but need to spell it out explicitly here now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm. That's surprising.
|
@amankrx I'm good with this after you have tested with some of the larger workloads. Once you've done so, please let us know. |
MarcusSorealheis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (pending testing outcome)
chrisstaite-menlo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrisstaite-menlo reviewed 5 of 25 files at r1, 2 of 11 files at r2, 3 of 6 files at r3, 5 of 14 files at r4.
Reviewable status: 0 of 1 LGTMs obtained, and 14 of 29 files reviewed, and pending CI: Redis store tester, Bazel Dev / macos-15, Cargo Dev / macos-15, and 4 discussions need to be resolved
nativelink-error/src/lib.rs line 283 at r4 (raw file):
}; make_err!(code, "{:?}: {error}", error.kind())
If adding one parameter then I'd remove the other from inline to avoid confusion.
nativelink-config/Cargo.toml line 13 at r4 (raw file):
byte-unit = { version = "5.1.6", default-features = false, features = ["byte"] } clap = { version = "4.5.35", features = ["derive"], default-features = false }
Seems a bit much to bring in the whole of clap for ValueEnum...
nativelink-store/src/redis_store.rs line 140 at r4 (raw file):
// Channel for getting subscription messages subscriber_channel: Mutex<Option<UnboundedReceiver<PushInfo>>>,
Is this necessary? Can it be generated on demand with the Psubscription?
nativelink-store/src/redis_store.rs line 224 at r4 (raw file):
} async fn get_client(&'_ self) -> Result<ClientWithPermit<C>, Error> {
Is the '_ necessary?
nativelink-store/src/redis_store.rs line 386 at r4 (raw file):
let connection_manager_config = { ConnectionManagerConfig::new() // .set_automatic_resubscription()
Commented out code should be removed.
nativelink-store/src/redis_store.rs line 397 at r4 (raw file):
client, connection_manager_config, // connection_manager_config.set_push_sender(tx),
Commented out code, should be removed.
nativelink-store/src/redis_store.rs line 1021 at r4 (raw file):
} pub trait Psubscribe: Send + 'static {
This is a terrible name for a trait.
nativelink-store/src/redis_store.rs line 1063 at r4 (raw file):
.psubscribe(&pub_sub_channel) .await .expect("psubscribe");
Not a fan of expect here. Can you instead handle it with an error message and early return instead? Panicing should not be necessary.
Description
Replaces fred with redis-rs, because there's a lot of issues with the former and the latter is more maintained and more widely used (~12x download count on crates.io - ~4M vs. 47M)
Type of change
not work as expected)
in theory, this is a drop-in change. In practice, there's likely a number of underlying changes - which is kinda the point, but there's probably some undertested areas that we don't know about issues in yet.
How Has This Been Tested?
bazel test //...and a bunch of time with the store testerChecklist
bazel test //...passes locallygit amendsee some docsThis change is