nsivabalan commented on code in PR #18984:
URL: https://github.com/apache/hudi/pull/18984#discussion_r3654392563
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/ddl/QueryBasedDDLExecutor.java:
##########
@@ -210,29 +224,43 @@ public void touchPartitionsToTable(String tableName,
List<String> touchPartition
}
log.info("Touching partitions " + touchPartitions.size() + " on " +
tableName);
List<String> sqls = constructPartitionAlterStatements(tableName,
touchPartitions, PartitionAlterType.TOUCH);
- for (String sql : sqls) {
- runSQL(sql);
- }
+ runSQLs(sqls);
}
/**
* Builds SQL statements to either touch partitions or set their location.
- * TOUCH: one ALTER TABLE ... TOUCH PARTITION (p1) PARTITION (p2) ...
- * SET_LOCATION: one ALTER TABLE ... PARTITION (p) SET LOCATION '...' per
partition.
+ *
+ * <p>The first element of the returned list is always a {@code USE database}
+ * statement. Hive 2.x's ALTER PARTITION ... SET LOCATION does not respect
the
+ * {@code db.table} qualifier (silently routes to the connection's current
+ * database), so the {@code USE} is load-bearing. Parallel execution paths
must
+ * run this statement on every worker before fanning out the rest.
+ *
+ * <p>TOUCH: when {@code HIVE_SYNC_BATCHING_ENABLED} is set, one
+ * {@code ALTER TABLE ... TOUCH PARTITION (p1) ...} per batch of
+ * {@code HIVE_BATCH_SYNC_PARTITION_NUM} partitions. Otherwise a single
statement
+ * covering all partitions, matching pre-batching behavior.
+ *
+ * <p>SET_LOCATION: one {@code ALTER TABLE ... PARTITION (p) SET LOCATION
'...'}
+ * per partition (Hive SQL does not support multi-partition SET LOCATION in
one
+ * statement).
*/
private List<String> constructPartitionAlterStatements(String tableName,
List<String> partitions, PartitionAlterType alterType) {
List<String> result = new ArrayList<>();
- // Hive 2.x doesn't like db.table name for operations, hence we need to
change to using the database first
String useDatabase = "USE " + HIVE_ESCAPE_CHARACTER + databaseName +
HIVE_ESCAPE_CHARACTER;
result.add(useDatabase);
String alterTablePrefix = "ALTER TABLE " + HIVE_ESCAPE_CHARACTER +
tableName + HIVE_ESCAPE_CHARACTER;
+ int batchSyncPartitionNum =
config.getBooleanOrDefault(HIVE_SYNC_BATCHING_ENABLED)
Review Comment:
You're right — `JDBCExecutor` doesn't override `runSQLs`, so with the flag
on it was splitting TOUCH into `batch_num` statements and then executing them
serially. Changed statement count and partial-application semantics for zero
benefit, and directly contradicted the documented "JDBC is unaffected"
contract. Fixed in `84691ce`.
I took the stronger of the two options you offered. The config read in
`constructPartitionAlterStatements` is replaced by a `getTouchBatchSize(int)`
hook: the base implementation returns the full partition count (one statement,
the long-standing behavior), and only `HiveQueryDDLExecutor` overrides it —
keyed on `driverPool.isPresent()` rather than on the config. Gating on actual
pool presence rather than the flag means the split structurally cannot take
effect on a path that would just execute the batches serially, so this can't
regress the same way if another subclass is added later.
Regression test added as `TestQueryBasedDDLExecutorTouchBatching` — asserts
a serial executor emits a single TOUCH statement with the flag on, that its SQL
is byte-identical with the flag on and off, and (as a control) that a
parallel-dispatch executor still splits. It's a unit test on SQL shape rather
than an e2e run, so it pins the contract without needing Hive up.
Also tightened the `batching.enabled` doc to lead with "has no effect in HMS
or JDBC mode".
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/ddl/HiveQueryDDLExecutor.java:
##########
@@ -73,6 +86,15 @@ public HiveQueryDDLExecutor(HiveSyncConfig config,
IMetaStoreClient metaStoreCli
if (this.hiveDriver != null) {
this.hiveDriver.close();
}
+ // driverPool (if present) was already constructed by the caller before
this
+ // ctor ran; since we're about to throw, no one else will call close()
on it.
+ driverPool.ifPresent(pool -> {
Review Comment:
Confirmed — real gap. `super(config)` runs the `PartitionValueExtractor`
reflection before `HiveQueryDDLExecutor`'s try block is entered, so a bad
`hoodie.datasource.hive_sync.partition_extractor_class` throws where neither
catch covers the pool: the executor's catch never runs, and
`HoodieHiveSyncClient`'s catch just rethrows. The pool's worker threads and
Drivers leak.
Fixed in `e56b4e9` — `HoodieHiveSyncClient`'s constructor catch now closes
the pool. Putting it there rather than in the executor covers every window
between building the pool and handing ownership over, including the pre-try one
you identified. `close()` is idempotent, so overlapping with the executor's own
cleanup is harmless.
--
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]
