Separate connection info from protobuf. #1088
Conversation
nihohit
force-pushed
the
no-socket
branch
3 times, most recently
from
23af104 to
078d31e
Compare
| signal-hook = "^0.3" | ||
| signal-hook-tokio = {version = "^0.3", features = ["futures-v0_3"] } | ||
| signal-hook = { version = "^0.3", optional = true } | ||
| signal-hook-tokio = {version = "^0.3", features = ["futures-v0_3"], optional = true } |
There was a problem hiding this comment.
signal-hook-tokio prevented me to build glide on Windows. Now it is optional and I can go thru. redis-rs compilation fails though.
We are getting closer to Windows support. Just FYI.
glide-core/Cargo.toml
Outdated
| once_cell = "1.18.0" | ||
| arcstr = "1.1.5" | ||
| sha1_smol = "1.0.0" | ||
|
|
||
| [features] | ||
| socket-layer = ["directories", "integer-encoding", "num_cpus", "signal-hook", "signal-hook-tokio"] |
There was a problem hiding this comment.
I think it is worth adding description for the features somewhere. What about DEVELOPER.md?
There was a problem hiding this comment.
Shouldn't protobuf be under socket-layer too?
| let read_from = request.read_from.enum_value().unwrap_or(ReadFrom::Primary); | ||
| let read_from_replicas = !matches!(read_from, ReadFrom::Primary,); // TODO - implement different read from replica strategies. | ||
| let read_from = request.read_from.unwrap_or_default(); | ||
| let read_from_replicas = !matches!(read_from, ReadFrom::Primary); // TODO - implement different read from replica strategies. |
There was a problem hiding this comment.
Please, create a GH task for this TODO
There was a problem hiding this comment.
please, check the GH issues. They've been there for months.
There was a problem hiding this comment.
https://github.com/aws/glide-for-redis/issues?q=is%3Aissue+is%3Aopen+read+from
nothing there
Can you give me a link?
| */ | ||
|
|
||
| #[derive(Default)] | ||
| pub struct ConnectionRequest { |
There was a problem hiding this comment.
It is very similar to what I did in https://github.com/Bit-Quill/glide-for-redis/pull/126/files#diff-16c3ad2d12e1e72e855619ea6d9e15e29fab999c7545fbb03b0d8a9f995b96ab, but not FFI-ready.
Is there a way to update your implementation, so I'll reuse it instead of duplicating code? repr(C) and * c_char, etc.
There was a problem hiding this comment.
Applicable fot all structs and enums there
There was a problem hiding this comment.
unless you have a very good reason for it, you shouldn't use * c_char in your Rust structs, only in function signatures. instantiate them to Rust strings ASAP instead of passing unsafe pointers through the system.
There was a problem hiding this comment.
let's decide if we want this, get this in, and then make the adjustments to make if fully FFI-compatible. I don't want to overload this PR with multiple changes.
|
@barshaul thoughts about this? keep or close? |
This will allow the FFI clients to remove their dependency on protobuf. Pros: * faster compile times * no need for protobuf dependency in wrappers.
Co-authored-by: Aaron <[email protected]>
|
@aaron-congo @Yury-Fridlyand @SanHalacogluImproving @jonathanl-bq |
* Separate connection info from protobuf. This will allow the FFI clients to remove their dependency on protobuf. Pros: * faster compile times * no need for protobuf dependency in wrappers. * Update glide-core/src/client/types.rs Co-authored-by: Aaron <[email protected]> * round --------- Co-authored-by: Shachar Langbeheim <[email protected]> Co-authored-by: Aaron <[email protected]>
This will allow the FFI clients to remove their dependency on protobuf.
Pros:
Cons:
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.