-
Notifications
You must be signed in to change notification settings - Fork 437
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
[GAIA] Add storage adapter for graphscope store #521
[GAIA] Add storage adapter for graphscope store #521
Conversation
@@ -26,6 +26,7 @@ maxgraph-common = { path = "../../common/rust/common" } | |||
|
|||
[dependencies.rocksdb] | |||
git = "https://github.com/pingcap/rust-rocksdb.git" | |||
rev = "23a604459899f4f7a4c8967f16b6fa623474f27b" |
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.
can we depend on a general/stable verison?
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.
This is a historical issue: the MaxGraph storage has depended on this wired version. While it is necessary to migrate to a stable version, I think we can do that by creating another issue.
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.
Sure, created as issue #525
None, | ||
prop_ids.as_ref(), | ||
params.limit.unwrap_or(0), | ||
// TODO(bingqing): optimization here, each worker may scan some partitions on current server; We use worker 0 to scan all partitions for now |
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.
Comments too loooong, make it two/more lines.
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.
These apply to the following functions
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.
Done
None, | ||
None, | ||
prop_ids.as_ref(), | ||
params.limit.unwrap_or(0), |
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.
Does zero limit mean no limit? Better comment this.
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.
Yes, and I added the comment
.get_all_vertices( | ||
si, | ||
label_ids.as_ref(), | ||
None, |
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.
Can common these two None
to increase readability.
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.
Done
// Zero limit means no limit. Same as follows. | ||
params.limit.unwrap_or(0), | ||
// TODO(bingqing): optimization, each worker may scan some partitions on current server. | ||
// Empty vector means all partitions, since we use worker 0 to scan all for now. |
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.
The comment looks strange to me. worker 0 for now? Why? What is your future plan?
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.
We assign workers to scan partitions in source operator, and for now, worker 0 (specifically, the first worker on each server) is going to scan all partitions on current server for correctness guarantee. In the future, source op should assign each worker to scan some partitions, i.e., allow parallel scan.
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.
BTW, I didn't do this optimization since V2 store and vineyard store seems to give the info of "which worker should scan which partitions" in a ununified way. I will do this optimization after adapting to vineyard.
.ok_or(str_to_dyn_error("get schema failed"))?; | ||
let edge_label_ids = encode_storage_label(params.labels.as_ref(), schema.clone()); | ||
|
||
let stmt = from_fn(move |v: ID| { |
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.
My concern: calling build_partition_vertex_ids for each vertex may not be a good idea, where build_partition_vertex_ids is for a bulk of vertices and will have to new a hashmap each time. This is too heavy for processing a single vertex. Can have better design! This applies to the following codes.
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.
Totally agree. However, GlobalGraphQuery trait defined in GraphScope is designed to process a bulk of vertices, while in GAIA, the bulk process is actually in engine, and adapter is only able to process a single vertex each time.
In the new commit, I remove the unnecessary new HashMap in build_partition_vertex_ids function.
let server_index = self | ||
.graph_partition_manager | ||
.get_server_id(partition_id as PropId) | ||
.unwrap_or_else(|| { |
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.
Looks like get_partition
should return Result. This error is very critical, should not just return 0 directly, but throwing out the error instead.
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.
Done
@@ -0,0 +1,74 @@ | |||
// |
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.
Have no idea why you cannot directly import these functions from Gaia. You can import it into somewhere in graph_proxy for internal use. If it is private in Gaia, make it public is okay.
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.
Done
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.
A critical issue is the build_partition_(label_)vertex_ids
for each vertex. Be aware the costly operation in this function.
@@ -446,17 +446,10 @@ fn build_partition_label_vertex_ids( | |||
/// Transform type of ids to PartitionVertexIds as required by graphscope store, | |||
/// which consists of (PartitionId,Vec<VertexId>) | |||
fn build_partition_vertex_ids( | |||
ids: &[ID], | |||
id: &ID, | |||
graph_partition_manager: Arc<dyn GraphPartitionManager>, | |||
) -> Vec<PartitionVertexIds> { |
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.
Why not get_partition_vertex_id() -> PartitionVertexId
. Also, id is better using id: ID
without reference.
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.
How about the other function with label_ids ?
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've rename the functions. And also using id: ID
. For the other function with label_ids (get_partition_label_vertex_ids
), since it is batch query indeed, I kept the old way.
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.
[WIP] Rocksdb dependence will be refined, as issue #525 suggests.
What do these changes do?
Related issue number
Fixes #417