shaoyu-li commented on code in PR #13058: URL: https://github.com/apache/gravitino/pull/13058#discussion_r3985848079
########## catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/TableLocationProviderFactory.java: ########## @@ -0,0 +1,85 @@ +/* + * 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.base.Preconditions; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.ServiceLoader; +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}. */ +public class TableLocationProviderFactory { + + private static final Logger LOG = LoggerFactory.getLogger(TableLocationProviderFactory.class); + + private TableLocationProviderFactory() {} + + /** + * Creates and initializes the {@link TableLocationProvider} registered under the given name. + * + * <p>A new instance is returned on every call, so the caller owns its lifecycle and is + * responsible for closing it. + * + * @param name the provider name to look up, matched case-insensitively against {@link + * TableLocationProvider#name()} + * @param catalogProperties the properties of the catalog the provider belongs to + * @return the initialized provider + * @throws IllegalArgumentException if no provider, or more than one provider, is registered under + * the given name + */ + public static TableLocationProvider create(String name, Map<String, String> catalogProperties) { + Preconditions.checkArgument( + StringUtils.isNotBlank(name), "Table location provider name must not be blank"); + + ClassLoader cl = + Optional.ofNullable(Thread.currentThread().getContextClassLoader()) + .orElse(TableLocationProvider.class.getClassLoader()); + ServiceLoader<TableLocationProvider> loader = + ServiceLoader.load(TableLocationProvider.class, cl); + + List<TableLocationProvider> providers = + loader.stream() Review Comment: Done in 29a4ea9. Discovery builds a name-to-`Class` index once per class loader and remembers it, so after the first lookup only the selected provider is constructed. Duplicate names are recorded at discovery and fail the lookup that asks for them, rather than being found by whichever candidate happened to come first. The index is weak on both sides -- keyed weakly by class loader, holding each class through a `WeakReference` -- so it cannot pin the loader of a dropped catalog, which matters given #12986. The ceiling is what I described above and the javadoc now says it plainly: `name()` is an instance method, so the scan still has to construct every candidate once. Caching moves that from once per catalog to once per class loader; it does not remove it. be4c82f adds tests for the index itself, and a fix to the tests that went with it: the index is per class loader and the test class shares one, so only the first factory test was exercising a real `ServiceLoader` scan and the rest were reading back what it found. The index is now forgotten before each test, and two of them cover the cache directly -- a second lookup constructing no candidate beyond the one it selects, and an invalidated index being rescanned rather than reported as a missing provider. One correction to what I said above, while I am here. I claimed the lookup survives a broken jar. It survives a candidate whose constructor or `name()` throws, which is what the test provider does. It does not survive a services file naming a class that cannot be loaded at all: that fails the scan with a `ServiceConfigurationError` before any candidate is reached, and catching it per candidate would mean driving the loader's iterator by hand and risking a loop that never terminates. The javadoc and the docs say the narrower thing now. -- 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]
