[OSPP] Spark RDD Reader for GraphScope #2103

Issac-Newton · 2022-09-29T13:59:24Z

What do these changes do?

Using gRPC to transfer data in GraphX(RDD) to GraphScope

Related issue number

welcome · 2022-09-29T13:59:26Z

Thanks for submitting your first pull request! You are awesome! 🤗
Please make sure you have signed the CLA, as this is a mandatory check before a PR being merged.
Welcome to the GraphScope community! 🎉 You can meet the community on DingTalk or Slack.

CLAassistant · 2022-09-29T14:00:21Z

All committers have signed the CLA.

zhanglei1949 · 2022-10-28T03:02:55Z

Hi, thank you for opening pull request for GraphScope! Could you please rebase your branch to main?

zhanglei1949 · 2022-10-28T06:06:40Z

hi, @Issac-Newton, Please check your coding style(c++, java) to pass CI test. https://github.com/alibaba/GraphScope/blob/main/.github/workflows/docs.yml#L21
https://github.com/alibaba/GraphScope/blob/main/.github/workflows/docs.yml#L58

codecov-commenter · 2022-10-28T07:11:41Z

Codecov Report

Merging #2103 (bd8ff48) into main (74af74f) will decrease coverage by 5.86%.
The diff coverage is n/a.

❗ Current head bd8ff48 differs from pull request most recent head 6d8e869. Consider uploading reports for the commit 6d8e869 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2103      +/-   ##
==========================================
- Coverage   72.93%   67.07%   -5.87%     
==========================================
  Files          89       89              
  Lines        9789     9789              
==========================================
- Hits         7140     6566     -574     
- Misses       2649     3223     +574

Impacted Files	Coverage Δ
...aphscope/tests/kubernetes/test_resource_builder.py	`0.00% <0.00%> (-100.00%)`	⬇️
...on/graphscope/tests/kubernetes/test_demo_script.py	`0.00% <0.00%> (-91.75%)`	⬇️
python/graphscope/deploy/kubernetes/utils.py	`11.90% <0.00%> (-58.58%)`	⬇️
python/graphscope/deploy/kubernetes/cluster.py	`23.01% <0.00%> (-51.70%)`	⬇️
...thon/graphscope/tests/kubernetes/test_with_mars.py	`0.00% <0.00%> (-44.83%)`	⬇️
...n/graphscope/deploy/kubernetes/resource_builder.py	`30.52% <0.00%> (-26.32%)`	⬇️
python/graphscope/interactive/query.py	`71.95% <0.00%> (-17.08%)`	⬇️
python/graphscope/client/rpc.py	`77.09% <0.00%> (-9.17%)`	⬇️
python/graphscope/tests/conftest.py	`76.53% <0.00%> (-4.43%)`	⬇️
python/graphscope/client/session.py	`73.62% <0.00%> (-2.85%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25ab466...6d8e869. Read the comment docs.

zhanglei1949 · 2022-10-28T05:46:00Z

analytical_engine/core/java/rdd_transfer_client.cc

+
+int main()
+{
+    MPI_Init(NULL, NULL);


Please use grape::CommSpec instead. Refer to any unit test under analytical_engine/test/

zhanglei1949 · 2022-10-28T05:50:17Z

analytical_engine/core/java/rdd_transfer_client.cc

@@ -0,0 +1,296 @@
+#include <iostream>


Could we extract the definition of rdd_transfer_client to a header file, for example, rdd_reader.h? And create a test file under analytical_engine/java/grape-rdd-reader/src/main/cpp/rdd_reader.cc. Also include your CMakeLists.txt under grape-rdd-reader.

zhanglei1949 · 2022-10-28T05:55:56Z

analytical_engine/java/rdd-to-graphscope/pom.xml

@@ -0,0 +1,255 @@
+<?xml version="1.0" encoding="UTF-8"?>


Rename this module to grape-rdd-reader, and include this module as a submodule of grape-jdk-parent.

zhanglei1949 · 2022-10-28T05:56:41Z

analytical_engine/java/rdd-to-graphscope/pom.xml

+        <protoc.version>3.19.2</protoc.version>
+
+        <scala.version>2.13.8</scala.version>
+        <spark.version>3.2.1</spark.version>


All these properties should be declared in parent pom, and should be consistent with current declared version.

zhanglei1949 · 2022-10-28T05:56:57Z

analytical_engine/java/rdd-to-graphscope/pom.xml

+
+        <!-- required for jdk9 -->
+        <maven.compiler.source>1.7</maven.compiler.source>
+        <maven.compiler.target>1.7</maven.compiler.target>


Check this.

zhanglei1949 · 2022-10-28T06:01:46Z

analytical_engine/java/rdd-to-graphscope/src/main/java/RDDReaderTransfer/RDDReadServer.java

@@ -0,0 +1,238 @@
+package RDDReaderTransfer;


Please include license for every single file you added.

zhanglei1949 · 2022-10-28T06:02:38Z

analytical_engine/java/rdd-to-graphscope/src/main/java/RDDReaderTransfer/RDDReadServer.java

+
+
+public class RDDReadServer {
+    //静态方法用于获取当前节点的ip


Please replace/remove all Chinese comments with english comments,

zhanglei1949 · 2022-10-28T06:03:01Z

analytical_engine/java/rdd-to-graphscope/src/main/java/RDDReaderTransfer/RDDReadServer.java

+            @Override
+            public void run() {
+                // Use stderr here since the logger may have been reset by its JVM shutdown hook.
+                System.err.println("*** shutting down gRPC server since JVM is shutting down");


Please user Logger.error

zhanglei1949 · 2022-10-28T06:03:54Z

analytical_engine/java/rdd-to-graphscope/src/main/java/RDDReaderTransfer/RDDReadServer.java

+
+    private void stop() throws InterruptedException {
+        if (server != null) {
+            server.shutdown().awaitTermination(30, TimeUnit.SECONDS);


The awaitTermination time should be confinable or at least declared as a constant in class. Avoid directly use the value.

zhanglei1949 · 2022-10-28T06:04:55Z

analytical_engine/java/rdd-to-graphscope/src/main/scala/RDDTransfer.scala

@@ -0,0 +1,68 @@
+package RDDReaderTransfer


If this is a unit test, move to src/test/scala

zhanglei1949 · 2022-10-30T15:54:29Z

analytical_engine/core/java/rdd_transfer_client.cc

@@ -0,0 +1,218 @@
+/** Copyright 2022 Alibaba Group Holding Limited.


rename to rdd_reader_client.h?

zhanglei1949 self-requested a review September 29, 2022 15:20

zhanglei1949 changed the title ~~using grpc to transfer rdd~~ [OSPP] Spark RDD Reader for GraphScope Sep 29, 2022

zhanglei1949 reviewed Oct 30, 2022

View reviewed changes

zhanglei1949 approved these changes Oct 31, 2022

View reviewed changes

Issac-Newton added 8 commits October 31, 2022 13:51

using grpc to transfer rdd

79eeed1

Update RDDReadServer.java

7eb9f86

transfer graph

e086080

clang-format in cpp

6656b77

java format

2026231

add graphx_client for load

1dfa78c

fix structure

a7294d2

fix format

a1b89c3

sighingnow merged commit f4dea3e into alibaba:main Oct 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OSPP] Spark RDD Reader for GraphScope #2103

[OSPP] Spark RDD Reader for GraphScope #2103

Issac-Newton commented Sep 29, 2022 •

edited by sighingnow

welcome bot commented Sep 29, 2022

CLAassistant commented Sep 29, 2022 •

edited

zhanglei1949 commented Oct 28, 2022 •

edited

zhanglei1949 commented Oct 28, 2022

codecov-commenter commented Oct 28, 2022 •

edited

zhanglei1949 Oct 28, 2022

zhanglei1949 Oct 28, 2022

zhanglei1949 Oct 28, 2022

zhanglei1949 Oct 28, 2022

zhanglei1949 Oct 28, 2022

zhanglei1949 Oct 28, 2022

zhanglei1949 Oct 28, 2022

zhanglei1949 Oct 28, 2022

zhanglei1949 Oct 28, 2022

zhanglei1949 Oct 28, 2022

zhanglei1949 Oct 30, 2022



		public class RDDReadServer {
		//静态方法用于获取当前节点的ip

		@@ -0,0 +1,218 @@
		/** Copyright 2022 Alibaba Group Holding Limited.

[OSPP] Spark RDD Reader for GraphScope #2103

[OSPP] Spark RDD Reader for GraphScope #2103

Conversation

Issac-Newton commented Sep 29, 2022 • edited by sighingnow

What do these changes do?

Related issue number

welcome bot commented Sep 29, 2022

CLAassistant commented Sep 29, 2022 • edited

zhanglei1949 commented Oct 28, 2022 • edited

zhanglei1949 commented Oct 28, 2022

codecov-commenter commented Oct 28, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Issac-Newton commented Sep 29, 2022 •

edited by sighingnow

CLAassistant commented Sep 29, 2022 •

edited

zhanglei1949 commented Oct 28, 2022 •

edited

codecov-commenter commented Oct 28, 2022 •

edited