Skip to content

Comments

feat: Introduce HugeGraphFlinkCDCLoader#291

Merged
javeme merged 11 commits intoapache:masterfrom
simon824:flinkLoader
Jun 21, 2022
Merged

feat: Introduce HugeGraphFlinkCDCLoader#291
javeme merged 11 commits intoapache:masterfrom
simon824:flinkLoader

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

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

@javeme javeme Jun 6, 2022

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

@simon824 simon824 Jun 7, 2022

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

@javeme javeme Jun 7, 2022

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
javeme
javeme reviewed Jun 8, 2022
View reviewed changes
javeme
javeme previously approved these changes Jun 8, 2022
View reviewed changes
javeme
javeme previously approved these changes Jun 13, 2022
View reviewed changes
imbajin
imbajin reviewed Jun 14, 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
} 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

@javeme javeme Jun 21, 2022

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

@javeme javeme javeme approved these changes

@imbajin imbajin imbajin approved these changes

+2 more reviewers

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

@diaohancai diaohancai diaohancai approved these changes

Reviewers whose approvals may not affect merge requirements

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