[GitHub] [hudi] xushiyan commented on a diff in pull request #7068: [HUDI-5096] boolean param is broken in HiveSyncTool

[GitBox <gi...@apache.org>]

xushiyan commented on code in PR #7068:
URL: https://github.com/apache/hudi/pull/7068#discussion_r1006874024


##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncConfig.java:
##########
@@ -108,45 +108,45 @@ public static class HiveSyncConfigParams {
             + "instead of org.apache.hudi package. Use this when you are in the process of migrating from "
             + "com.uber.hoodie to org.apache.hudi. Stop using this after you migrated the table definition to "
             + "org.apache.hudi input format.")
-    public Boolean usePreApacheInputFormat;
+    public boolean usePreApacheInputFormat;

Review Comment:
   this will make the member variables default to `false` which may differ from the config property 's default value.



##########
hudi-sync/hudi-hive-sync/src/test/java/org/apache/hudi/hive/replication/TestHiveSyncGlobalCommitTool.java:
##########
@@ -69,6 +70,7 @@ private HiveSyncGlobalCommitParams getGlobalCommitConfig(String commitTime) thro
     params.loadedProps.setProperty(LOCAL_BASE_PATH, localCluster.tablePath(DB_NAME, TBL_NAME));
     params.loadedProps.setProperty(REMOTE_BASE_PATH, remoteCluster.tablePath(DB_NAME, TBL_NAME));
     params.loadedProps.setProperty(META_SYNC_GLOBAL_REPLICATE_TIMESTAMP.key(), commitTime);
+    params.loadedProps.setProperty(HIVE_USE_JDBC.key(), "true");

Review Comment:
   should not be needed if default value fixed



##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/replication/HiveSyncGlobalCommitParams.java:
##########
@@ -85,7 +85,9 @@ public void load() throws IOException {
 
   Properties mkGlobalHiveSyncProps(boolean forRemote) {
     TypedProperties props = new TypedProperties(loadedProps);
-    props.putAll(globalHiveSyncConfigParams.toProps());
+    globalHiveSyncConfigParams.toProps().entrySet().forEach(
+        e -> props.putIfAbsent(e.getKey(), e.getValue())
+    );

Review Comment:
   why this change? if anything not working, it should be `globalHiveSyncConfigParams.toProps()` to be fixed, not making a workaround here, which masks the root issue.



##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncConfig.java:
##########
@@ -108,45 +108,45 @@ public static class HiveSyncConfigParams {
             + "instead of org.apache.hudi package. Use this when you are in the process of migrating from "
             + "com.uber.hoodie to org.apache.hudi. Stop using this after you migrated the table definition to "
             + "org.apache.hudi input format.")
-    public Boolean usePreApacheInputFormat;
+    public boolean usePreApacheInputFormat;
     @Deprecated
     @Parameter(names = {"--use-jdbc"}, description = "Hive jdbc connect url")
-    public Boolean useJdbc;
+    public boolean useJdbc;

Review Comment:
   the member var should just be assigned to the config's default value, to be consistent



-- 
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