danny0405 commented on code in PR #19575: URL: https://github.com/apache/hudi/pull/19575#discussion_r3870639302
########## hudi-client/hudi-spark-client/src/main/java/org/apache/hudi/index/RecordIndexLookupMetrics.java: ########## @@ -0,0 +1,99 @@ +/* + * 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.index; + +import org.apache.hudi.common.engine.HoodieEngineContext; +import org.apache.hudi.common.metrics.Registry; +import org.apache.hudi.config.HoodieWriteConfig; +import org.apache.hudi.metrics.DistributedRegistry; +import org.apache.hudi.metrics.ExecutorMetricRegistry; +import org.apache.hudi.metrics.RecordIndexMetricNames; + +import java.util.Collection; +import java.util.HashSet; +import java.util.Set; + +/** Executor-side emission for the record index lookup counters. */ +public class RecordIndexLookupMetrics { + + private RecordIndexLookupMetrics() { + } + + /** + * The registry a lookup task collects into, or null when nothing should be collected. Captured in the + * lookup closure and passed to {@link #recordShardLookup}, so delivery is by closure capture rather + * than by name. + * + * <p>Requires the reporter to be on as well: with {@code hoodie.metrics.on} off there is nowhere to + * publish, and collecting would register an accumulator and scan every shard for nothing. + */ + public static Registry resolveRegistry(HoodieEngineContext context, HoodieWriteConfig config) { + if (!config.isMetricsOn() || !ExecutorMetricRegistry.RECORD_INDEX_LOOKUP.isEnabled(config)) { + return null; + } + // TBL_NAME has no default and Builder.validate() only requires BASE_PATH, so a config built without + // forTable() reaches here with a null name. getMetricRegistry dereferences it immediately. + String tableName = config.getTableName(); + if (tableName == null || tableName.isEmpty()) { + return null; + } + Registry registry = context.getMetricRegistry(tableName, + ExecutorMetricRegistry.RECORD_INDEX_LOOKUP.scopedName(config.getBasePath())); + // Only the accumulator-backed registry aggregates back to the driver. A LocalRegistry here would + // collect on the executor and be dropped on the floor, so report nothing instead. + if (!(registry instanceof DistributedRegistry)) { + return null; + } + // Runs on the driver before the closure ships, so anything held now predates this write. Publishing + // releases what it reports, which leaves a non-empty registry only after an attempt that never + // committed. Paths that tear metrics down between writes lose it anyway; Spark SQL DML and StreamSync + // do not, and this commit must not report work it did not do. + registry.clear(); Review Comment: [P2] Do not clear a table-shared accumulator at lookup construction The registry key is shared by every write client for this table, so this clear cannot establish a per-commit boundary when writers overlap. For example, writer A can finish its lookup, writer B can clear those counts here, and then either commit can publish zero, the other writer counts, or a mixture. That makes the advertised per-commit counters incorrect under supported multiwriter operation. Could the accumulator be scoped by the in-flight write or instant, or could collection be explicitly disabled for multiwriter configurations until that scoping exists? -- 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]
