feat(services/zookeeper): Add zookeeper backend #2735
Conversation
|
Hi, so sorry for the delay😔. Currently I've fixed the bugs and the tests has passed. The |
Thanks a lot for your effort!
Don't worry about this. Rust 1.72 released! So part of our code didn't pass the clippy.
That's ok to implement in the future. |
Addressed in #2927. Please merge with main and try again! |
| } | ||
| } | ||
|
|
||
| fn create_nested_node<'a>( |
There was a problem hiding this comment.
Maybe we can use async fn create_nested_node() directly instead of playing with BoxFuture?
There was a problem hiding this comment.
I guess it is this form cause of recursion. I have a similar version using loop. Hope it could help @Murphy-OrangeMud .
PS: I think I should port some convenient methods from java world.
There was a problem hiding this comment.
I have release version 0.6.0 which has mkdir to create nodes up to given path, just like mkdir -p in filesystem. Hope it could help. @Murphy-OrangeMud
| if path.as_bytes()[0] != b'/' { | ||
| path = "/".to_string() + path.strip_suffix('/').unwrap_or(&path); | ||
| } | ||
| match self.create_nested_node(&path, value).await { |
There was a problem hiding this comment.
If it's required to create parent node first for zookeepr, I perfer to check if parent exist first instead handling error.
There was a problem hiding this comment.
Yes, create_nested_node() is a recursive function that creates parent node recursively till a node exists. So when we find that the parent node doesn't exit (the NoNode error in zookeeper-client indicates this) we call this function to create the parent nodes till all the parent nodes on the path in the tree has created. Also it's because the fact that the function is both recursive and async that we use BoxFuture.
|
kezhuw/zookeeper-client-rust#13 It seems that a test has newly been added and this pr failed the test. Tested with the sdk and has opened an issue here. |
There was a problem hiding this comment.
Comments related to this pr are inlined.
Beside above, thanks for the adoption of kezhuw/zookeeper-client-rust. I have created issues for some missing features. I think it is better to move forward without them for now to keep passion.
- TLS support (TLS support kezhuw/zookeeper-client-rust#14)
- Recursive create and delete (API for recursive create kezhuw/zookeeper-client-rust#15)
Anyway, have fun!
| } | ||
| } | ||
|
|
||
| fn create_nested_node<'a>( |
There was a problem hiding this comment.
I guess it is this form cause of recursion. I have a similar version using loop. Hope it could help @Murphy-OrangeMud .
PS: I think I should port some convenient methods from java world.
| match self.create_nested_node(&path, value).await { | ||
| Ok(()) => {} | ||
| Err(e) => { | ||
| return Err(Error::new( |
There was a problem hiding this comment.
I saw Error::set_temporary. I think it is a natural fit for zk::Error::ConnectionLoss.
Besides this, is it better to enrich the error a bit ?
@Xuanwo Is it better to shape Error::new to accept message: impl Into<String> ? I saw Error::new(_, &format!("")).
| /// currently we do not support sasl authentication | ||
| const ZOOKEEPER_AUTH_SCHEME: &str = "digest"; | ||
|
|
||
| /// Zookeeper backend builder |
There was a problem hiding this comment.
Shall we document the limitations of this backend ?
ZooKeeper, as an aged CP system, does not work well as file backend though its node hierarchy is shaped like a tree. I list some of notables here for what I know.
- Data node size limitation is 1MiB.
- Does not work well when
listdirectory with large number of children.
The first might be crucial.
There was a problem hiding this comment.
Yes, we will record the first limitation by adding a new capability with a maximum file size.
Murphy-OrangeMud
requested review from
messense,
suyanhanx,
tisonkun,
silver-ymz and
xyjixyjixyji
as code owners
|
Running |
It seems that this test （`test_copy_file_with_non_ascii_name`） will always fail. This is the screenshot using zookeeper commandline client with local zookeeper clusters.
|
You can skip this test for now like what we do for supabase. |
|
Hi @Murphy-OrangeMud, are you still on this? This PR has been open for a while, and I believe we can resolve conflicts and workaround some issue so that we can merge it first. |
|
Hi, @Murphy-OrangeMud, thanks for your contribution first. We've made numerous updates to our Rust core, rendering this PR quite outdated. I'll be closing it but feel free to open a new one. I'm here to offer any help you might need. |
Add Zookeeper backend skeleton.
An issue to be discussed is that Zookeeper supports plugin Acl. In this implementation the authentication is similar to redis using username+password digest and the access control is consistent throughout the session. Maybe we could apply acl at a finer grain in the future.