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

[feat] Support redis call with wasm-rust #1417

Merged
merged 16 commits into from
Oct 29, 2024
Merged

[feat] Support redis call with wasm-rust #1417

merged 16 commits into from
Oct 29, 2024

Conversation

007gzs
Copy link
Collaborator

@007gzs 007gzs commented Oct 22, 2024

Ⅰ. Describe what this PR did

Support redis call with wasm-rust

Ⅱ. Does this pull request fix one issue?

#1297
fixes #908

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

依赖 higress-group/proxy-wasm-rust-sdk#4

@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.52%. Comparing base (ef31e09) to head (85129c2).
Report is 174 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1417      +/-   ##
==========================================
+ Coverage   35.91%   43.52%   +7.61%     
==========================================
  Files          69       76       +7     
  Lines       11576    12320     +744     
==========================================
+ Hits         4157     5362    +1205     
+ Misses       7104     6622     -482     
- Partials      315      336      +21     

see 69 files with indirect coverage changes

@007gzs 007gzs requested a review from johnlanni October 22, 2024 04:23
@johnlanni
Copy link
Collaborator

cc @jizhuozhi @Lynskylate

}

impl RedisClient {
pub fn new<T: AsRef<str>>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about use config and Function Options pattern to reduce constructors?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any suggestions on which of the following solutions should I choose?

build1:

pub struct RedisClientBuilder{
    upstream: String,
    username: Option<String>,
    password: Option<String>,
    timeout: Duration,
}
impl RedisClientBuilder {
    pub fn new(cluster: &dyn Cluster, timeout: Duration) -> Self{
        RedisClientBuilder {
            upstream: cluster.cluster_name(),
            username: None,
            password: None,
            timeout,
        }
    }
    pub fn username<T: AsRef<str>>(mut self, username: Option<T>) -> Self{
        self.username = username.map(|u| u.as_ref().to_string());
        self
    }
    pub fn password<T: AsRef<str>>(mut self, password: Option<T>) -> Self{
        self.password = password.map(|p| p.as_ref().to_string());
        self
    }
    pub fn build(self) -> RedisClient{
        RedisClient {
            upstream: self.upstream,
            username: self.username,
            password: self.password,
            timeout: self.timeout,
        }
    }
}
// use 1
let mut builder = RedisClientBuilder::new(
    &StaticIpCluster::new("redis", 80, ""),
    Duration::from_secs(5)
);
builder = builder.password(config.password.as_ref());
let c = builder.build();
// use 2
let c = RedisClientBuilder::new(
    &StaticIpCluster::new("redis", 80, ""),
    Duration::from_secs(5)
).password(config.password.as_ref()).build();

build2:

pub struct RedisClientBuilder{
    upstream: String,
    username: Option<String>,
    password: Option<String>,
    timeout: Duration,
}
impl RedisClientBuilder {
    pub fn new(cluster: &dyn Cluster, timeout: Duration) -> Self{
        RedisClientBuilder {
            upstream: cluster.cluster_name(),
            username: None,
            password: None,
            timeout,
        }
    }
    pub fn username<T: AsRef<str>>(&mut self, username: Option<T>) -> &Self{
        self.username = username.map(|u| u.as_ref().to_string());
        self
    }
    pub fn password<T: AsRef<str>>(&mut self, password: Option<T>) -> &Self{
        self.password = password.map(|p| p.as_ref().to_string());
        self
    }
    pub fn build(&self) -> RedisClient{
        RedisClient {
            upstream: self.upstream.clone(),
            username: self.username.clone(),
            password: self.password.clone(),
            timeout: self.timeout.clone(),
        }
    }
}
// use 1
let mut builder = RedisClientBuilder::new(
    &StaticIpCluster::new("redis", 80, ""),
    Duration::from_secs(5)
);
builder.password(config.password.as_ref());
let c = builder.build();
let c2 = builder.build();
// use 2
let c = RedisClientBuilder::new(
    &StaticIpCluster::new("redis", 80, ""),
    Duration::from_secs(5)
).password(config.password.as_ref()).build();

config:

pub struct RedisClientConfig{
    upstream: String,
    username: Option<String>,
    password: Option<String>,
    timeout: Duration,
}
impl RedisClientConfig {
    pub fn new(cluster: &dyn Cluster, timeout: Duration) -> Self{
        RedisClientConfig {
            upstream: cluster.cluster_name(),
            username: None,
            password: None,
            timeout,
        }
    }
    pub fn username<T: AsRef<str>>(&mut self, username: Option<T>) -> &Self{
        self.username = username.map(|u| u.as_ref().to_string());
        self
    }
    pub fn password<T: AsRef<str>>(&mut self, password: Option<T>) -> &Self{
        self.password = password.map(|p| p.as_ref().to_string());
        self
    }
}
// use
let mut builder = RedisClientConfig::new(
    &StaticIpCluster::new("redis", 80, ""),
    Duration::from_secs(5)
);
builder.password(config.password.as_ref());
let c = RedisClient::new(&builder);
let c2 = RedisClient::new(&builder);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

self.timeout,
)
}
fn call(&self, query: &[u8], call_fn: Box<RedisValueCallbackFn>) -> Result<u32, Status> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

raw call could be public for some custom commends

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

custom commends can call command function
pub fn command(&self, cmd: &Cmd, call_fn: Box<RedisValueCallbackFn>) -> Result<u32, Status>

@@ -86,6 +77,24 @@ impl EventStream {

None
}
}
impl EventStream {
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a well formated code, there should have a newline bewteen two blocks

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done in wasm-rust/src * rust-lang/rustfmt#4866

@@ -86,6 +77,24 @@ impl EventStream {

None
}
}
impl EventStream {
pub fn new() -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no need to keep new but just use default to construct, and I don't think it's a good habit that mixing different things in single PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done, I made modifications only after discovering an error with clippy during local testing

@@ -257,7 +252,7 @@ where
}
}
}
self.http_content.borrow().log().warn(&format!(
self.http_content.borrow().log().debug(&format!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

well, maybe we need a log level guard because format is not zero cost, and in some condition you will build a very large string but you are not consume it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This may need to be implemented using macros. You can create a new issue to record it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

call_fn(&res, status, token_id);
})
}
pub struct RedisClientBuilder {
Copy link
Collaborator

Choose a reason for hiding this comment

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

as above, well formated should keep newlines between different blocks

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator

@jizhuozhi jizhuozhi left a comment

Choose a reason for hiding this comment

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

LGTM

@jizhuozhi jizhuozhi merged commit 2219a17 into alibaba:main Oct 29, 2024
12 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.

Support redis call with wasm-rust
4 participants