hudi-agent commented on code in PR #18983:
URL: https://github.com/apache/hudi/pull/18983#discussion_r3654069723
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HoodieHiveSyncClient.java:
##########
@@ -201,6 +209,22 @@ private IMetaStoreClient
createMetaStoreClient(HiveSyncConfig config) {
}
}
+ private IMetaStoreClientPool maybeBuildPartitionClientPool(HiveSyncConfig
config) {
+ if (!config.getBooleanOrDefault(HIVE_SYNC_BATCHING_ENABLED)) {
+ return null;
+ }
+ if (config.getBooleanOrDefault(HIVE_SYNC_USE_SPARK_CATALOG)) {
+ // The Spark catalog client is constructed via reflection against a
Spark-side
+ // class and isn't compatible with the direct RetryingMetaStoreClient
pool path.
+ // Fall back to single-client sequential behavior rather than failing
the sync.
+ log.warn("hive_sync.batching.enabled=true is not supported with
use_spark_catalog=true; "
+ + "falling back to sequential partition sync.");
+ return null;
+ }
+ int size = config.getIntOrDefault(HIVE_SYNC_BATCHING_THREADS);
+ return new IMetaStoreClientPool(config, size);
Review Comment:
🤖 Since a new HoodieHiveSyncClient — and therefore this pool of N Thrift
connections + executor — is built and torn down on every sync invocation
(including per-commit streaming syncs where a commit may add/drop zero
partitions), would it be worth constructing the pool lazily on the first
partition op instead of eagerly in the constructor? As written we open N
connections up front even when there's nothing to fan out, and a failure while
opening the extra connections fails the whole sync.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncConfigHolder.java:
##########
@@ -122,6 +122,23 @@ public class HiveSyncConfigHolder {
.defaultValue(1000)
.markAdvanced()
.withDocumentation("The number of partitions one batch when synchronous
partitions to hive.");
+ public static final ConfigProperty<Boolean> HIVE_SYNC_BATCHING_ENABLED =
ConfigProperty
+ .key("hoodie.datasource.hive_sync.batching.enabled")
+ .defaultValue(false)
+ .markAdvanced()
+ .sinceVersion("1.1.0")
+ .withDocumentation("When true, HMS-mode partition operations
(add/update/touch/drop) are split into "
+ + "batches of `hoodie.datasource.hive_sync.batch_num` and dispatched
in parallel to a pool of "
+ + "IMetaStoreClient instances. Table-level operations (create/alter
table, last commit time, "
+ + "writer version) continue to use the single session client.
Default off; existing behavior "
+ + "is unchanged unless explicitly opted in.");
+ public static final ConfigProperty<Integer> HIVE_SYNC_BATCHING_THREADS =
ConfigProperty
+ .key("hoodie.datasource.hive_sync.batching.threads")
Review Comment:
🤖 nit: `batching.threads` reads as if it only controls the thread count, but
it actually sizes both the client pool and the worker pool together. Something
like `batching.pool_size` or `batching.parallelism` would better communicate
what the knob actually controls.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncConfigHolder.java:
##########
@@ -122,6 +122,23 @@ public class HiveSyncConfigHolder {
.defaultValue(1000)
.markAdvanced()
.withDocumentation("The number of partitions one batch when synchronous
partitions to hive.");
+ public static final ConfigProperty<Boolean> HIVE_SYNC_BATCHING_ENABLED =
ConfigProperty
+ .key("hoodie.datasource.hive_sync.batching.enabled")
+ .defaultValue(false)
+ .markAdvanced()
+ .sinceVersion("1.1.0")
+ .withDocumentation("When true, HMS-mode partition operations
(add/update/touch/drop) are split into "
+ + "batches of `hoodie.datasource.hive_sync.batch_num` and dispatched
in parallel to a pool of "
Review Comment:
🤖 nit: the doc references `` `hoodie.datasource.hive_sync.batch_num` `` but
the actual config key appears to be
`hoodie.datasource.hive_sync.batch_sync_partition_num` (i.e.
`HIVE_BATCH_SYNC_PARTITION_NUM`). Could you use the real key so the doc is
copy-paste correct?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
