-
Notifications
You must be signed in to change notification settings - Fork 454
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 NebulaGraph #5116
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: feathercyc <[email protected]>
8d5dae0
to
7180639
Compare
Test results are as follows:
|
[[package]] | ||
name = "futures" | ||
version = "0.1.31" | ||
source = "registry+https://github.com/rust-lang/crates.io-index" | ||
checksum = "3a471a38ef8ed83cd6e40aa59c1ffe17db6855c18e3604d9c4ed8c08ebc28678" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who is depending on futures 0.1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's futures-util
, fbthrift-*
used in rust-nebula
and bb8
depends on futures
and futures
depends on futures-util
. futures-utils/dependencies displayed that futures=0.1.25
.
cargo tree
is as follows:
├── bb8 v0.8.5
│ ├── async-trait v0.1.82 (proc-macro) (*)
│ ├── futures-util v0.3.30
│ │ ├── futures v0.1.31
│ │ ├── futures-channel v0.3.30
│ │ │ ├── futures-core v0.3.30
│ │ │ └── futures-sink v0.3.30
Does it need to be removed? I just don't know how to do it:(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems someone has enabled futures-util
's compat
feature in wrong.
Ok, I got it: https://github.com/bk-rs/fbthrift-git-rs/blob/36ddca22a4aca8bbf02482ca20ff3fe525a24a87/Cargo.toml#L17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vkill has contributed almost all of fbThrift's packages, can we remove this without communicating with him😇?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it's possible, but I don't have high hopes. If I can establish a stable connection with vkill, I won't have to release a new rust-nebula
crate. I will make modifications directly on his nebula-rust
crate:(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @GG2002 The feature compat is also enabled in the latest official version. I don't think there is any problem with it.
https://github.com/facebook/fbthrift/blob/v2024.09.23.00/thrift/lib/rust/Cargo.toml#L18
@vkill says that.
What should I do now?
@@ -342,6 +343,11 @@ compio = { version = "0.11.0", optional = true, features = [ | |||
] } | |||
# for services-s3 | |||
crc32c = { version = "0.6.6", optional = true } | |||
# for services-nebula-graph | |||
rust-nebula = { version = "0.1", optional = true, git = "https://github.com/nebula-contrib/rust-nebula", features = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, this could prevent us from making a new release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll publish rust-nebula
crate in a few days, you can take a look at my code first.
@@ -163,6 +163,7 @@ services-moka = ["dep:moka"] | |||
services-mongodb = ["dep:mongodb"] | |||
services-monoiofs = ["dep:monoio", "dep:flume"] | |||
services-mysql = ["dep:sqlx", "sqlx?/mysql"] | |||
services-nebulagraph = ["dep:rust-nebula", "dep:bb8", "dep:snowflaked"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenDAL has strict naming pattern for service names.
Please either use:
nebulagraph
/Nebulagraph
or
nebula_graph
/NebulaGraph
In every place this name showed up.
Hi, @GG2002. It might take us a while to complete the communication. Would you like to split this PR into parts so we can merge some of them first? I believe we can add the builder, config first without introduce the client impl in. |
It's ok. |
Which issue does this PR close?
Closes #4553.
Rationale for this change
What changes are included in this PR?
Support
read
,write
,list
,delete
ops on NebulaGraph.To access NebulaGraph, users must provide the
username
andpassword
for NebulaGraph, as well as specify thespace
,tag
, and thekey field
andvalue field
of thetag
.Since each
vertex
in NebulaGraph must have a unique ID(called VID), but NebulaGraph lacks a self-incrementing ID, the unique IDs are generated using the Snowflake algorithm. Additionally, theset
operation involves firstdelete
existing originalvertex
, followed by inserting a newvertex
.NebulaGraph does not support the Blob type, hence
Base64+string
type is employed for file access. If it's supported in the future, just removeBase64
.Are there any user-facing changes?
No(?)