[GitHub] [hudi] yihua commented on a diff in pull request #8301: [HUDI-5988] Add a param, Implement a full partition sync operation wh…

["yihua (via GitHub)" <gi...@apache.org>]

yihua commented on code in PR #8301:
URL: https://github.com/apache/hudi/pull/8301#discussion_r1187759698


##########
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/procedures/HiveSyncProcedure.scala:
##########
@@ -41,7 +41,8 @@ class HiveSyncProcedure extends BaseProcedure with ProcedureBuilder
     ProcedureParameter.optional(5, "mode", DataTypes.StringType, ""),
     ProcedureParameter.optional(6, "partition_fields", DataTypes.StringType, ""),
     ProcedureParameter.optional(7, "partition_extractor_class", DataTypes.StringType, ""),
-    ProcedureParameter.optional(8, "strategy", DataTypes.StringType, "")
+    ProcedureParameter.optional(8, "strategy", DataTypes.StringType, ""),
+    ProcedureParameter.optional(9, "partition_fixmode", DataTypes.StringType, "")

Review Comment:
   Let's fix the naming here too.



##########
hudi-sync/hudi-sync-common/src/main/java/org/apache/hudi/sync/common/HoodieSyncConfig.java:
##########
@@ -163,6 +163,11 @@ public class HoodieSyncConfig extends HoodieConfig {
       .defaultValue("")
       .withDocumentation("The spark version used when syncing with a metastore.");
 
+  public static final ConfigProperty<String> META_SYNC_PARTITION_FIXMODE = ConfigProperty
+      .key("hoodie.datasource.hive_sync.partition_fixmode")
+      .defaultValue("false")
+      .withDocumentation("Implement a full partition sync operation when partitions are lost.");

Review Comment:
   If the `incremental` is the naming, default should be `true`, i.e., "lastCommitTimeSynced is used as the baseline to synchronize the newly generated partitions after lastCommitTimeSynced".



##########
hudi-sync/hudi-sync-common/src/main/java/org/apache/hudi/sync/common/HoodieSyncConfig.java:
##########
@@ -226,6 +231,8 @@ public static class HoodieSyncConfigParams {
     public Boolean isConditionalSync;
     @Parameter(names = {"--spark-version"}, description = "The spark version")
     public String sparkVersion;
+    @Parameter(names = {"--partition-fixmode"}, description = "Implement a full partition sync operation when partitions are lost.")

Review Comment:
   Same here for naming.



##########
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/procedures/HiveSyncProcedure.scala:
##########
@@ -66,6 +67,7 @@ class HiveSyncProcedure extends BaseProcedure with ProcedureBuilder
     val partitionFields = getArgValueOrDefault(args, PARAMETERS(6)).get.asInstanceOf[String]
     val partitionExtractorClass = getArgValueOrDefault(args, PARAMETERS(7)).get.asInstanceOf[String]
     val strategy = getArgValueOrDefault(args, PARAMETERS(8)).get.asInstanceOf[String]
+    val partitionFixMode = getArgValueOrDefault(args, PARAMETERS(9)).get.asInstanceOf[String]

Review Comment:
   variable naming too.



##########
hudi-sync/hudi-sync-common/src/main/java/org/apache/hudi/sync/common/HoodieSyncConfig.java:
##########
@@ -163,6 +163,11 @@ public class HoodieSyncConfig extends HoodieConfig {
       .defaultValue("")
       .withDocumentation("The spark version used when syncing with a metastore.");
 
+  public static final ConfigProperty<String> META_SYNC_PARTITION_FIXMODE = ConfigProperty
+      .key("hoodie.datasource.hive_sync.partition_fixmode")
+      .defaultValue("false")
+      .withDocumentation("Implement a full partition sync operation when partitions are lost.");

Review Comment:
   Agree that `hoodie.datasource.hive_sync.incremental` is a better naming.  Since this can also apply to Glue Catalog sync, can we name it with `meta.sync`, i.e., `hoodie.meta.sync.incremental`?  @xushiyan what is the naming convention?  I see different prefixes are used, e.g., `hoodie.meta.sync`, `hoodie.datasource.meta_sync`, `hoodie.meta_sync`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org