wenzhenghu commented on code in PR #65126: URL: https://github.com/apache/doris/pull/65126#discussion_r3607287252
########## fe/fe-core/src/main/java/org/apache/doris/datasource/metacache/NameCacheValue.java: ########## @@ -0,0 +1,118 @@ +// 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.doris.datasource.metacache; + +import org.apache.doris.common.Pair; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Lists; + +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Objects; +import java.util.stream.Collectors; + +/** + * Immutable snapshot of remote/local names and the case-insensitive remote-name index. + */ +public final class NameCacheValue { + private final ImmutableList<Pair<String, String>> names; + private final ImmutableList<String> localNames; + private final ImmutableMap<String, String> lowerCaseToRemoteName; + private final ImmutableMap<String, String> localNameToRemoteName; + private final ImmutableSet<String> localNameSet; + + private NameCacheValue(List<Pair<String, String>> names) { + // Deep-copy each pair so callers cannot mutate the snapshot through reused Pair instances. + this.names = ImmutableList.copyOf(copyPairs(names)); + // Build the lower-case index with last-write-wins semantics so the snapshot does not + // introduce case-conflict validation beyond what the catalog-aware loader already enforces. + Map<String, String> indexBuilder = new java.util.HashMap<>(); + Map<String, String> localNameIndexBuilder = new java.util.HashMap<>(); + ImmutableList.Builder<String> localNamesBuilder = ImmutableList.builder(); + for (Pair<String, String> pair : this.names) { + indexBuilder.put(pair.key().toLowerCase(Locale.ROOT), pair.key()); + localNamesBuilder.add(pair.value()); + localNameIndexBuilder.put(pair.value(), pair.key()); + } + localNames = localNamesBuilder.build(); + lowerCaseToRemoteName = ImmutableMap.copyOf(indexBuilder); + localNameToRemoteName = ImmutableMap.copyOf(localNameIndexBuilder); + localNameSet = ImmutableSet.copyOf(localNames); + } + + public static NameCacheValue of(List<Pair<String, String>> names) { + return new NameCacheValue(Objects.requireNonNull(names, "names can not be null")); + } + + public static NameCacheValue empty() { + return of(ImmutableList.of()); + } + + public List<Pair<String, String>> names() { + // Return fresh Pair objects even though the list itself is immutable, so callers cannot + // mutate the published snapshot through reused Pair instances. + return ImmutableList.copyOf(copyPairs(names)); + } + + public List<String> localNames() { + return localNames; + } + + public String remoteNameOfLocalName(String localName) { + return localNameToRemoteName.get(localName); + } + + public boolean containsLocalName(String localName) { + return localNameSet.contains(localName); + } + + public String remoteNameForCaseInsensitiveLookup(String name) { + return lowerCaseToRemoteName.get(name.toLowerCase(Locale.ROOT)); + } + + public NameCacheValue withName(String remoteName, String localName) { + for (Pair<String, String> pair : names) { + if (pair.key().equals(remoteName) && !pair.value().equals(localName)) { + throw new IllegalArgumentException( + "remote name already maps to another local name: " + remoteName); + } + } + // Copy-on-write keeps readers on a stable snapshot while publishing a new value atomically. + List<Pair<String, String>> copy = Lists.newArrayList(names); Review Comment: Thanks for pointing this out. I agree this is a real P2 performance issue, and it appears to be a regression introduced by the immutable-snapshot / copy-on-write names-cache refactor in PR #65126. That refactor was introduced mainly for correctness. The goal was to make names-cache publication safe under concurrent loads and HMS incremental events, avoid in-place mutation of shared cache state, and ensure that lagging async/manual loads cannot overwrite newer event-driven updates. My current preference is not to address this in PR #65126. After several review rounds, the remaining issues in this PR are already being narrowed down, and fixing this one properly would likely require a broader redesign of the names-cache update/publication path. I am concerned that pulling that kind of architectural change into the current stage of the PR would expand the scope too much and create unnecessary late-stage rework. Also, this copy-on-write overhead has already been discussed in earlier review rounds, and parts of the impact on more frequent execution paths have already been reduced in the current PR series. The remaining concern is mainly concentrated on HMS event handling. In practice, CREATE/DROP is usually not a high-frequency path, and the worst-case cost is most visible on very large metadata sets. To make sure this does not get lost, I opened a follow-up issue here: https://github.com/apache/doris/issues/65779. Would it be reasonable to track it there after #65126 is merged, and then address it in a separate PR once we agree on the design? Possible directions I have in mind are sharding/striping the names cache, or reducing unnecessary deep copies by safely reusing immutable references. It might also make sense to handle it together with the other known HMS event follow-up items. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
