[HUDI-2883] Refactor hive sync tool / config to use reflection and standardize configs #4175

rmahindra123 · 2021-12-01T07:18:23Z

Refactor hive sync tool / config to use reflection and standardize configs

nsivabalan · 2022-01-17T22:47:12Z

@rmahindra123 : I can help review this patch. Can you rebase w/ latest master. also, lets sync up sometime to get some context on the patch.

nsivabalan · 2022-02-17T15:10:26Z

hudi-integ-test/src/main/java/org/apache/hudi/integ/testsuite/helpers/HiveServiceProvider.java

    if (this.config.isHiveLocal()) {
-      writer.getDeltaStreamerWrapper().getDeltaSyncService().getDeltaSync()
-          .syncHive(getLocalHiveServer().getHiveConf());
+      hiveSyncTool = new HiveSyncTool(writer.getWriteConfig().getProps(),


have we removed or deprecated DeltaSync.syncHive(...) in this patch? if not, would prefer to go via deltaSync syncHive api.

this is just a helper function right? The flow seems a bit weird to call the method from DeltaSync. I had removed the method in DeltaSync as well since this was the only place it was used. Can discuss more 1:1 if required.

...a-connect/src/main/java/org/apache/hudi/connect/writers/KafkaConnectTransactionServices.java

hudi-spark-datasource/hudi-spark-common/src/main/java/org/apache/hudi/DataSourceUtils.java

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

.../src/main/scala/org/apache/spark/sql/hudi/command/AlterHoodieTableDropPartitionCommand.scala

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

nsivabalan · 2022-02-17T19:43:54Z

hudi-sync/hudi-hive-sync/src/test/java/org/apache/hudi/hive/testutils/HiveTestUtil.java

+    hiveSyncProps.setProperty(HiveSyncConfig.META_SYNC_ASSUME_DATE_PARTITION.key(), "true");
+    hiveSyncProps.setProperty(HiveSyncConfig.HIVE_USE_PRE_APACHE_INPUT_FORMAT.key(), "false");
+    hiveSyncProps.setProperty(HiveSyncConfig.META_SYNC_PARTITION_FIELDS.key(), "datestr");
+    hiveSyncProps.setProperty(HiveSyncConfig.HIVE_BATCH_SYNC_PARTITION_NUM.key(), "3");


I don't see you are setting jdbcUrl and you are additionally setting this BATCH_SYNC_PARTITION_NUM in this patch. is that intentional ?

Good catch. Thanks for the thorough review.

I haven't removed it, its set in line 123.
hiveSyncProps.setProperty(HiveSyncConfig.HIVE_URL.key(), hiveTestService.getJdbcHive2Url());
2 For batch sync partition num, literally every test was setting it manually to HiveTestUtil.hiveSyncConfig.batchSyncNum = 3;

hudi-sync/hudi-sync-common/src/main/java/org/apache/hudi/sync/common/HoodieSyncConfig.java

hudi-sync/hudi-sync-common/src/main/java/org/apache/hudi/sync/common/util/SyncUtilHelpers.java

hudi-utilities/src/test/java/org/apache/hudi/utilities/TestHiveIncrementalPuller.java

nsivabalan · 2022-02-17T19:56:35Z

@rmahindra123 : just to be cautious. can you try test this patch out for an existing hudi table for the hive sync functionality.

create a new table and do hive sync using older hudi (master)
and then use this patch to do another round of ingestion. may be add a new partition this time. ensure hive sync works w/o any change in configs compared to 1.

nsivabalan · 2022-02-17T19:59:20Z

@xiarixiaoyao : can you help us test this patch out. Rajesh refactored hive sync quite a bit mostly around class structuring, config usages, instantiation etc. But if you can test this out and let us know if it looks good, would be nice.
btw, feel free to review the patch as well if you are interested.

nsivabalan

left some comments. for some of the resolved comments, I will sync up directly with you

hudi-sync/hudi-sync-common/src/main/java/org/apache/hudi/sync/common/HoodieSyncConfig.java

rmahindra123 · 2022-03-09T07:05:45Z

addressed reviewer comments

nsivabalan · 2022-03-10T15:07:49Z

@xiarixiaoyao : Did you get a chance to test this patch out.

xiarixiaoyao · 2022-03-11T01:42:08Z

@nsivabalan @rmahindra123 will review and test this pr This weekend

hudi-sync/hudi-hive-sync/src/test/java/org/apache/hudi/hive/testutils/HiveTestUtil.java

xushiyan · 2022-03-11T10:59:25Z

hudi-sync/hudi-hive-sync/src/test/java/org/apache/hudi/hive/testutils/HiveTestUtil.java

+    hiveSyncProps.setProperty(HiveSyncConfig.META_SYNC_PARTITION_FIELDS.key(), "datestr");
+    hiveSyncProps.setProperty(HiveSyncConfig.HIVE_BATCH_SYNC_PARTITION_NUM.key(), "3");
+
+    hiveSyncConfig = new HiveSyncConfig(hiveSyncProps);


i'd prefer to have builder pattern to reliably construct HiveSyncConfig instead of passing uncontrollable "raw" props.

xushiyan · 2022-03-11T11:09:21Z

hudi-sync/hudi-sync-common/src/main/java/org/apache/hudi/sync/common/AbstractSyncTool.java

+  protected TypedProperties props;

-  public AbstractSyncTool(Properties props, FileSystem fileSystem) {
+  public AbstractSyncTool(TypedProperties props, Configuration conf, FileSystem fs) {


I think a cleaner interface would be

Suggested change

public AbstractSyncTool(TypedProperties props, Configuration conf, FileSystem fs) {

public AbstractSyncTool(HoodieSyncConfig syncConfig, FileSystem fs) {

hadoopConf should be set by fs.getConf(). And props is not controllable; hard for user to decide what to pass in here.

HoodieSyncConfig is the matching config class for SyncTool as the names suggest. Internally we can still do props = syncConfig.getProps();

...ync/hudi-sync-common/src/test/java/org/apache/hudi/sync/common/util/TestSyncUtilHelpers.java

xushiyan · 2022-03-11T11:23:41Z

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

+      TypedProperties metaProps = new TypedProperties();
+      metaProps.putAll(props);
+      metaProps.put(HoodieSyncConfig.META_SYNC_BASE_PATH, cfg.targetBasePath);
+      metaProps.put(HoodieSyncConfig.META_SYNC_BASE_FILE_FORMAT, cfg.baseFileFormat);
+      if (props.getBoolean(HiveSyncConfig.HIVE_SYNC_BUCKET_SYNC.key(), HiveSyncConfig.HIVE_SYNC_BUCKET_SYNC.defaultValue())) {
+        metaProps.put(HiveSyncConfig.HIVE_SYNC_BUCKET_SYNC_SPEC, HiveSyncConfig.getBucketSpec(props.getString(HoodieIndexConfig.BUCKET_INDEX_HASH_FIELD.key()),
+            props.getInteger(HoodieIndexConfig.BUCKET_INDEX_NUM_BUCKETS.key())));
+      }


with builder pattern we should be able to simplify this sort of props construction

xushiyan · 2022-03-18T06:58:33Z

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

    this.sparkSchemaLengthThreshold = getIntOrDefault(HIVE_SYNC_SCHEMA_STRING_LENGTH_THRESHOLD);
    this.createManagedTable = getBooleanOrDefault(HIVE_CREATE_MANAGED_TABLE);
-    this.bucketSpec = props.getString(HIVE_SYNC_BUCKET_SYNC_SPEC.key(), null);
+    this.bucketSpec = getStringOrDefault(HIVE_SYNC_BUCKET_SYNC_SPEC);


@rmahindra123 Not sure why this was particularly set to null. seems like the default "" is ok.

xushiyan · 2022-03-18T06:58:53Z

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

  }

-  // enhance the similar function in child class
-  public static HiveSyncConfig copy(HiveSyncConfig cfg) {


this is not used.

...sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/replication/GlobalHiveSyncConfig.java

xushiyan · 2022-03-18T07:04:10Z

hudi-sync/hudi-sync-common/src/main/java/org/apache/hudi/sync/common/AbstractSyncTool.java

  @Deprecated
  public AbstractSyncTool(Properties props, FileSystem fileSystem) {
-    this(new TypedProperties(props), new Configuration(), fileSystem);
+    this(new TypedProperties(props), fileSystem.getConf(), fileSystem);


@rmahindra123 if user provides an fs, shall we always make use of its conf? Actually not sure why the new API allow passing a different conf rather than use the one from fileSystem

hudi-bot · 2022-03-19T11:24:10Z

CI report:

309116a Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

nsivabalan

LGTM

nsivabalan · 2022-03-21T21:52:06Z

hudi-sync/hudi-sync-common/src/main/java/org/apache/hudi/sync/common/AbstractSyncTool.java

  @Deprecated
  public AbstractSyncTool(Properties props, FileSystem fileSystem) {
-    this(new TypedProperties(props), new Configuration(), fileSystem);
+    this(new TypedProperties(props), fileSystem.getConf(), fileSystem);


…n and standardize configs (apache#4175)" This reverts commit 5f570ea.

…andardize configs (apache#4175) - Refactor hive sync tool / config to use reflection and standardize configs Co-authored-by: sivabalan <[email protected]> Co-authored-by: Rajesh Mahindra <[email protected]> Co-authored-by: Raymond Xu <[email protected]>

rmahindra123 changed the title ~~[HUDI-2883] [HUDI-2883] Refactor hive sync tool / config to use reflection and standardize configs~~ [HUDI-2883] Refactor hive sync tool / config to use reflection and standardize configs Dec 1, 2021

rmahindra123 force-pushed the rm_ref_hive_sync branch from f74bd30 to f6a00f3 Compare December 2, 2021 08:20

rmahindra123 changed the title ~~[HUDI-2883] Refactor hive sync tool / config to use reflection and standardize configs~~ [HUDI-2883] WIP Refactor hive sync tool / config to use reflection and standardize configs Dec 2, 2021

rmahindra123 marked this pull request as draft December 2, 2021 22:13

rmahindra123 force-pushed the rm_ref_hive_sync branch 5 times, most recently from 8e0bc42 to 9c13f28 Compare December 6, 2021 21:31

rmahindra123 changed the title ~~[HUDI-2883] WIP Refactor hive sync tool / config to use reflection and standardize configs~~ [HUDI-2883] Refactor hive sync tool / config to use reflection and standardize configs Dec 7, 2021

rmahindra123 marked this pull request as ready for review December 7, 2021 00:38

vinothchandar self-assigned this Dec 7, 2021

nsivabalan self-assigned this Jan 17, 2022

nsivabalan added the priority:critical Production degraded; pipelines stalled label Feb 8, 2022

rmahindra123 force-pushed the rm_ref_hive_sync branch from da0912c to 9202dc0 Compare February 13, 2022 18:45

nsivabalan requested changes Feb 17, 2022

View reviewed changes

rmahindra123 force-pushed the rm_ref_hive_sync branch from 9202dc0 to aa7efec Compare March 6, 2022 02:50

nsivabalan reviewed Mar 9, 2022

View reviewed changes

hudi-sync/hudi-sync-common/src/main/java/org/apache/hudi/sync/common/HoodieSyncConfig.java Show resolved Hide resolved

xushiyan self-assigned this Mar 11, 2022

xushiyan added priority:blocker Production down; release blocker and removed priority:critical Production degraded; pipelines stalled labels Mar 11, 2022

xushiyan reviewed Mar 11, 2022

View reviewed changes

xushiyan force-pushed the rm_ref_hive_sync branch 2 times, most recently from 95caf2b to 60d9472 Compare March 17, 2022 13:49

apache deleted a comment from hudi-bot Mar 17, 2022

xushiyan force-pushed the rm_ref_hive_sync branch from 51aabb2 to 3520d00 Compare March 18, 2022 06:55

xushiyan reviewed Mar 18, 2022

View reviewed changes

xushiyan force-pushed the rm_ref_hive_sync branch 3 times, most recently from d20111e to de57bff Compare March 18, 2022 13:37

apache deleted a comment from hudi-bot Mar 18, 2022

xushiyan force-pushed the rm_ref_hive_sync branch from de57bff to 7481149 Compare March 18, 2022 15:12

apache deleted a comment from hudi-bot Mar 18, 2022

nsivabalan and others added 3 commits March 19, 2022 17:47

Fixing integ test hive sync node

6843e6d

[HUDI-2883] Refactor hive sync tool and config

09f0556

fix props and tests

309116a

xushiyan force-pushed the rm_ref_hive_sync branch from f102d78 to 309116a Compare March 19, 2022 10:16

xushiyan mentioned this pull request Mar 20, 2022

[HUDI-2757] Implement Hudi AWS Glue sync #5076

Merged

nsivabalan approved these changes Mar 21, 2022

View reviewed changes

nsivabalan merged commit 5f570ea into apache:master Mar 22, 2022

alexeykudinkin pushed a commit to onehouseinc/hudi that referenced this pull request Mar 23, 2022

Revert "[HUDI-2883] Refactor hive sync tool / config to use reflectio…

6accc83

…n and standardize configs (apache#4175)" This reverts commit 5f570ea.

alexeykudinkin pushed a commit to onehouseinc/hudi that referenced this pull request Mar 23, 2022

Revert "[HUDI-2883] Refactor hive sync tool / config to use reflectio…

a657a94

…n and standardize configs (apache#4175)" This reverts commit 5f570ea.

alexeykudinkin pushed a commit to onehouseinc/hudi that referenced this pull request Mar 24, 2022

Revert "[HUDI-2883] Refactor hive sync tool / config to use reflectio…

7e0f687

…n and standardize configs (apache#4175)" This reverts commit 5f570ea.

matthiasdg mentioned this pull request Nov 8, 2022

[HUDI-5096] Upgrade jcommander to 1.78 #7068

Merged

4 tasks

	public AbstractSyncTool(TypedProperties props, Configuration conf, FileSystem fs) {
	public AbstractSyncTool(HoodieSyncConfig syncConfig, FileSystem fs) {

[HUDI-2883] Refactor hive sync tool / config to use reflection and standardize configs #4175

[HUDI-2883] Refactor hive sync tool / config to use reflection and standardize configs #4175

Uh oh!

Conversation

rmahindra123 commented Dec 1, 2021

Uh oh!

nsivabalan commented Jan 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

nsivabalan commented Feb 17, 2022

Uh oh!

nsivabalan commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nsivabalan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rmahindra123 commented Mar 9, 2022

Uh oh!

nsivabalan commented Mar 10, 2022

Uh oh!

xiarixiaoyao commented Mar 11, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hudi-bot commented Mar 19, 2022

CI report:

Uh oh!

nsivabalan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nsivabalan commented Feb 17, 2022 •

edited

Loading