shaoyu-li commented on code in PR #13058: URL: https://github.com/apache/gravitino/pull/13058#discussion_r4008557434
########## catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/TableLocationProviderFactory.java: ########## @@ -0,0 +1,274 @@ +/* + * 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.gravitino.catalog.lakehouse.generic; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Preconditions; +import java.lang.ref.WeakReference; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Optional; +import java.util.ServiceConfigurationError; +import java.util.ServiceLoader; +import java.util.Set; +import java.util.WeakHashMap; +import java.util.stream.Collectors; +import org.apache.commons.lang3.StringUtils; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * A factory that discovers {@link TableLocationProvider}s through {@link ServiceLoader}. + * + * <p>Discovery is done once per class loader and remembered, because it is the expensive half: + * selecting by name requires every registered provider to be instantiated so that {@link + * TableLocationProvider#name()} can be called on it, and {@code name()} is an instance method, so + * there is no way to learn the names without doing that at least once. What the cache removes is + * repeating it for every catalog; it cannot remove the first pass. Creating a catalog after the + * first then instantiates only the provider it selected. + * + * <p>The remembered index holds classes, and a class keeps its class loader alive, so the entries + * are weak on both sides: the map is keyed weakly by class loader and holds each class through a + * {@link WeakReference}. A strong value would pin the loader of a dropped catalog through the very + * map meant to speed the next one up, which is the leak {@code [#12986]} removed elsewhere. A + * collected entry simply causes the next lookup to scan again. + */ +public class TableLocationProviderFactory { + + private static final Logger LOG = LoggerFactory.getLogger(TableLocationProviderFactory.class); + + /** + * Provider classes by lower-cased name, per class loader. Weak on both sides; see the class + * javadoc. Guarded by synchronization on the map itself rather than by a concurrent map, because + * {@link WeakHashMap} is not thread-safe and the map is touched once per catalog creation. + */ + private static final Map<ClassLoader, Index> INDEXES = Review Comment: Checked it against `IsolatedClassLoader` and you are right, so it is removed in 8ffd3c2. `isCatalogClass` matches `org.apache.gravitino.catalog.lakehouse.`, so `isSharedClass` returns false and the factory -- along with its static map -- is loaded once per isolated loader. The key is the loader that owns the map, so the map never holds more than one entry, and the weak references point at classes from the loader that keeps them alive anyway. The javadoc described a leak this codebase cannot produce, which is worse than no cache. Gone with it: `Index`'s weak values, the map, `invalidateCache()`, and the two tests that were pinning the cache. Discovery is a plain per-catalog scan now, matching `LakehouseTableDelegatorFactory` next door. One test remains on the topic, asserting that a second lookup does scan again -- the behaviour is now a deliberate choice rather than an oversight, so it is worth pinning in that direction. -- 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]
