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.

Sign up for GitHub

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

Jump to bottom

feat: Introduce HugeGraphFlinkCDCLoader #291

Merged
merged 11 commits into from
Jun 21, 2022
Merged

feat: Introduce HugeGraphFlinkCDCLoader #291

merged 11 commits into from
Jun 21, 2022

Conversation

simon824
Copy link
Member

@simon824 simon824 commented Jun 6, 2022 •
edited
Loading

closed #290
basic version of HugeGraphFlinkCDCLoader

The options are divided into two parts, flink options and hugegraph-loader options. (no sorting required)
Since the parameter abbreviations of hugegraph-loader and flink overlap, please use the full name of hugegraph options, like --file instead of -f

test cmd

 ./bin/hugegraph-flink-cdc-loader.sh  \
-m yarn-cluster --file ./conf/spark.json --username admin --token admin \
--host 127.0.0.1 --port 8093 --graph my_graph2

@codecov
Copy link

codecov bot commented Jun 6, 2022 •
edited
Loading

Codecov Report

Merging #291 (9ec46ae) into master (9ee7abb) will increase coverage by 1.78%.
The diff coverage is 1.04%.

@@             Coverage Diff              @@
##             master     #291      +/-   ##
============================================
+ Coverage     71.12%   72.90%   +1.78%     
- Complexity      877     1849     +972     
============================================
  Files            82      242     +160     
  Lines          3816     8084    +4268     
  Branches        456      716     +260     
============================================
+ Hits           2714     5894    +3180     
- Misses          898     1807     +909     
- Partials        204      383     +179
Impacted Files Coverage Δ
...com/baidu/hugegraph/loader/constant/Constants.java 75.00% <ø> (ø)
...gegraph/loader/flink/HugeGraphDeserialization.java 0.00% <0.00%> (ø)
...ugegraph/loader/flink/HugeGraphFlinkCDCLoader.java 0.00% <0.00%> (ø)
.../hugegraph/loader/flink/HugeGraphOutputFormat.java 0.00% <0.00%> (ø)
.../hugegraph/loader/flink/HugeGraphSinkFunction.java 0.00% <0.00%> (ø)
...u/hugegraph/loader/spark/HugeGraphSparkLoader.java 0.00% <0.00%> (ø)
...m/baidu/hugegraph/loader/executor/LoadOptions.java 75.00% <100.00%> (+0.58%) ⬆️
...baidu/hugegraph/api/traverser/PersonalRankAPI.java 85.96% <0.00%> (ø)
...java/com/baidu/hugegraph/structure/auth/Group.java 72.72% <0.00%> (ø)
.../main/java/com/baidu/hugegraph/api/job/JobAPI.java 100.00% <0.00%> (ø)
... and 153 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 9ee7abb...9ec46ae. Read the comment docs.

javeme
javeme reviewed Jun 6, 2022
View reviewed changes
Copy link
Contributor

@javeme javeme left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, some tiny comments

hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphOutputFormat.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphSinkFunction.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphSinkFunction.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphSinkFunction.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphSinkFunction.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphOutputFormat.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphFlinkCDCLoader.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphFlinkCDCLoader.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphFlinkCDCLoader.java Outdated Show resolved Hide resolved
javeme
javeme reviewed Jun 6, 2022
View reviewed changes
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphDeserialization.java Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphFlinkCDCLoader.java
return;
}
loader.load();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Added lines #L54 - L60 were not covered by tests

can we add some tests for this code?

Copy link
Member Author

Choose a reason for hiding this comment

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

About test , I plan to introduce testcontainers for doing e2e test, starting 3 containers for mysql, flink, hugegraph, for testing the correctness of the entire process (similar for sparkLoader). What do you think?

This may take some time, I will submit later or the next pr.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a good idea, let's do it the next pr

javeme
javeme reviewed Jun 7, 2022
View reviewed changes
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/constant/Constants.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/constant/Constants.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphDeserialization.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphOutputFormat.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphOutputFormat.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphOutputFormat.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphOutputFormat.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphOutputFormat.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphOutputFormat.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphFlinkCDCLoader.java Outdated Show resolved Hide resolved
javeme
javeme reviewed Jun 8, 2022
View reviewed changes
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphFlinkCDCLoader.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphFlinkCDCLoader.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphOutputFormat.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphOutputFormat.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphOutputFormat.java Outdated Show resolved Hide resolved
hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphOutputFormat.java Outdated Show resolved Hide resolved
javeme
javeme previously approved these changes Jun 8, 2022
View reviewed changes
javeme
javeme previously approved these changes Jun 13, 2022
View reviewed changes
@simon824 simon824 mentioned this pull request Jun 16, 2022
Closed
1 task
diaohancai
diaohancai approved these changes Jun 16, 2022
View reviewed changes
Copy link

@diaohancai diaohancai left a comment

Choose a reason for hiding this comment

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

LGTM

javeme
javeme approved these changes Jun 21, 2022
View reviewed changes
Copy link
Contributor

@javeme javeme left a comment

Choose a reason for hiding this comment

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

LGTM

hugegraph-loader/src/main/java/com/baidu/hugegraph/loader/flink/HugeGraphFlinkCDCLoader.java
@@ -93,7 +93,7 @@ private MySqlSource<String> buildMysqlSource(InputStruct struct) {
port = uriBuilder.getPort();
} catch (URISyntaxException e) {
throw new IllegalArgumentException(
String.format("Failed to parse Url(%s) to get hostName and port", url), e);
String.format("Failed to parse url(%s) to get hostName and port", url), e);
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny: we can use hostname

@javeme javeme merged commit 87869a1 into apache:master Jun 21, 2022
@javeme javeme added the loader hugegraph-loader label Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diaohancai diaohancai diaohancai approved these changes

@imbajin imbajin imbajin approved these changes

@seagle-yuan seagle-yuan seagle-yuan approved these changes

@javeme javeme javeme approved these changes

Assignees
No one assigned
Labels
loader hugegraph-loader
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Introduce HugeGraphFlinkCDCLoader
5 participants
@simon824 @javeme @seagle-yuan @imbajin @diaohancai