hudi-agent commented on code in PR #19575: URL: https://github.com/apache/hudi/pull/19575#discussion_r3762969332
########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metrics/RecordIndexMetricNames.java: ########## @@ -0,0 +1,139 @@ +/* + * 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.metrics; + +import org.apache.hudi.common.metrics.Registry; +import org.apache.hudi.common.util.Option; +import org.apache.hudi.config.HoodieWriteConfig; + +import java.util.Collections; +import java.util.Map; + +/** + * Counter names for the record level index lookup phase, and the commit-boundary drain that publishes + * them. + * + * <p>Lives in hudi-client-common rather than hudi-spark-client so the shared commit path can reach it. + * Because the drain happens on that shared path, it applies to every engine write path -- Spark + * DataSource, Spark SQL and DeltaStreamer -- without any of them knowing about it. + */ +public class RecordIndexMetricNames { + + /** Registry name, keyed per table in {@code Registry.REGISTRY_MAP}. */ + public static final String REGISTRY_NAME = "HoodieRecordIndexLookup"; + + /** Prefix under which counters are stamped into commit metadata. */ + public static final String COMMIT_METADATA_PREFIX = "hoodie.rli.lookup."; + + /** Metric-name components used when the same counters are published to a reporter. */ + public static final String METRIC_ACTION = "rli"; + public static final String METRIC_QUALIFIER = "lookup"; + + // Callers. Counters are tagged so dedupe traffic is distinguishable from tag-location traffic. + public static final String CALLER_TAG_LOCATION = "tag"; + public static final String CALLER_DEDUPE = "dedupe"; + + public static final String RECORDS_LOOKED_UP = "records_looked_up"; + public static final String HITS = "hits"; + public static final String MISSES = "misses"; + public static final String SHARDS_READ = "shards_read"; + + private RecordIndexMetricNames() { + } + + /** Counter key for a metric attributed to a caller, e.g. {@code tag.hits}. */ + public static String key(String caller, String metric) { + return caller + "." + metric; + } + + /** + * Snapshots this table's record index lookup counters into the commit's extra metadata, without + * consuming them. + * + * <p>Split from {@link #publishAndRelease} so the counters survive a commit that never lands: a + * conflict in preCommit or a failure in commit would otherwise destroy them and publish reporter + * gauges for work that was rolled back. The caller releases them only once the commit has succeeded. + * + * @param commitMetadata the extra-metadata map being assembled for this commit + * @param config the write config, for the table name and the feature gate + * @return the snapshotted counters, to be handed to {@link #publishAndRelease} on success; empty + * when the feature is off or nothing was recorded Review Comment: 🤖 nit: `snapshotIntoCommitMetadata` mutates `commitMetadata` in-place AND returns a separate `Map<String, Long>` — two output channels from one call. Have you considered returning a small result object (or a `Pair`) that holds both the snapshot and whatever the caller needs to pass to `publishAndRelease`, so the side-effectful mutation is not implicit in the signature? <sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub> ########## hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metrics/RecordIndexMetricNames.java: ########## @@ -0,0 +1,139 @@ +/* + * 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.metrics; + +import org.apache.hudi.common.metrics.Registry; +import org.apache.hudi.common.util.Option; +import org.apache.hudi.config.HoodieWriteConfig; + +import java.util.Collections; +import java.util.Map; + +/** + * Counter names for the record level index lookup phase, and the commit-boundary drain that publishes + * them. + * + * <p>Lives in hudi-client-common rather than hudi-spark-client so the shared commit path can reach it. + * Because the drain happens on that shared path, it applies to every engine write path -- Spark + * DataSource, Spark SQL and DeltaStreamer -- without any of them knowing about it. Review Comment: 🤖 nit: the name `RecordIndexMetricNames` signals a constants class, but `snapshotIntoCommitMetadata` and `publishAndRelease` are substantive operations. Could you consider something like `RecordIndexLookupMetricsHelper` or `RecordIndexCommitDrain` — or splitting the constants into a separate `RecordIndexMetricNames` interface and putting the drain logic in a class with a name that reflects its responsibility? <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]
