danny0405 commented on code in PR #20165: URL: https://github.com/apache/hudi/pull/20165#discussion_r4213857763
########## hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncStats.java: ########## @@ -0,0 +1,101 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hudi.hive; + +import org.apache.hudi.common.util.Option; + +/** + * What one {@link HiveSyncTool#syncHoodieTable()} did, for callers that report on their syncs. + * + * <p>A MERGE_ON_READ table is registered as several Hive tables, all synced by one call. The + * durations add up over those tables. The partition count and the schema flag describe one of them, + * since each gets the same partitions and the same schema. + * + * <p>After a failed sync, the stats cover the steps that ran before the failure. + */ +public class HiveSyncStats { + + private long totalMs = -1; + private long schemaReadMs = -1; + private long partitionScanMs = -1; + private int partitionsAdded = 0; + private boolean schemaEvolved = false; + + /** + * Time spent reading the table schema from storage, or empty when no step read it, as when the + * metastore is already up to date with the latest commit. + */ + public Option<Long> getSchemaReadMs() { + return schemaReadMs < 0 ? Option.empty() : Option.of(schemaReadMs); + } + + /** + * Time spent finding the partitions to sync, from the timeline or by listing storage, or empty + * when no step looked for them. + */ + public Option<Long> getPartitionScanMs() { + return partitionScanMs < 0 ? Option.empty() : Option.of(partitionScanMs); + } + + /** + * Time spent on the rest of the sync, which is the calls to the metastore: checking for the + * database and tables, creating or altering them, and reading and writing their partitions. Empty + * when the sync did not run, because the metastore client could not be created. + */ + public Option<Long> getMetastoreMs() { + if (totalMs < 0) { + return Option.empty(); + } + return Option.of(Math.max(0, totalMs - Math.max(0, schemaReadMs) - Math.max(0, partitionScanMs))); Review Comment: Could we call this `otherDurationMs` / `remainingSyncDurationMs` and update the Flink metric name accordingly? The subtraction also includes local work such as `getPartitionEvents()` (partition comparison and path parsing), schema comparison, and logging. In a full sync, partition comparison runs after the scan timer stops, so expensive local processing can appear as metastore latency. Naming this as the remainder would preserve the simple implementation while accurately describing what it measures; measuring metastore latency specifically would require timing the client operations. ########## hudi-flink-datasource/hudi-flink/src/main/java/org/apache/hudi/sink/StreamWriteOperatorCoordinator.java: ########## @@ -486,12 +486,18 @@ private void syncHive() { */ public void doSyncHive() { HoodieTimer timer = HoodieTimer.start(); + HoodieTimer initTimer = HoodieTimer.start(); try (HiveSyncTool syncTool = hiveSyncContext.hiveSyncTool()) { + hiveSyncMetrics.updateInitDuration(initTimer.endTimer()); Review Comment: Suggestion: extract creation into a small `createHiveSyncTool()` helper that records initialization duration in `finally`. That would remove the initialization timer from this orchestration method and also record attempts that throw while connecting to the metastore. Currently a connection timeout records init time only when the exception is ignored and construction returns; otherwise the slow attempt is absent from the init histogram. I see the new test explicitly expects that exclusion, so this is a proposed adjustment to the metric contract, with the test updated to expect a sample for a failed construction too. ########## hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncTool.java: ########## @@ -600,6 +645,7 @@ private boolean syncPartitions(String tableName, List<PartitionEvent> partitionE if (!newPartitions.isEmpty()) { log.info("New Partitions {}", newPartitions); syncClient.addPartitionsToTable(tableName, newPartitions); + syncStats.recordPartitionsAdded(newPartitions.size()); Review Comment: Could we preserve the count of successfully added batches when a later batch fails? `HMSDDLExecutor.addPartitionsToTable()` makes multiple `add_partitions` calls, but this recording happens only after all of them return. For example, for a COW table, if the first batch adds 100 partitions and the second throws, the failed sync reports zero additions. On retry those 100 partitions already exist, so they are excluded from the next ADD list and never reach this counter. Recording confirmed progress at the batch boundary would avoid that undercount. A test that fails the second batch and then retries would cover this case. -- 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]
