Copilot commented on code in PR #10914: URL: https://github.com/apache/gravitino/pull/10914#discussion_r3195160125
########## core/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/base/TestAuthMappers.java: ########## @@ -0,0 +1,124 @@ +/* + * 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.File; +import java.io.IOException; +import java.util.List; +import java.util.UUID; +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.Test; +import org.mockito.Mockito; + +public class TestAuthMappers { + + private static final String JDBC_STORE_PATH = + "/tmp/gravitino_jdbc_authMappers_" + UUID.randomUUID().toString().replace("-", ""); + private static final String DB_DIR = JDBC_STORE_PATH + "/testdb"; + private static JDBCBackend backend; + private static EntityChangeLogMapper entityChangeLogMapper; + private static SqlSession sharedSession; + + @BeforeAll + public static void setup() throws Exception { + File dir = new File(DB_DIR); + if (dir.exists()) { + FileUtils.deleteDirectory(dir); + } + dir.mkdirs(); + + 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", DB_DIR)); + 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); + } + + @AfterAll + public static void tearDown() throws IOException { + if (sharedSession != null) { + sharedSession.close(); + } + if (backend != null) { + backend.close(); + } + File dir = new File(JDBC_STORE_PATH); + if (dir.exists()) { + FileUtils.deleteDirectory(dir); + } + } + + @Test + void testEntityChangeLogInsertAndSelect() { + long now = System.currentTimeMillis(); + entityChangeLogMapper.insertChange( + "metalake1", "TABLE", "cat.schema.tbl", OperateType.ALTER, now); + + List<EntityChangeRecord> records = entityChangeLogMapper.selectChanges(now - 1, 10); + Assertions.assertEquals(1, records.size()); + EntityChangeRecord record = records.get(0); + Assertions.assertEquals("metalake1", record.getMetalakeName()); + Assertions.assertEquals("TABLE", record.getEntityType()); + Assertions.assertEquals("cat.schema.tbl", record.getFullName()); + Assertions.assertEquals(OperateType.ALTER, record.getOperateType()); + Assertions.assertEquals(now, record.getCreatedAt()); + } + + @Test + void testEntityChangeLogPruneOldEntries() { + long old = 1000L; + long recent = System.currentTimeMillis(); + entityChangeLogMapper.insertChange( + "metalake1", "SCHEMA", "cat.schema", OperateType.INSERT, old); + entityChangeLogMapper.insertChange( + "metalake1", "TABLE", "cat.schema.tbl", OperateType.DROP, recent); + + entityChangeLogMapper.pruneOldEntries(old + 1); + + List<EntityChangeRecord> after = entityChangeLogMapper.selectChanges(0L, 100); + Assertions.assertEquals(1, after.size()); + Assertions.assertEquals(recent, after.get(0).getCreatedAt()); Review Comment: The two tests share the same persistent H2 database and don’t clear `entity_change_log` between test cases. Since `testEntityChangeLogInsertAndSelect` inserts a row and `testEntityChangeLogPruneOldEntries` later calls `selectChanges(0L, 100)` and asserts `size == 1`, the second test becomes order-dependent and can fail when prior rows exist. Clear the table (or recreate the DB) in a `@BeforeEach`, or scope assertions to the timestamps inserted by the current test (e.g., query after `recent - 1`). ########## core/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/base/TestAuthMappers.java: ########## @@ -0,0 +1,124 @@ +/* + * 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.File; +import java.io.IOException; +import java.util.List; +import java.util.UUID; +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.Test; +import org.mockito.Mockito; + +public class TestAuthMappers { + + private static final String JDBC_STORE_PATH = + "/tmp/gravitino_jdbc_authMappers_" + UUID.randomUUID().toString().replace("-", ""); + private static final String DB_DIR = JDBC_STORE_PATH + "/testdb"; + private static JDBCBackend backend; Review Comment: This test hard-codes its working directory under `/tmp`, which makes it less portable (and can fail in restricted environments). Prefer creating a temporary directory via `Files.createTempDirectory(...)` (as used in other tests) and derive the DB path from that directory. ########## 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 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 LIMIT #{maxRows}"; Review Comment: `selectEntityChanges` orders only by `created_at` and doesn’t return the `id` primary key. This makes polling/pagination ambiguous when multiple rows share the same `created_at` and `maxRows` is smaller than the number of rows for that timestamp (callers can re-read the same rows or get stuck). Consider selecting `id` as well and ordering by `(created_at, id)` so consumers can advance a stable cursor. ########## core/src/test/java/org/apache/gravitino/storage/relational/mapper/provider/base/TestAuthMappers.java: ########## @@ -0,0 +1,124 @@ +/* + * 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.File; +import java.io.IOException; +import java.util.List; +import java.util.UUID; +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.Test; +import org.mockito.Mockito; + +public class TestAuthMappers { Review Comment: The class/file name `TestAuthMappers` is misleading because the tests exercise `EntityChangeLogMapper` rather than auth mappers. Renaming the test class (and file) to something like `TestEntityChangeLogMapper` would make it easier to discover and maintain. ########## core/src/main/java/org/apache/gravitino/storage/relational/service/TableMetaService.java: ########## @@ -207,6 +215,20 @@ public <E extends Entity & HasIdentifier> TableEntity updateTable( TableColumnMetaService.getInstance() .updateColumnPOsFromTableDiff(oldTableEntity, newTableEntity, newTablePO); } + }, + () -> { + if (isRenamed && updateResult.get() > 0) { + long now = System.currentTimeMillis(); + SessionUtils.doWithoutCommit( + EntityChangeLogMapper.class, + mapper -> + mapper.insertChange( + metalakeName, + Entity.EntityType.TABLE.name(), + oldFullName, + OperateType.ALTER, + now)); + } Review Comment: New behavior is introduced here (emitting `entity_change_log` rows on rename/drop), but the existing relational service tests don’t appear to assert this side effect yet. Please add focused tests (e.g., in the existing `TestTableMetaService`/related relational service tests) that verify the expected changelog row is inserted for rename and delete paths, including the correct `operate_type` and `entity_full_name` semantics. -- 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]
