Copilot commented on code in PR #11066:
URL: https://github.com/apache/gravitino/pull/11066#discussion_r3232718013
##########
plugins/idp-basic/build.gradle.kts:
##########
@@ -24,10 +24,29 @@ plugins {
}
dependencies {
+ implementation(project(":core"))
implementation(libs.bcprov.jdk18on)
implementation(libs.commons.lang3)
implementation(libs.guava)
+ implementation(libs.mybatis)
+ testImplementation(project(":api"))
+ testImplementation(project(":clients:client-java"))
+ testImplementation(project(":common"))
+ testImplementation(project(":lance:lance-common")) {
+ exclude("*")
Review Comment:
`exclude(\"*\")` is not a valid Gradle Kotlin DSL exclude signature and is
likely to fail the build script compilation. If the goal is to avoid pulling
any transitive dependencies from `:lance:lance-common`, configure the
dependency as non-transitive (e.g., `isTransitive = false`) or replace this
with explicit `exclude(group = ..., module = ...)` entries for the specific
transitive modules that need to be removed.
##########
plugins/idp-basic/src/test/java/org/apache/gravitino/storage/relational/BackendTestExtension.java:
##########
@@ -0,0 +1,164 @@
+/*
+ * 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;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.sql.SQLException;
+import java.util.Comparator;
+import java.util.List;
+import java.util.stream.Stream;
+import org.apache.gravitino.Config;
+import org.apache.gravitino.Configs;
+import org.apache.gravitino.integration.test.util.BaseIT;
+import org.apache.gravitino.storage.relational.session.SqlSessionFactoryHelper;
+import org.junit.jupiter.api.extension.AfterEachCallback;
+import org.junit.jupiter.api.extension.BeforeEachCallback;
+import org.junit.jupiter.api.extension.Extension;
+import org.junit.jupiter.api.extension.ExtensionContext;
+import org.junit.jupiter.api.extension.TestTemplateInvocationContext;
+import org.junit.jupiter.api.extension.TestTemplateInvocationContextProvider;
+
+class BackendTestExtension implements TestTemplateInvocationContextProvider {
+ private static final String DOCKER_TEST_FLAG = "dockerTest";
+
+ @Override
+ public boolean supportsTestTemplate(ExtensionContext context) {
+ return true;
+ }
+
+ @Override
+ public Stream<TestTemplateInvocationContext>
provideTestTemplateInvocationContexts(
+ ExtensionContext context) {
+ List<String> backends =
+ "true".equalsIgnoreCase(System.getenv(DOCKER_TEST_FLAG))
+ ? List.of("h2", "mysql", "postgresql")
+ : List.of("h2");
+ return backends.stream().map(BackendInvocationContext::new);
+ }
Review Comment:
The PR description says to enable multi-database execution by setting
`dockerTest=true`, but the extension checks an environment variable via
`System.getenv(\"dockerTest\")`. In typical Gradle usage, `-PdockerTest=true`
(project property) or `-DdockerTest=true` (system property) are more common
than exporting an env var; consider also checking
`System.getProperty(\"dockerTest\")` (and/or a Gradle-provided system property)
or update the documentation to explicitly require an environment variable.
##########
plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/IdpUserMetaSQLProviderFactory.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.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:
`getProvider()` may return `null` if `databaseId` is missing/unknown or if
the backend type is not present in `IDP_USER_META_SQL_PROVIDER_MAP`, which can
surface later as a confusing `NullPointerException` during SQL generation.
Prefer failing fast here with a clear exception message (e.g., throw
`IllegalStateException` when the provider is not found) so misconfiguration is
easier to diagnose.
##########
plugins/idp-basic/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/postgresql/IdpUserMetaPostgreSQLProvider.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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.provider.postgresql;
+
+import org.apache.gravitino.storage.relational.mapper.IdpUserMetaMapper;
+import
org.apache.gravitino.storage.relational.mapper.provider.base.IdpUserMetaBaseSQLProvider;
+import org.apache.ibatis.annotations.Param;
+
+public class IdpUserMetaPostgreSQLProvider extends IdpUserMetaBaseSQLProvider {
+
+ @Override
+ public String softDeleteIdpUser(Long userId) {
+ return "UPDATE "
+ + IdpUserMetaMapper.IDP_USER_TABLE_NAME
+ + " SET deleted_at = CAST(EXTRACT(EPOCH FROM CURRENT_TIMESTAMP) * 1000
AS BIGINT),"
+ + " current_version = current_version + 1,"
+ + " last_version = last_version + 1"
+ + " WHERE user_id = #{userId} AND deleted_at = 0";
+ }
+
+ @Override
+ public String deleteIdpUserMetasByLegacyTimeline(Long legacyTimeline,
@Param("limit") int limit) {
+ return "DELETE FROM "
+ + IdpUserMetaMapper.IDP_USER_TABLE_NAME
+ + " WHERE user_id IN (SELECT user_id FROM "
+ + IdpUserMetaMapper.IDP_USER_TABLE_NAME
+ + " WHERE deleted_at > 0 AND deleted_at < #{legacyTimeline} LIMIT
#{limit})";
Review Comment:
The PostgreSQL cleanup query applies `LIMIT` without an `ORDER BY`, which
makes the subset of rows deleted non-deterministic across executions and query
plans. If stable batching is important (e.g., to ensure oldest-deleted users
are cleaned first), add an explicit ordering inside the subquery (typically by
`deleted_at` and then `user_id`) before applying the limit.
--
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]
