[HUDI-875] Abstract hudi-sync-common, and support hudi-hive-sync #1810

lw309637554 · 2020-07-08T16:13:06Z

Tips

Thank you very much for contributing to Apache Hudi.
Please review https://hudi.apache.org/contributing.html before opening a pull request.

What is the purpose of the pull request

Abstract hudi-sync-common ,and migrate the hudi-hive-sync to hudi-sync-hive. hudi-sync-hive implement the hudi-sync-common .

Then will support hudi-sync-aliyun-dla implement the hudi-sync-common .

This is the RFC https://cwiki.apache.org/confluence/display/HUDI/RFC+-+17+Abstract+common+meta+sync+module+support+multiple+meta+service.

And the old PR is #1716 ,which just abstract the hudi-sync-common.

Brief change log

(for example:)

Modify AnnotationLocation checkstyle rule in checkstyle.xml

Verify this pull request

(Please pick either of the following options)

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

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end.
Added HoodieClientWriteTest to verify the change.
Manually verified the change by running a job locally.

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.

garyli1019

@lw309637554 Thanks for your contribution! Overall looks good.
Open to discuss where to put these modules. I vote to put the base class to hudi-common and have separate modules for different query engines.

...ync/hudi-sync-common/src/main/java/org/apache/hudi/sync/common/AbstractSyncHoodieClient.java

hudi-sync/hudi-hive-sync/pom.xml

hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HoodieHiveClient.java

garyli1019 · 2020-07-09T03:52:57Z

hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/SchemaDifference.java

nit: alphabetical order

hudi-utilities/src/main/java/org/apache/hudi/utilities/deltastreamer/DeltaSync.java

hudi-utilities/src/main/java/org/apache/hudi/utilities/deltastreamer/HoodieDeltaStreamer.java

lw309637554 · 2020-07-09T12:03:19Z

@lw309637554 Thanks for your contribution! Overall looks good.
Open to discuss where to put these modules. I vote to put the base class to hudi-common and have separate modules for different query engines.

thanks so much for your very valuable suggestion.

lw309637554 · 2020-07-09T12:15:05Z

@vinothchandar I meet some mistakes, just opened a new PR.
In the new PR, I fix the build break. This is my thinks about backwards compatible .
a. about users code compatible: because do not modify the hivesync class name. Users do not need to modify these old code.
b. about users pom dependency compatible: because do not modify the module name hudi-hive-sync. Users not need to modify pom dependency
c. about users local jar files compatible: if users's local jars is do not shaded the hudi-hive-sync's
Indirect dependence. Old users just need add the hudi-sync-common.jar to their directory. Just like hudi-utilities-bundle modiry in this PR.
Reply: we can do this to put the hudi-sync-common base class to hudi-common
d. about sync parameters, just add new parameters. Compatible is ok.

what about your suggestion?

leesf · 2020-07-15T11:02:28Z

hudi-spark/src/main/scala/org/apache/hudi/HoodieSparkSqlWriter.scala

+    // for backward compatibility
+    if (hiveSyncEnabled) {
+      metaSyncEnabled = true
+      syncClientToolClass = DEFAULT_SYNC_CLIENT_TOOL_CLASS


if user sync both hive and dla meta, the dla meta would not get synced.?

yes, this will back compatibility

would users sync to both? down the line it may make sense to provide support for syncing to multiple things.

but even here, if we just append the HiveSync class when hiveSyncEnabled=true, we can support syncing to both Hive and dla?

yes, when user set hiveSyncEnabled and --sync-tool-classes, sync both hive and --sync-tool-classes make sense. i will fix it

leesf · 2020-07-15T11:12:23Z

hudi-sync/hudi-dla-sync/src/main/java/org/apache/hudi/dla/DLASyncTool.java

+    Option<String> lastCommitTimeSynced = Option.empty();
+    /*if (tableExists) {
+      lastCommitTimeSynced = hoodieDLAClient.getLastCommitTimeSynced(tableName);
+    }*/
+    LOG.info("Last commit time synced was found to be " + lastCommitTimeSynced.orElse("null"));


since dla meta do not support alter table properties yet, it would be simpler here

leesf

LGTM overall, left some minor comments.

lw309637554 · 2020-07-15T12:32:39Z

@vinothchandar @garyli1019 I think in this PR the hudi-sync abstract is ready. Expect your review. Thanks

the module abstract is
hudi-sync
hudi-hive-sync/
hudi-dla-sync/
about backwards compatible .
a. about users code compatible: because do not modify the hivesync class name. Users do not need to modify these old code.
b. about users pom dependency compatible: because do not modify the module name hudi-hive-sync. Users not need to modify pom dependency
c. about users local jar files compatible: if users's local jars is do not shaded the hudi-hive-sync's
Indirect dependence. Old users just need add the hudi-sync-common.jar to their directory. Just like hudi-utilities-bundle modiry in this PR.
d. about sync parameters, just add new parameters such as --enable-sync as new default parameter,also backwards compatible --enable-hive-sync.
some others works ,such as update the doc. will do in other issues.

what about your suggestion?

garyli1019

Hi @lw309637554 , I totally understand the importance of the backward capability. IMO, that will be great if we can remove the hive dependency from hudi-spark and hudi-utilities. If we treat syncHive separately, we still need to include some Hive related packages in these two modules.
I had this dependency issue before while I was testing the delta streamer. I didn't use Hive at all but I need to resolve some Hive dependency conflicts in my production environment. So I'd incline to sacrifice some backward capability and move all the Hive related packages to hudi-hive-sync. Do you think this is possible?
Happy to hear what you guys think.

hudi-spark/src/main/scala/org/apache/hudi/HoodieSparkSqlWriter.scala

lw309637554 · 2020-07-16T06:13:07Z

Hi @lw309637554 , I totally understand the importance of the backward capability. IMO, that will be great if we can remove the hive dependency from hudi-spark and hudi-utilities. If we treat syncHive separately, we still need to include some Hive related packages in these two modules.
I had this dependency issue before while I was testing the delta streamer. I didn't use Hive at all but I need to resolve some Hive dependency conflicts in my production environment. So I'd incline to sacrifice some backward capability and move all the Hive related packages to hudi-hive-sync. Do you think this is possible?
Happy to hear what you guys think.

this is a good point. I think remove hive dependency from hudi-spark and hudi-utilities is a another work . we can open another issue resolve it

garyli1019 · 2020-07-16T18:48:38Z

this is a good point. I think remove hive dependency from hudi-spark and hudi-utilities is a another work . we can open another issue resolve it

sounds good. follow up ticket: https://issues.apache.org/jira/browse/HUDI-1101

lw309637554 · 2020-07-17T00:11:39Z

this is a good point. I think remove hive dependency from hudi-spark and hudi-utilities is a another work . we can open another issue resolve it

sounds good. follow up ticket: https://issues.apache.org/jira/browse/HUDI-1101

very nice

lw309637554 · 2020-07-21T09:11:24Z

@vinothchandar The pr is ready overall. Can you help to review ?

vinothchandar · 2020-07-23T05:50:14Z

@lw309637554 can you please give me a couple days. I am trying to prioritize all the 0.6.0 blockers for now.

lw309637554 · 2020-07-24T09:22:03Z

couple

okay ,thanks

leesf · 2020-08-01T13:14:49Z

@lw309637554 would you please rebase and fix the conflicts.

lw309637554 · 2020-08-01T16:08:07Z

@lw309637554 would you please rebase and fix the conflicts.

okay, done

vinothchandar · 2020-08-03T03:34:47Z

This is on my plate for this week.

vinothchandar · 2020-08-04T04:04:22Z

IMO, that will be great if we can remove the hive dependency from hudi-spark and hudi-utilities

We can discuss on the JIRA. but this needs more thought. We want spark datasource write and deltastreamer to continue to sync to hive, when the write completes. So, its a necessary thing IMO

vinothchandar

Hive related changes and overall structure looks good to me. Thanks for an elegant implementation @lw309637554 .

Left some comments around making the hive vs meta handling more generic. Let me know what you think. if supporting multiple sync targets is a desirable thing, then it may make sense to structure like that.

vinothchandar · 2020-08-04T04:17:17Z

hudi-spark/src/main/scala/org/apache/hudi/HoodieSparkSqlWriter.scala

+    // for backward compatibility
+    if (hiveSyncEnabled) {
+      metaSyncEnabled = true
+      syncClientToolClass = DEFAULT_SYNC_CLIENT_TOOL_CLASS


would users sync to both? down the line it may make sense to provide support for syncing to multiple things.

but even here, if we just append the HiveSync class when hiveSyncEnabled=true, we can support syncing to both Hive and dla?

hudi-spark/src/main/scala/org/apache/hudi/HoodieSparkSqlWriter.scala

hudi-utilities/src/main/java/org/apache/hudi/utilities/deltastreamer/HoodieDeltaStreamer.java

packaging/hudi-hive-sync-bundle/pom.xml

vinothchandar · 2020-08-04T05:21:33Z

...lities/src/main/java/org/apache/hudi/utilities/deltastreamer/HoodieDeltaStreamerMetrics.java

+      if (hiveSync) {
+        Metrics.registerGauge(getMetricsName("deltastreamer", "hiveSyncDuration"), getDurationInMs(syncNs));
+      } else {
+        Metrics.registerGauge(getMetricsName("deltastreamer", "metaSyncDuration"), getDurationInMs(syncNs));


what if both hive and meta sync are off? we would still emit metrics for meta?

should we derive the metric name from the sync tool class. i.e instead of metaSyncDuration, we do dlaSyncDuration? that seems more usable and understandable

i have do it , different sync tool class have its own metrics with name of sync class

vinothchandar · 2020-08-04T05:23:01Z

hudi-utilities/src/main/java/org/apache/hudi/utilities/deltastreamer/DeltaSync.java

    // Send DeltaStreamer Metrics
-    metrics.updateDeltaStreamerMetrics(overallTimeMs, hiveSyncTimeMs);
+    metrics.updateDeltaStreamerMetrics(overallTimeMs, hiveSyncTimeMs, true);
+    metrics.updateDeltaStreamerMetrics(overallTimeMs, metaSyncTimeMs, false);


is there a way to do this by iterating over the configured sync tool classes? i.e only do it when sync is configured?

ok , have do this in syncMeta

vinothchandar · 2020-08-04T05:45:19Z

@lw309637554 I rebased this off master and also did some of the smaller stuff myself. if we can make a call on the multiple targets and the metrics questions, we can can resolve and land

lw309637554

@lw309637554 I rebased this off master and also did some of the smaller stuff myself. if we can make a call on the multiple targets and the metrics questions, we can can resolve and land

thanks @vinothchandar
a. about metrics questions: every sync class have a metric with the sync class name as metric name
b. when user set sync-hive-enable, just add sync-hive to the sync classes, all the sync classes will sync. as both sync hive and dla

lw309637554 · 2020-08-04T15:02:38Z

hudi-spark/src/main/scala/org/apache/hudi/HoodieSparkSqlWriter.scala

+    // for backward compatibility
+    if (hiveSyncEnabled) {
+      metaSyncEnabled = true
+      syncClientToolClass = DEFAULT_SYNC_CLIENT_TOOL_CLASS


yes, when user set hiveSyncEnabled and --sync-tool-classes, sync both hive and --sync-tool-classes make sense. i will fix it

hudi-utilities/src/main/java/org/apache/hudi/utilities/deltastreamer/HoodieDeltaStreamer.java

lw309637554 · 2020-08-04T15:57:35Z

...lities/src/main/java/org/apache/hudi/utilities/deltastreamer/HoodieDeltaStreamerMetrics.java

+      if (hiveSync) {
+        Metrics.registerGauge(getMetricsName("deltastreamer", "hiveSyncDuration"), getDurationInMs(syncNs));
+      } else {
+        Metrics.registerGauge(getMetricsName("deltastreamer", "metaSyncDuration"), getDurationInMs(syncNs));


i have do it , different sync tool class have its own metrics with name of sync class

lw309637554 · 2020-08-04T15:59:51Z

hudi-utilities/src/main/java/org/apache/hudi/utilities/deltastreamer/DeltaSync.java

    // Send DeltaStreamer Metrics
-    metrics.updateDeltaStreamerMetrics(overallTimeMs, hiveSyncTimeMs);
+    metrics.updateDeltaStreamerMetrics(overallTimeMs, hiveSyncTimeMs, true);
+    metrics.updateDeltaStreamerMetrics(overallTimeMs, metaSyncTimeMs, false);


ok , have do this in syncMeta

vinothchandar

@lw309637554 one issue with the datasource config parsing.

hudi-spark/src/main/scala/org/apache/hudi/HoodieSparkSqlWriter.scala

hudi-utilities/src/main/java/org/apache/hudi/utilities/deltastreamer/HoodieDeltaStreamer.java

hudi-sync/hudi-dla-sync/src/main/java/org/apache/hudi/dla/DLASyncTool.java

leesf · 2020-08-05T01:18:06Z

hudi-sync/hudi-dla-sync/src/main/java/org/apache/hudi/dla/HoodieDLAClient.java

please use // TODO here

leesf · 2020-08-05T01:21:08Z

...ync/hudi-sync-common/src/main/java/org/apache/hudi/sync/common/AbstractSyncHoodieClient.java

please change to warn

leesf · 2020-08-05T01:21:28Z

...ync/hudi-sync-common/src/main/java/org/apache/hudi/sync/common/AbstractSyncHoodieClient.java

leesf · 2020-08-05T01:23:05Z

hudi-utilities/src/main/java/org/apache/hudi/utilities/deltastreamer/DeltaSync.java

use HiveSyncTool.class.getName here?

leesf · 2020-08-05T01:25:46Z

hudi-spark/src/main/scala/org/apache/hudi/DataSourceOptions.scala

use HiveSyncTool.class.getName?

vinothchandar · 2020-08-05T04:36:30Z

@lw309637554 is this ready for a final review?

vinothchandar · 2020-08-05T04:42:04Z

...lities/src/main/java/org/apache/hudi/utilities/deltastreamer/HoodieDeltaStreamerMetrics.java

do we need the entire class name here? Would that not make for a long metric name? :)

May be have a getShortName() method for the AbstractSyncTool class and return "hive" and "dla" from them?

ok ,i will do it

vinothchandar

@leesf @lw309637554 I reviewed the logic again. It looks good to me.
Left one last suggestion on using a short name for metrics, vs using the class name as is.

I will let you both decide and @leesf can land this, after a final review

vinothchandar · 2020-08-05T14:49:42Z

@lw309637554 this seems ready?

lw309637554 · 2020-08-05T15:07:27Z

@lw309637554 this seems ready?

yes, but the test failed for some reason, i rerun it

leesf closed this Jul 9, 2020

leesf reopened this Jul 9, 2020

leesf assigned vinothchandar Jul 9, 2020

lw309637554 force-pushed the HUDI-875-lw branch from 72d2828 to 38c480a Compare July 9, 2020 03:56

garyli1019 reviewed Jul 9, 2020

View reviewed changes

lw309637554 force-pushed the HUDI-875-lw branch 2 times, most recently from 6bf22a2 to 7013097 Compare July 9, 2020 07:36

lw309637554 force-pushed the HUDI-875-lw branch 2 times, most recently from 65edf78 to 843f16a Compare July 15, 2020 03:25

[HUDI-875] Abstract hudi-sync-common, and support hudi-hive-sync

4c08849

lw309637554 force-pushed the HUDI-875-lw branch from 843f16a to 4c08849 Compare July 15, 2020 04:18

[HUDI-875] Abstract support hudi-dla-sync

3697950

lw309637554 force-pushed the HUDI-875-lw branch from 32bd090 to 3697950 Compare July 15, 2020 09:19

leesf reviewed Jul 15, 2020

View reviewed changes

garyli1019 reviewed Jul 16, 2020

View reviewed changes

hudi-spark/src/main/scala/org/apache/hudi/HoodieSparkSqlWriter.scala Show resolved Hide resolved

leesf self-assigned this Aug 1, 2020

[HUDI-875] merge master

458d7eb

Merge branch 'master' into pull/1810

fcc3a9c

vinothchandar approved these changes Aug 4, 2020

View reviewed changes

Smaller CR feedback

e350377

lw309637554 commented Aug 4, 2020

View reviewed changes

vinothchandar reviewed Aug 4, 2020

View reviewed changes

leesf reviewed Aug 5, 2020

View reviewed changes

hudi-sync/hudi-dla-sync/src/main/java/org/apache/hudi/dla/DLASyncTool.java Outdated Show resolved Hide resolved

leesf reviewed Aug 5, 2020

View reviewed changes

lw309637554 force-pushed the HUDI-875-lw branch from 3ba4653 to fe598ff Compare August 5, 2020 04:30

vinothchandar reviewed Aug 5, 2020

View reviewed changes

vinothchandar approved these changes Aug 5, 2020

View reviewed changes

lw309637554 force-pushed the HUDI-875-lw branch 2 times, most recently from 015ac1b to 856ef5a Compare August 5, 2020 09:55

[HUDI-875] complete metasync metrics and sync all syncclasses

3808c32

lw309637554 force-pushed the HUDI-875-lw branch from 856ef5a to 3808c32 Compare August 5, 2020 13:08

merge master

4dae21d

lw309637554 force-pushed the HUDI-875-lw branch from 5014e14 to 4dae21d Compare August 6, 2020 03:07

vinothchandar merged commit 51ea27d into apache:master Aug 6, 2020

[HUDI-875] Abstract hudi-sync-common, and support hudi-hive-sync #1810

[HUDI-875] Abstract hudi-sync-common, and support hudi-hive-sync #1810

Uh oh!

Conversation

lw309637554 commented Jul 8, 2020

Tips

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

Uh oh!

garyli1019 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lw309637554 commented Jul 9, 2020

Uh oh!

lw309637554 commented Jul 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leesf Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leesf left a comment

Choose a reason for hiding this comment

Uh oh!

lw309637554 commented Jul 15, 2020

Uh oh!

garyli1019 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lw309637554 commented Jul 16, 2020

Uh oh!

garyli1019 commented Jul 16, 2020

Uh oh!

lw309637554 commented Jul 17, 2020

Uh oh!

lw309637554 commented Jul 21, 2020

Uh oh!

vinothchandar commented Jul 23, 2020

Uh oh!

lw309637554 commented Jul 24, 2020

Uh oh!

leesf commented Aug 1, 2020

Uh oh!

lw309637554 commented Aug 1, 2020

Uh oh!

vinothchandar commented Aug 3, 2020

Uh oh!

vinothchandar commented Aug 4, 2020

Uh oh!

vinothchandar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leesf Jul 15, 2020 •

edited

Loading

leesf Aug 5, 2020 •

edited

Loading