nsivabalan commented on code in PR #18064:
URL: https://github.com/apache/hudi/pull/18064#discussion_r2761001863
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/ddl/DDLExecutor.java:
##########
@@ -83,6 +83,14 @@ void createTable(String tableName, HoodieSchema
storageSchema, String inputForma
*/
void updatePartitionsToTable(String tableName, List<String>
changedPartitions);
+ /**
+ * Touches partitions for a given table.
Review Comment:
can we enhance the documentation. how does this differ
from`updatePartitionsToTable`
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HoodieHiveSyncClient.java:
##########
@@ -88,12 +90,12 @@ public HoodieHiveSyncClient(HiveSyncConfig config,
HoodieTableMetaClient metaCli
// disable jdbc and depend on metastore client for all hive registrations
try {
this.client = IMetaStoreClientUtil.getMSC(config.getHiveConf());
+ setMetaConf(config.getHiveConf());
if (!StringUtils.isNullOrEmpty(config.getString(HIVE_SYNC_MODE))) {
HiveSyncMode syncMode =
HiveSyncMode.of(config.getString(HIVE_SYNC_MODE));
switch (syncMode) {
case HMS:
- ddlExecutor = new HMSDDLExecutor(config, this.client);
- break;
+ throw new UnsupportedOperationException("HMS sync mode is not
supported.");
Review Comment:
sorry, why we had to make this change?
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/ddl/QueryBasedDDLExecutor.java:
##########
@@ -220,5 +220,31 @@ private List<String> constructChangePartitions(String
tableName, List<String> pa
}
return changePartitions;
}
+
+ @Override
+ public void touchPartitionsToTable(String tableName, List<String>
touchPartitions) {
+ if (touchPartitions.isEmpty()) {
+ log.info("No partitions to touch for " + tableName);
+ return;
+ }
+ log.info("Touching partitions " + touchPartitions.size() + " on " +
tableName);
+ List<String> sqls = constructTouchPartitions(tableName, touchPartitions);
+ for (String sql : sqls) {
+ runSQL(sql);
+ }
+ }
+
+ private List<String> constructTouchPartitions(String tableName, List<String>
partitions) {
Review Comment:
can we add additional arguments to `constructChangePartitions` and reuse
across both update and touch
##########
hudi-sync/hudi-hive-sync/src/test/java/org/apache/hudi/hive/TestHiveSyncTool.java:
##########
@@ -128,7 +128,6 @@ public class TestHiveSyncTool {
private static final List<Object> SYNC_MODES = Arrays.asList(
"hiveql",
- "hms",
Review Comment:
why removing "hms"
##########
hudi-sync/hudi-hive-sync/src/test/java/org/apache/hudi/hive/TestHiveSyncTool.java:
##########
@@ -476,6 +487,15 @@ public void testBasicSync(boolean
useSchemaFromCommitMetadata, String syncMode,
assertEquals(9, tablePartitions.size());
}
+ @Test
+ public void testHMSSyncModeDeprecation() throws Exception {
Review Comment:
I did not know we are deprecating hms mode.
can you clarify why?
and even if we have compelling reasons, can we raise a separate patch for
it.
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HoodieHiveSyncClient.java:
##########
@@ -88,12 +90,12 @@ public HoodieHiveSyncClient(HiveSyncConfig config,
HoodieTableMetaClient metaCli
// disable jdbc and depend on metastore client for all hive registrations
try {
this.client = IMetaStoreClientUtil.getMSC(config.getHiveConf());
+ setMetaConf(config.getHiveConf());
Review Comment:
can you help me understand, do we need this for every sync mode or just 1 of
them.
and why is it necessary now? For eg, hive sync thru hms mode has been
working for community users w/o issues. So, trying to gauge why we need this
change.
##########
hudi-sync/hudi-hive-sync/src/test/java/org/apache/hudi/hive/TestHiveSyncTool.java:
##########
@@ -365,13 +364,26 @@ public void testBasicSync(boolean
useSchemaFromCommitMetadata, String syncMode,
assertEquals(hivePartitions.size(), relativePartitionPaths.size());
assertTrue(relativePartitionPaths.containsAll(newPartition));
+ // Touch partitions
+ hiveClient.touchPartitionsToTable(HiveTestUtil.TABLE_NAME,
Arrays.asList());
+ assertEquals(7,
hiveClient.getAllPartitions(HiveTestUtil.TABLE_NAME).size(),
Review Comment:
not sure if the validation is helping us catch anything.
Even if someone changes something in touch partitions impl in future, likely
this test might still succeed right. for eg, say due to some bug, touch
partitions in HMSDDLExecutor did not even call the remote hms.
this is pretty much same as L355 right. So, irrespective of whether touch
partitions is called or not, these assertions are going to succeed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
