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

Core: Extract proto from socket-layer feature. #2861

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Yury-Fridlyand
Copy link
Collaborator

No changes for clients or user API.

But we can build non-UDS clients on windows now:

C:\projects\glide\go>cargo build
   Compiling glide-core v0.1.0 (C:\projects\glide\glide-core)
   Compiling glide-rs v0.1.0 (C:\projects\glide\go)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 5.78s

@Yury-Fridlyand Yury-Fridlyand added the Rust core redis-rs/glide-core matter label Dec 24, 2024
@Yury-Fridlyand Yury-Fridlyand requested a review from a team as a code owner December 24, 2024 03:10
@@ -13,10 +13,11 @@ fn build_protobuf() {
.input("src/protobuf/response.proto")
.input("src/protobuf/connection_request.proto")
.customize(customization_options)
.out_dir("src/generated")
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 is not mandatory, but it puts generated files to a place where they could be found

@@ -28,7 +28,7 @@ use tokio::runtime::Runtime;
#[derive(Debug)]
pub struct CommandResponse {
response_type: ResponseType,
int_value: c_long,
int_value: i64,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

c_long is i64, but on windows is i32 🤦

@avifenesh
Copy link
Member

In what case we don't need the proto?

@Yury-Fridlyand
Copy link
Collaborator Author

socket-layer includes proto. This PR adds option to activate proto feature without activating socket-layer.
For example, go client uses protobuf, but doesn't use UDS.

@avifenesh
Copy link
Member

socket-layer includes proto. This PR adds option to activate proto feature without activating socket-layer. For example, go client uses protobuf, but doesn't use UDS.

I completely understand. But now when you are changing, which is great, I'm wondering why Protobuf feature at all? Do we have a scenario when we don't use proto?

@Yury-Fridlyand
Copy link
Collaborator Author

Do we have a scenario when we don't use proto?

Not yet.

@avifenesh
Copy link
Member

Do we have a scenario when we don't use proto?

Not yet.

So i think we should remove it. If and when we will, we can add it.

@Yury-Fridlyand
Copy link
Collaborator Author

I just recalled that .Net client don't use it. But maybe it would.

@avifenesh
Copy link
Member

I just recalled that .Net client don't use it. But maybe it would.

@Sa1Gur any input on C# usage (or not) of protobuf for communication?

@Sa1Gur
Copy link
Collaborator

Sa1Gur commented Feb 3, 2025

I just recalled that .Net client don't use it. But maybe it would.

@Sa1Gur any input on C# usage (or not) of protobuf for communication?

It is not using protobuf and frankly I would be surprised if using of protobuf would bring any benefit (but to be sure we need to measure of course). .NET have quite efficient support of native calls. And I doubt anything could bit that. But again - needs to be tested

@avifenesh
Copy link
Member

I just recalled that .Net client don't use it. But maybe it would.

@Sa1Gur any input on C# usage (or not) of protobuf for communication?

It is not using protobuf and frankly I would be surprised if using of protobuf would bring any benefit (but to be sure we need to measure of course). .NET have quite efficient support of native calls. And I doubt anything could bit that. But again - needs to be tested

@Yury-Fridlyand so its seems that it is needed.
@Sa1Gur Out of the scope of this PR, but I would love to understand how this support is so efficient so we don't need a communication protocol at all.
Consider it an optional subject for contributors meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust core redis-rs/glide-core matter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants