shaoyu-li commented on code in PR #13058: URL: https://github.com/apache/gravitino/pull/13058#discussion_r3985438700
########## 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: Agreed it's worth fixing, with a caveat on how far it can go. `name()` is an instance method, so selection has to instantiate every candidate at least once to learn its name — caching moves that from once per catalog to once per classloader, it doesn't remove it. And `LakehouseTableDelegatorFactory` caches *instances*, which this SPI can't: providers are one per catalog, each with its own `initialize()`. So: cache name-to-Class once, reflectively instantiate only the selected provider per catalog, surface duplicate names at discovery. The javadoc will say plainly what that does and does not buy. Separately, the lookup now survives a provider whose constructor or `name()` throws — a broken jar fails with `NoClassDefFoundError`, which isn't a `RuntimeException`, and one bad jar shouldn't stop every catalog from starting. -- 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]
