Copilot commented on code in PR #11167:
URL: https://github.com/apache/gravitino/pull/11167#discussion_r3272290510
##########
core/src/main/java/org/apache/gravitino/GravitinoEnv.java:
##########
@@ -523,6 +531,14 @@ public void shutdown() {
}
}
+ if (idpManager != null) {
+ try {
+ idpManager.close();
+ } catch (Exception e) {
+ LOG.warn("Failed to close IdpManager", e);
Review Comment:
`shutdown()` closes `idpManager` but keeps the reference non-null. After
shutdown, `idpManager()` will still succeed and return a potentially closed
manager (use-after-close risk), and tests currently have to manually null out
the field via reflection. Prefer setting `idpManager = null` after closing so
`idpManager()` correctly reflects the environment lifecycle and
re-initialization paths don't need test-only reflection.
##########
core/src/main/java/org/apache/gravitino/GravitinoEnv.java:
##########
@@ -458,6 +460,12 @@ public StatisticDispatcher statisticDispatcher() {
return statisticDispatcher;
}
+ /** Get the built-in IdP manager implementation. */
+ public IdpManager idpManager() {
+ Preconditions.checkArgument(idpManager != null, "GravitinoEnv is not
initialized.");
+ return idpManager;
+ }
Review Comment:
`shutdown()` closes `idpManager` but keeps the reference non-null. After
shutdown, `idpManager()` will still succeed and return a potentially closed
manager (use-after-close risk), and tests currently have to manually null out
the field via reflection. Prefer setting `idpManager = null` after closing so
`idpManager()` correctly reflects the environment lifecycle and
re-initialization paths don't need test-only reflection.
##########
plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/IdpUserMetaSQLProviderFactory.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.storage.relational.mapper;
+
+import com.google.common.collect.ImmutableMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.gravitino.storage.relational.JDBCBackend.JDBCBackendType;
+import
org.apache.gravitino.storage.relational.mapper.provider.base.IdpUserMetaBaseSQLProvider;
+import
org.apache.gravitino.storage.relational.mapper.provider.h2.IdpUserMetaH2Provider;
+import
org.apache.gravitino.storage.relational.mapper.provider.postgresql.IdpUserMetaPostgreSQLProvider;
+import org.apache.gravitino.storage.relational.po.IdpUserPO;
+import org.apache.gravitino.storage.relational.session.SqlSessionFactoryHelper;
+import org.apache.ibatis.annotations.Param;
+
+public class IdpUserMetaSQLProviderFactory {
+ private static final Map<JDBCBackendType, IdpUserMetaBaseSQLProvider>
+ IDP_USER_META_SQL_PROVIDER_MAP =
+ ImmutableMap.of(
+ JDBCBackendType.MYSQL, new IdpUserMetaMySQLProvider(),
+ JDBCBackendType.H2, new IdpUserMetaH2Provider(),
+ JDBCBackendType.POSTGRESQL, new IdpUserMetaPostgreSQLProvider());
+
+ public static IdpUserMetaBaseSQLProvider getProvider() {
+ String databaseId =
+ SqlSessionFactoryHelper.getInstance()
+ .getSqlSessionFactory()
+ .getConfiguration()
+ .getDatabaseId();
+
+ JDBCBackendType jdbcBackendType = JDBCBackendType.fromString(databaseId);
+ return IDP_USER_META_SQL_PROVIDER_MAP.get(jdbcBackendType);
Review Comment:
If `databaseId` is missing/unknown or `jdbcBackendType` is not in
`IDP_USER_META_SQL_PROVIDER_MAP`, this returns null and will later throw a
`NullPointerException` at the call site. It would be more diagnosable to fail
fast here with an explicit exception (e.g., `IllegalStateException`) including
`databaseId`/`jdbcBackendType`. The same pattern appears in the
group/group-user-rel provider factories.
##########
core/src/main/java/org/apache/gravitino/storage/relational/provider/IdpMetaProviderLoader.java:
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.storage.relational.provider;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ServiceLoader;
+
+/**
+ * Loads the single plugin-backed IdP metadata provider implementation from
the runtime classpath.
+ */
+public final class IdpMetaProviderLoader {
+
+ private IdpMetaProviderLoader() {}
+
+ public static <T> T loadService(Class<T> serviceClass) {
+ List<T> services = new ArrayList<>();
+ for (T service : ServiceLoader.load(serviceClass)) {
+ services.add(service);
+ }
+
+ if (services.isEmpty()) {
+ throw new IllegalStateException(
+ String.format("No %s implementation found",
serviceClass.getSimpleName()));
+ }
+
+ if (services.size() > 1) {
+ throw new IllegalStateException(
+ String.format("Multiple %s implementations found",
serviceClass.getSimpleName()));
+ }
+
+ return services.get(0);
+ }
Review Comment:
`loadService` iterates `ServiceLoader` every time it is called. Since
`IdpUserMetaService`/`IdpGroupMetaService` invoke this on each operation, this
can repeatedly scan the classpath and repeatedly instantiate providers, which
is unnecessarily expensive and can be problematic if a provider is stateful.
Consider caching the resolved singleton per `serviceClass` (e.g., a static
`ConcurrentHashMap<Class<?>, Object>` with lazy init) so provider resolution
happens once per JVM/classloader.
##########
core/src/main/java/org/apache/gravitino/IdpManagerFactory.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.ServiceLoader;
+import org.apache.gravitino.authorization.IdpManager;
+
+/**
+ * This class is responsible for creating instances of IdpManager
implementations. IdpManager
+ * implementations are used to manage built-in IdP users and groups within the
Apache Gravitino
+ * framework.
+ */
+public class IdpManagerFactory {
+
+ // Private constructor to prevent instantiation of this factory class.
+ private IdpManagerFactory() {}
+
+ /**
+ * Creates an instance of IdpManager from the runtime classpath.
+ *
+ * @return An instance of IdpManager.
+ */
+ public static IdpManager createIdpManager() {
+ return loadService(IdpManager.class);
+ }
+
+ private static <T> T loadService(Class<T> serviceClass) {
+ List<T> services = new ArrayList<>();
+ for (T service : ServiceLoader.load(serviceClass)) {
+ services.add(service);
+ }
+
+ if (services.isEmpty()) {
+ throw new IllegalStateException(
+ String.format("No %s implementation found",
serviceClass.getSimpleName()));
+ }
+
+ if (services.size() > 1) {
+ throw new IllegalStateException(
+ String.format("Multiple %s implementations found",
serviceClass.getSimpleName()));
+ }
+
+ return services.get(0);
+ }
Review Comment:
This service-loading logic duplicates
`IdpMetaProviderLoader.loadService(...)` almost exactly. To reduce duplication
and keep behavior consistent (especially if caching or logging is added later),
consider reusing a shared utility (e.g., move a generic loader to a common
package and use it from both `IdpManagerFactory` and `IdpMetaProviderLoader`).
--
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]
