Copilot commented on code in PR #10914: URL: https://github.com/apache/gravitino/pull/10914#discussion_r3199753242
########## core/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/base/TestEntityChangeLogMapper.java: ########## @@ -0,0 +1,150 @@ +/* + * 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.base; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.sql.Connection; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.List; +import org.apache.commons.io.FileUtils; +import org.apache.gravitino.Config; +import org.apache.gravitino.Configs; +import org.apache.gravitino.storage.relational.JDBCBackend; +import org.apache.gravitino.storage.relational.mapper.EntityChangeLogMapper; +import org.apache.gravitino.storage.relational.po.auth.EntityChangeRecord; +import org.apache.gravitino.storage.relational.po.auth.OperateType; +import org.apache.gravitino.storage.relational.session.SqlSessionFactoryHelper; +import org.apache.ibatis.session.SqlSession; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; + +public class TestEntityChangeLogMapper { + + private static Path jdbcStorePath; + private static String dbDir; + private static JDBCBackend backend; + private static EntityChangeLogMapper entityChangeLogMapper; + private static SqlSession sharedSession; + + @BeforeAll + public static void setup() throws Exception { + jdbcStorePath = Files.createTempDirectory("gravitino_jdbc_entityChangeLog_"); + Path dbPath = jdbcStorePath.resolve("testdb"); + Files.createDirectories(dbPath); + dbDir = dbPath.toString(); + + Config config = Mockito.mock(Config.class); + Mockito.when(config.get(Configs.ENTITY_STORE)).thenReturn("relational"); + Mockito.when(config.get(Configs.ENTITY_RELATIONAL_STORE)).thenReturn("h2"); + Mockito.when(config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_URL)) + .thenReturn(String.format("jdbc:h2:file:%s;DB_CLOSE_DELAY=-1;MODE=MYSQL", dbDir)); + Mockito.when(config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_USER)).thenReturn("gravitino"); + Mockito.when(config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_PASSWORD)) + .thenReturn("gravitino"); + Mockito.when(config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_DRIVER)) + .thenReturn("org.h2.Driver"); + Mockito.when(config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_MAX_CONNECTIONS)).thenReturn(20); + Mockito.when(config.get(Configs.ENTITY_RELATIONAL_JDBC_BACKEND_WAIT_MILLISECONDS)) + .thenReturn(1000L); + + backend = new JDBCBackend(); + backend.initialize(config); + + sharedSession = SqlSessionFactoryHelper.getInstance().getSqlSessionFactory().openSession(true); + entityChangeLogMapper = sharedSession.getMapper(EntityChangeLogMapper.class); + } Review Comment: `sharedSession` is reused across all tests, but `@BeforeEach truncate()` deletes rows using a different `SqlSession`. Because MyBatis defaults `localCacheScope` to `SESSION`, `sharedSession` can return cached `selectChanges(...)` results from a previous test (e.g., `selectChanges(0L, 100)`), making the suite order-dependent/flaky. Prefer opening a fresh session/mapper per test, or perform the truncate using `sharedSession` and call `sharedSession.clearCache()` after cleanup. ########## core/src/main/java/org/apache/gravitino/storage/relational/mapper/provider/base/EntityChangeLogBaseSQLProvider.java: ########## @@ -0,0 +1,54 @@ +/* + * 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.base; + +import static org.apache.gravitino.storage.relational.mapper.EntityChangeLogMapper.ENTITY_CHANGE_LOG_TABLE_NAME; + +import org.apache.gravitino.storage.relational.po.auth.OperateType; +import org.apache.ibatis.annotations.Param; + +public class EntityChangeLogBaseSQLProvider { + + public String selectEntityChanges( + @Param("createdAtAfter") long createdAtAfter, @Param("maxRows") int maxRows) { + return "SELECT id, metalake_name as metalakeName, entity_type as entityType," + + " entity_full_name as fullName, operate_type as operateType, created_at as createdAt" + + " FROM " + + ENTITY_CHANGE_LOG_TABLE_NAME + + " WHERE created_at >= #{createdAtAfter} ORDER BY created_at, id LIMIT #{maxRows}"; + } Review Comment: `createdAtAfter` is named as an exclusive lower bound, but the SQL uses `created_at >= #{createdAtAfter}` (inclusive). This mismatch can easily cause duplicated reads or incorrect caller assumptions. Either change the predicate to `>` to match the parameter name, or keep `>=` but rename the parameter / add Javadoc that the bound is inclusive (and how callers should advance the cursor when multiple rows share the same timestamp). ########## core/src/main/java/org/apache/gravitino/storage/relational/po/auth/OperateType.java: ########## @@ -0,0 +1,52 @@ +/* + * 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.po.auth; + +/** + * Operate type emitted into {@code entity_change_log.operate_type}. + * + * <p>The numeric {@code code} is what is persisted in the database. Codes are stable forever and + * must never be reused: removing a value from this enum is allowed, but the same numeric code + * cannot subsequently be assigned to a different meaning, otherwise old log rows would be + * misinterpreted by readers. + */ +public enum OperateType { + ALTER(1), + DROP(2), + INSERT(3); Review Comment: `OperateType` / `EntityChangeRecord` are placed under `...storage.relational.po.auth`, but they represent general entity change-log/caching infrastructure rather than authorization/authn. This package name is misleading and will make future discoverability/ownership unclear. Consider moving them under a more appropriate package (e.g., `...po.changelog` or just `...po`) before the API spreads across more call sites. -- 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]
