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

FFI connection without protobuf + C# #126

Open
wants to merge 15 commits into
base: csharp/integ_yuryf_connection
Choose a base branch
from

Conversation

Yury-Fridlyand
Copy link

@Yury-Fridlyand Yury-Fridlyand commented Mar 2, 2024

  • Structs on C# side
  • Structs on Rust side
  • Build protobuf request from struct on Rust side
  • Build struct on C# side
  • Docs
  • Memory test - no leak proof
  • Tests
  • More tests for the god of tests

Config usage example

var config = new StandaloneClientConfigurationBuilder()
    .WithAddress("localhost", 6379)
    .WithAddress("hoho", 8080)
    .WithAddress("asdf")
    .WithAuthentication(null, "pwd")
    .With(ProtocolVersion.RESP3)
    .With(TlsMode.SecureTls)
    .WithRequestTimeout(23400)
    .With(ReadFrom.PreferReplica)
    .WithDataBaseId(42)
    .WithConnectionRetryStrategy(1, 2, 3)
    .WithClientName("pewpew")
    .Build();

Config extended samples

var builder = new StandaloneClientConfigurationBuilder();
builder
    .WithAddress(("server", 42))
    .WithAddress("server", 42)
    .WithAddress(null, (uint?)42)
    .WithAddress("server", null)
    .WithAddress(42)
    .WithAddress(port: 42)
    .WithAddress("server")
    .WithAddress(("server", null))
    .WithAddress((host: "server", port: 42))
  //.WithAddress((port: 42, host: "server")) - doesn't work
    .WithAddress(port: 42, host: "server")
    .WithAddress(port: 42)
    .WithAddress(host: "server")
    ;
builder.Addresses += ("server", 42);
builder.Addresses += (host: "server", port: 42);
//builder.Addresses += (port: 42, host: "server"); - doesn't work
builder.Addresses += ("server", null);
builder.Addresses += (null, (uint?)42);
builder.Addresses += "server";
builder.Addresses += 42;

// TODO have to make property with another name
// builder.Address = "server"; - doesn't work

// TODO do we need this?
/*
builder.Addresses = new List<(string host, uint port)>();
builder.Addresses += new List<(string host, uint port)>();
builder.Addresses = new (string host, uint port)[] { };
builder.Addresses += new (string host, uint port)[] { };
*/


builder.TlsMode = TlsMode.NoTls;
builder.WithTlsMode(TlsMode.InsecureTls);
builder.With(TlsMode.InsecureTls);

builder.ConnectionRetryStrategy = new ConnectionRetryStrategy(/*values*/);
builder.WithConnectionRetryStrategy(new ConnectionRetryStrategy(/*values*/));
builder.With(new ConnectionRetryStrategy(/*values*/));
builder.WithConnectionRetryStrategy(1, 2, 3);

//TODO do we need this?
/*
builder.AuthenticationInfo = new AuthInfo(...)
builder.WithAuthenticationInfo("123") // - is it a username or a password?
builder.WithAuthenticationInfo(new AuthInfo(...))
builder.With(new AuthInfo(...))
builder.With("user", "pass")
*/
builder.AuthenticationInfo = ("user", "looser");
builder.AuthenticationInfo = (null, "looser");
builder.AuthenticationInfo = (username: "user", password: "looser");
//builder.AuthenticationInfo = (password: "looser", username: "user"); - does't work
builder
    .WithAuthenticationInfo("user", "looser")
    .WithAuthenticationInfo(null, "looser")
    .WithAuthenticationInfo(password: "looser", username: "user")
    .WithAuthenticationInfo(("user", "looser"))
    .WithAuthenticationInfo((username: "user", password: "looser"))
  //.WithAuthenticationInfo((password: "looser", username: "user")) - doesn't work
    ;

Signed-off-by: Yury-Fridlyand <[email protected]>
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
csharp/lib/src/connection.rs Outdated Show resolved Hide resolved
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
csharp/lib/src/lib.rs Show resolved Hide resolved
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Comment on lines 470 to 471
Clean(); // memory leak protection on rebuilding a config from the builder
built = true;
Copy link
Author

Choose a reason for hiding this comment

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

this stuff is not thread safe, but I assume it is ok

Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
@Yury-Fridlyand Yury-Fridlyand marked this pull request as ready for review March 6, 2024 21:17
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
use std::ffi::c_char;

#[repr(C)]
pub struct ConnectionConfig {

Choose a reason for hiding this comment

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

Should we have documentation for the types here?

Copy link
Author

Choose a reason for hiding this comment

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

1:1
Will do

Copy link
Author

Choose a reason for hiding this comment

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

Done. Do you know how to add a reference to an arbitrary file by relative path ?

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

I can't understand where relative path starts from. I tried to point to connection_request.proto, but all experiments with ../../.. failed

Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
Signed-off-by: Yury-Fridlyand <[email protected]>
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
csharp/lib/src/lib.rs Outdated Show resolved Hide resolved
Copy link

@jonathanl-bq jonathanl-bq left a comment

Choose a reason for hiding this comment

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

This approval is for the Rust portion of the code. I left some comments on documentation that needs to be updated, but otherwise, this looks fine to me.

Signed-off-by: Yury-Fridlyand <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants