Skip to content

Conversation

@lw309637554
Copy link
Contributor

@lw309637554 lw309637554 commented Jun 8, 2020

Tips

What is the purpose of the pull request

Introduce a new pom module named hudi-common-sync, before start abstract common meta sync module support multiple meta service. Design is in https://cwiki.apache.org/confluence/display/HUDI/RFC+-+17+Abstract+common+meta+sync+module+support+multiple+meta+service

Brief change log

Introduce a new pom module named hudi-common-sync, before start abstract common meta sync module support multiple meta service.

Verify this pull request

(Please pick either of the following options)

This pull request is a trivial rework / code cleanup without any test coverage.

Committer checklist

  • Has a corresponding JIRA in PR title & commit

  • Commit message is descriptive of the change

  • CI is green

  • Necessary doc changes done or have another open PR

  • For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2020

Codecov Report

Merging #1716 into master will decrease coverage by 54.10%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #1716       +/-   ##
=============================================
- Coverage     72.25%   18.14%   -54.11%     
- Complexity      294      859      +565     
=============================================
  Files           374      352       -22     
  Lines         16371    15411      -960     
  Branches       1654     1525      -129     
=============================================
- Hits          11829     2797     -9032     
- Misses         3806    12256     +8450     
+ Partials        736      358      -378     
Impacted Files Coverage Δ Complexity Δ
.../java/org/apache/hudi/client/HoodieReadClient.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (ø%)
.../java/org/apache/hudi/metrics/MetricsReporter.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (ø%)
.../java/org/apache/hudi/common/model/ActionType.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (ø%)
...java/org/apache/hudi/io/HoodieRangeInfoHandle.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (ø%)
.../java/org/apache/hudi/hadoop/InputPathHandler.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (ø%)
...a/org/apache/hudi/exception/HoodieIOException.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (ø%)
...org/apache/hudi/table/action/commit/SmallFile.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (ø%)
...rg/apache/hudi/index/bloom/KeyRangeLookupTree.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (ø%)
...g/apache/hudi/exception/HoodieInsertException.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (ø%)
...g/apache/hudi/exception/HoodieUpsertException.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (ø%)
... and 340 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 acb1ada...82e999f. Read the comment docs.

@leesf
Copy link
Contributor

leesf commented Jun 10, 2020

@vinothchandar What do you think about the PR, introducing a new module.

@vinothchandar
Copy link
Member

Good with the module.. Few thoughts/suggestions

  • Can we nest this under hudi-sync as hudi-sync-common, hudi-sync-hive and you can then introduce the aliyun metastore as hudi-sync-aliyun?
  • We need to just ensure there are no upgrades/backwards incompatible changes for existing users .

Copy link
Member

Choose a reason for hiding this comment

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

is this pom same as the current hudi-hive-sync pom?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good with the module.. Few thoughts/suggestions

  • Can we nest this under hudi-sync as hudi-sync-common, hudi-sync-hive and you can then introduce the aliyun metastore as hudi-sync-aliyun?
  • We need to just ensure there are no upgrades/backwards incompatible changes for existing users .

thanks ,

  • i think "we nest this under hudi-sync as hudi-sync-common" is a good idea,i will modify like this
  • "there are no upgrades/backwards incompatible changes for existing users ." i will add more tests to guarantee

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this pom same as the current hudi-hive-sync pom?
not exactly the same,i will minimize it

@lw309637554 lw309637554 force-pushed the HUDI-875 branch 2 times, most recently from ef3c13f to 41a10f6 Compare June 14, 2020 15:53
@lw309637554
Copy link
Contributor Author

@vinothchandar What do you think about the updated PR

Copy link
Member

@vinothchandar vinothchandar left a comment

Choose a reason for hiding this comment

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

is the plan to eventually also move hudi-hive-sync under hudi-sync?

@vinothchandar
Copy link
Member

@umehrot2 @zhedoubushishi please weigh in with your thoughts as well

@lw309637554
Copy link
Contributor Author

is the plan to eventually also move hudi-hive-sync under hudi-sync?

yes,when /hudi-sync/hudi-sync-common is ready, will move hudi-hive-sync under hudi-sync

@vinothchandar
Copy link
Member

Sorry for the delay. Was spending time on another large review.

Can we make those changes in this pr itself? Would love to see end-end functionality working. IIRC your ultimate goal is to plug in alliyun metastore?

@lw309637554
Copy link
Contributor Author

Sorry for the delay. Was spending time on another large review.

Can we make those changes in this pr itself? Would love to see end-end functionality working. IIRC your ultimate goal is to plug in alliyun metastore?

ok , then i add the changes in this pr. aliyun metastore can plug is valuable for users.Now users already use kafka -> spark -> hudi + aliyun metastore -> presto + spark program.

@vinothchandar
Copy link
Member

Now users already use kafka -> spark -> hudi + aliyun metastore -> presto + spark program.

Thanks for letting me know.. Thats great! I am a fan of doing the refactor with a precise goal in mind.. so please feel free to reopen if you feel we need to restructure for something else.. AWS Glue also works with the current abstractions..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants