Copilot commented on code in PR #10740:
URL: https://github.com/apache/gravitino/pull/10740#discussion_r3063848701
##########
core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java:
##########
@@ -552,46 +555,63 @@ public void testDropCatalog() throws Exception {
"value2",
PROPERTY_KEY5_PREFIX + "1",
"value3");
- String comment = "comment";
Catalog catalog =
- catalogManager.createCatalog(ident, Catalog.Type.RELATIONAL, provider,
comment, props);
-
- // Test drop catalog
- Exception exception =
- Assertions.assertThrows(
- CatalogInUseException.class, () ->
catalogManager.dropCatalog(ident));
- Assertions.assertTrue(exception.getMessage().contains("Catalog
metalake.test41 is in use"));
-
- Assertions.assertDoesNotThrow(() -> catalogManager.disableCatalog(ident));
-
- CatalogEntity oldEntity = entityStore.get(ident, EntityType.CATALOG,
CatalogEntity.class);
- FieldUtils.writeField(catalog, "entity", oldEntity, true);
-
- CatalogManager.CatalogWrapper catalogWrapper =
- Mockito.mock(CatalogManager.CatalogWrapper.class);
- Capability capability = Mockito.mock(Capability.class);
- CapabilityResult unsupportedResult = CapabilityResult.unsupported("Not
managed");
-
Mockito.doReturn(catalogWrapper).when(catalogManager).loadCatalogAndWrap(ident);
- Mockito.doReturn(catalog).when(catalogWrapper).catalog();
- Mockito.doReturn(capability).when(catalogWrapper).capabilities();
- Mockito.doReturn(unsupportedResult).when(capability).managedStorage(any());
-
- boolean dropped = catalogManager.dropCatalog(ident);
- Assertions.assertTrue(dropped);
-
- // Test drop non-existed catalog
- NameIdentifier ident1 = NameIdentifier.of("metalake", "test42");
- boolean dropped1 = catalogManager.dropCatalog(ident1);
- Assertions.assertFalse(dropped1);
-
- // Drop operation will update the cache
+ catalogManager.createCatalog(ident, Catalog.Type.RELATIONAL, provider,
"comment", props);
+ CatalogEntity originalEntity = entityStore.get(ident, EntityType.CATALOG,
CatalogEntity.class);
+ FieldUtils.writeField(catalog, "entity", originalEntity, true);
+
+ CatalogManager.CatalogWrapper staleWrapper =
+ Mockito.mock(CatalogManager.CatalogWrapper.class,
Mockito.RETURNS_DEEP_STUBS);
+ Mockito.doReturn(catalog).when(staleWrapper).catalog();
+
+ CatalogManager.CatalogWrapper freshWrapper =
+ Mockito.mock(CatalogManager.CatalogWrapper.class,
Mockito.RETURNS_DEEP_STUBS);
+ Catalog freshCatalog = Mockito.mock(Catalog.class);
+ CatalogEntity freshEntity =
+ CatalogEntity.builder()
+ .withId(originalEntity.id())
+ .withName("cache_race_test_renamed")
+ .withNamespace(originalEntity.namespace())
+ .withType(originalEntity.getType())
+ .withProvider(originalEntity.getProvider())
+ .withComment(originalEntity.getComment())
+ .withProperties(originalEntity.getProperties())
+ .withAuditInfo(originalEntity.auditInfo())
+ .build();
+ FieldUtils.writeField(freshCatalog, "entity", freshEntity, true);
+ Mockito.doReturn(freshCatalog).when(freshWrapper).catalog();
+
+ AtomicBoolean staleInserted = new AtomicBoolean(false);
+ Answer<CatalogManager.CatalogWrapper> insertStaleWrapper =
+ invocation -> {
+ if (staleInserted.compareAndSet(false, true)) {
+ catalogManager.getCatalogCache().put(NameIdentifier.of("metalake",
"cache_race_test_renamed"), staleWrapper);
Review Comment:
The long `put(...)` call exceeds typical Google Java Style line length and
also reduces readability in tests. Please wrap the arguments onto multiple
lines consistent with the rest of this file’s formatting.
```suggestion
catalogManager
.getCatalogCache()
.put(
NameIdentifier.of("metalake",
"cache_race_test_renamed"), staleWrapper);
```
##########
core/src/test/java/org/apache/gravitino/storage/relational/TestRelationalEntityStore.java:
##########
@@ -0,0 +1,203 @@
+/*
+ * 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 static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.function.Function;
+import org.apache.commons.lang3.reflect.FieldUtils;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.gravitino.Config;
+import org.apache.gravitino.Configs;
+import org.apache.gravitino.Entity;
+import org.apache.gravitino.EntityAlreadyExistsException;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.SupportsRelationOperations;
+import org.apache.gravitino.cache.NoOpsCache;
+import org.apache.gravitino.exceptions.NoSuchEntityException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.InOrder;
+import org.mockito.Mockito;
+
+public class TestRelationalEntityStore {
+
+ private RelationalEntityStore store;
+ private RelationalBackend backend;
+
+ @BeforeEach
+ void setUp() throws IllegalAccessException {
+ store = new RelationalEntityStore();
+ backend = Mockito.mock(RelationalBackend.class);
+
+ Config config = new Config(false) {};
+ config.set(Configs.CACHE_ENABLED, false);
+
+ FieldUtils.writeField(store, "backend", backend, true);
+ FieldUtils.writeField(store, "cache", Mockito.spy(new NoOpsCache(config)),
true);
+ }
+
+ @Test
+ void testUpdateInvalidatesCacheAfterBackendUpdate()
+ throws IOException, NoSuchEntityException, EntityAlreadyExistsException,
IllegalAccessException {
+ NameIdentifier ident = NameIdentifier.of("metalake", "catalog");
+ NoOpsCache cache = (NoOpsCache) FieldUtils.readField(store, "cache", true);
+
+ Mockito.doAnswer(
+ invocation -> {
+ Mockito.verify(cache, Mockito.never()).invalidate(ident,
Entity.EntityType.CATALOG);
+ return null;
+ })
+ .when(backend)
+ .update(eq(ident), eq(Entity.EntityType.CATALOG), any(Function.class));
+
+ store.update(ident, null, Entity.EntityType.CATALOG, entity -> entity);
+
+ InOrder inOrder = Mockito.inOrder(backend, cache);
+ inOrder.verify(backend).update(eq(ident), eq(Entity.EntityType.CATALOG),
any(Function.class));
+ inOrder.verify(cache).invalidate(ident, Entity.EntityType.CATALOG);
+ }
+
+ @Test
+ void testDeleteInvalidatesCacheAfterBackendDelete()
+ throws IOException, NoSuchEntityException, IllegalAccessException {
+ NameIdentifier ident = NameIdentifier.of("metalake", "catalog");
+ NoOpsCache cache = (NoOpsCache) FieldUtils.readField(store, "cache", true);
+
+ Mockito.doAnswer(
+ invocation -> {
+ Mockito.verify(cache, Mockito.never()).invalidate(ident,
Entity.EntityType.CATALOG);
+ return true;
+ })
+ .when(backend)
+ .delete(ident, Entity.EntityType.CATALOG, true);
+
+ Assertions.assertTrue(store.delete(ident, Entity.EntityType.CATALOG,
true));
+
+ InOrder inOrder = Mockito.inOrder(backend, cache);
+ inOrder.verify(backend).delete(ident, Entity.EntityType.CATALOG, true);
+ inOrder.verify(cache).invalidate(ident, Entity.EntityType.CATALOG);
+ }
+
+ @Test
+ void testInsertRelationInvalidatesCacheAfterBackendInsert()
+ throws IOException, IllegalAccessException {
+ NameIdentifier src = NameIdentifier.of("metalake", "catalog", "schema",
"table1");
+ NameIdentifier dst = NameIdentifier.of("metalake", "tag1");
+ NoOpsCache cache = (NoOpsCache) FieldUtils.readField(store, "cache", true);
+
+ Mockito.doAnswer(
+ invocation -> {
+ Mockito.verify(cache, Mockito.never())
+ .invalidate(src, Entity.EntityType.TABLE,
SupportsRelationOperations.Type.TAG_REL);
+ Mockito.verify(cache, Mockito.never())
+ .invalidate(dst, Entity.EntityType.TAG,
SupportsRelationOperations.Type.TAG_REL);
+ return null;
+ })
+ .when(backend)
+ .insertRelation(
+ SupportsRelationOperations.Type.TAG_REL,
+ src,
+ Entity.EntityType.TABLE,
+ dst,
+ Entity.EntityType.TAG,
+ true);
+
+ store.insertRelation(
+ SupportsRelationOperations.Type.TAG_REL,
+ src,
+ Entity.EntityType.TABLE,
+ dst,
+ Entity.EntityType.TAG,
+ true);
+
+ InOrder inOrder = Mockito.inOrder(backend, cache);
+ inOrder.verify(backend)
+ .insertRelation(
+ SupportsRelationOperations.Type.TAG_REL,
+ src,
+ Entity.EntityType.TABLE,
+ dst,
+ Entity.EntityType.TAG,
+ true);
+ inOrder.verify(cache)
+ .invalidate(src, Entity.EntityType.TABLE,
SupportsRelationOperations.Type.TAG_REL);
+ inOrder.verify(cache)
+ .invalidate(dst, Entity.EntityType.TAG,
SupportsRelationOperations.Type.TAG_REL);
+ }
+
+ @Test
+ void testUpdateEntityRelationsInvalidatesCacheAfterBackendUpdate()
+ throws IOException, NoSuchEntityException, EntityAlreadyExistsException,
IllegalAccessException {
+ NameIdentifier src = NameIdentifier.of("metalake", "catalog", "schema",
"table1");
+ NameIdentifier destToAdd = NameIdentifier.of("metalake", "tag1");
+ NameIdentifier destToRemove = NameIdentifier.of("metalake", "tag2");
+ NameIdentifier[] destEntitiesToAdd = new NameIdentifier[] {destToAdd};
+ NameIdentifier[] destEntitiesToRemove = new NameIdentifier[]
{destToRemove};
+ NoOpsCache cache = (NoOpsCache) FieldUtils.readField(store, "cache", true);
+
+ Mockito.doAnswer(
+ invocation -> {
+ Mockito.verify(cache, Mockito.never())
+ .invalidate(src, Entity.EntityType.TABLE,
SupportsRelationOperations.Type.TAG_REL);
+ Mockito.verify(cache, Mockito.never())
+ .invalidate(
+ destToAdd, Entity.EntityType.TABLE,
SupportsRelationOperations.Type.TAG_REL);
+ Mockito.verify(cache, Mockito.never())
+ .invalidate(
+ destToRemove,
Review Comment:
The destination identifiers here look like tags, but the test expectations
use `EntityType.TABLE` for destination-side invalidation. For tag relations
(e.g., TAG_METADATA_OBJECT_REL), destination cache keys should be invalidated
with `EntityType.TAG` (consistent with how `insertRelation` invalidates using
`dstType`), otherwise destination relation caches can remain stale.
##########
core/src/test/java/org/apache/gravitino/storage/relational/TestRelationalEntityStore.java:
##########
@@ -0,0 +1,203 @@
+/*
+ * 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 static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.function.Function;
+import org.apache.commons.lang3.reflect.FieldUtils;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.gravitino.Config;
+import org.apache.gravitino.Configs;
+import org.apache.gravitino.Entity;
+import org.apache.gravitino.EntityAlreadyExistsException;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.SupportsRelationOperations;
+import org.apache.gravitino.cache.NoOpsCache;
+import org.apache.gravitino.exceptions.NoSuchEntityException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.InOrder;
+import org.mockito.Mockito;
+
+public class TestRelationalEntityStore {
+
+ private RelationalEntityStore store;
+ private RelationalBackend backend;
+
+ @BeforeEach
+ void setUp() throws IllegalAccessException {
+ store = new RelationalEntityStore();
+ backend = Mockito.mock(RelationalBackend.class);
+
+ Config config = new Config(false) {};
+ config.set(Configs.CACHE_ENABLED, false);
+
+ FieldUtils.writeField(store, "backend", backend, true);
+ FieldUtils.writeField(store, "cache", Mockito.spy(new NoOpsCache(config)),
true);
+ }
+
+ @Test
+ void testUpdateInvalidatesCacheAfterBackendUpdate()
+ throws IOException, NoSuchEntityException, EntityAlreadyExistsException,
IllegalAccessException {
+ NameIdentifier ident = NameIdentifier.of("metalake", "catalog");
+ NoOpsCache cache = (NoOpsCache) FieldUtils.readField(store, "cache", true);
+
+ Mockito.doAnswer(
+ invocation -> {
+ Mockito.verify(cache, Mockito.never()).invalidate(ident,
Entity.EntityType.CATALOG);
+ return null;
+ })
+ .when(backend)
+ .update(eq(ident), eq(Entity.EntityType.CATALOG), any(Function.class));
+
+ store.update(ident, null, Entity.EntityType.CATALOG, entity -> entity);
+
Review Comment:
This call passes `null` for the `Class<E> type` parameter, which can prevent
generic type inference and fail compilation. Pass the concrete entity class
used in this test (e.g., `CatalogEntity.class` / relevant type) instead of
`null`.
##########
core/src/test/java/org/apache/gravitino/storage/relational/TestRelationalEntityStore.java:
##########
@@ -0,0 +1,203 @@
+/*
+ * 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 static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.function.Function;
+import org.apache.commons.lang3.reflect.FieldUtils;
+import org.apache.commons.lang3.tuple.Pair;
Review Comment:
`Pair` is imported but not used in this test class. This will fail
compilation/style checks (Spotless/removeUnusedImports). Remove the unused
import.
```suggestion
```
##########
core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java:
##########
@@ -552,46 +555,63 @@ public void testDropCatalog() throws Exception {
"value2",
PROPERTY_KEY5_PREFIX + "1",
"value3");
- String comment = "comment";
Catalog catalog =
- catalogManager.createCatalog(ident, Catalog.Type.RELATIONAL, provider,
comment, props);
-
- // Test drop catalog
- Exception exception =
- Assertions.assertThrows(
- CatalogInUseException.class, () ->
catalogManager.dropCatalog(ident));
- Assertions.assertTrue(exception.getMessage().contains("Catalog
metalake.test41 is in use"));
-
- Assertions.assertDoesNotThrow(() -> catalogManager.disableCatalog(ident));
-
- CatalogEntity oldEntity = entityStore.get(ident, EntityType.CATALOG,
CatalogEntity.class);
- FieldUtils.writeField(catalog, "entity", oldEntity, true);
-
- CatalogManager.CatalogWrapper catalogWrapper =
- Mockito.mock(CatalogManager.CatalogWrapper.class);
- Capability capability = Mockito.mock(Capability.class);
- CapabilityResult unsupportedResult = CapabilityResult.unsupported("Not
managed");
-
Mockito.doReturn(catalogWrapper).when(catalogManager).loadCatalogAndWrap(ident);
- Mockito.doReturn(catalog).when(catalogWrapper).catalog();
- Mockito.doReturn(capability).when(catalogWrapper).capabilities();
- Mockito.doReturn(unsupportedResult).when(capability).managedStorage(any());
-
- boolean dropped = catalogManager.dropCatalog(ident);
- Assertions.assertTrue(dropped);
-
- // Test drop non-existed catalog
- NameIdentifier ident1 = NameIdentifier.of("metalake", "test42");
- boolean dropped1 = catalogManager.dropCatalog(ident1);
- Assertions.assertFalse(dropped1);
-
- // Drop operation will update the cache
+ catalogManager.createCatalog(ident, Catalog.Type.RELATIONAL, provider,
"comment", props);
+ CatalogEntity originalEntity = entityStore.get(ident, EntityType.CATALOG,
CatalogEntity.class);
+ FieldUtils.writeField(catalog, "entity", originalEntity, true);
+
+ CatalogManager.CatalogWrapper staleWrapper =
+ Mockito.mock(CatalogManager.CatalogWrapper.class,
Mockito.RETURNS_DEEP_STUBS);
+ Mockito.doReturn(catalog).when(staleWrapper).catalog();
+
+ CatalogManager.CatalogWrapper freshWrapper =
+ Mockito.mock(CatalogManager.CatalogWrapper.class,
Mockito.RETURNS_DEEP_STUBS);
+ Catalog freshCatalog = Mockito.mock(Catalog.class);
+ CatalogEntity freshEntity =
+ CatalogEntity.builder()
+ .withId(originalEntity.id())
+ .withName("cache_race_test_renamed")
+ .withNamespace(originalEntity.namespace())
+ .withType(originalEntity.getType())
+ .withProvider(originalEntity.getProvider())
+ .withComment(originalEntity.getComment())
+ .withProperties(originalEntity.getProperties())
+ .withAuditInfo(originalEntity.auditInfo())
+ .build();
+ FieldUtils.writeField(freshCatalog, "entity", freshEntity, true);
+ Mockito.doReturn(freshCatalog).when(freshWrapper).catalog();
+
+ AtomicBoolean staleInserted = new AtomicBoolean(false);
+ Answer<CatalogManager.CatalogWrapper> insertStaleWrapper =
+ invocation -> {
+ if (staleInserted.compareAndSet(false, true)) {
+ catalogManager.getCatalogCache().put(NameIdentifier.of("metalake",
"cache_race_test_renamed"), staleWrapper);
+ }
+ return null;
+ };
+ Mockito.doAnswer(insertStaleWrapper)
+ .when(catalogManager)
+ .createCatalogWrapper(any(CatalogEntity.class), eq(null));
+ Mockito.doReturn(freshWrapper)
+ .when(catalogManager)
+ .createCatalogWrapper(any(CatalogEntity.class), eq(null));
Review Comment:
The `doReturn(freshWrapper)` stubbing here uses the same matchers as the
preceding `doAnswer(...)`, which overrides the earlier stub. As a result, the
intended race simulation never executes. Use sequential stubbing
(`doAnswer(...).doReturn(...)`) or consolidate into a single `Answer`.
##########
core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java:
##########
@@ -552,46 +555,63 @@ public void testDropCatalog() throws Exception {
"value2",
PROPERTY_KEY5_PREFIX + "1",
"value3");
- String comment = "comment";
Catalog catalog =
- catalogManager.createCatalog(ident, Catalog.Type.RELATIONAL, provider,
comment, props);
-
- // Test drop catalog
- Exception exception =
- Assertions.assertThrows(
- CatalogInUseException.class, () ->
catalogManager.dropCatalog(ident));
- Assertions.assertTrue(exception.getMessage().contains("Catalog
metalake.test41 is in use"));
-
- Assertions.assertDoesNotThrow(() -> catalogManager.disableCatalog(ident));
-
- CatalogEntity oldEntity = entityStore.get(ident, EntityType.CATALOG,
CatalogEntity.class);
- FieldUtils.writeField(catalog, "entity", oldEntity, true);
-
- CatalogManager.CatalogWrapper catalogWrapper =
- Mockito.mock(CatalogManager.CatalogWrapper.class);
- Capability capability = Mockito.mock(Capability.class);
- CapabilityResult unsupportedResult = CapabilityResult.unsupported("Not
managed");
-
Mockito.doReturn(catalogWrapper).when(catalogManager).loadCatalogAndWrap(ident);
- Mockito.doReturn(catalog).when(catalogWrapper).catalog();
- Mockito.doReturn(capability).when(catalogWrapper).capabilities();
- Mockito.doReturn(unsupportedResult).when(capability).managedStorage(any());
-
- boolean dropped = catalogManager.dropCatalog(ident);
- Assertions.assertTrue(dropped);
-
- // Test drop non-existed catalog
- NameIdentifier ident1 = NameIdentifier.of("metalake", "test42");
- boolean dropped1 = catalogManager.dropCatalog(ident1);
- Assertions.assertFalse(dropped1);
-
- // Drop operation will update the cache
+ catalogManager.createCatalog(ident, Catalog.Type.RELATIONAL, provider,
"comment", props);
+ CatalogEntity originalEntity = entityStore.get(ident, EntityType.CATALOG,
CatalogEntity.class);
+ FieldUtils.writeField(catalog, "entity", originalEntity, true);
+
+ CatalogManager.CatalogWrapper staleWrapper =
+ Mockito.mock(CatalogManager.CatalogWrapper.class,
Mockito.RETURNS_DEEP_STUBS);
+ Mockito.doReturn(catalog).when(staleWrapper).catalog();
+
+ CatalogManager.CatalogWrapper freshWrapper =
+ Mockito.mock(CatalogManager.CatalogWrapper.class,
Mockito.RETURNS_DEEP_STUBS);
+ Catalog freshCatalog = Mockito.mock(Catalog.class);
+ CatalogEntity freshEntity =
+ CatalogEntity.builder()
+ .withId(originalEntity.id())
+ .withName("cache_race_test_renamed")
+ .withNamespace(originalEntity.namespace())
+ .withType(originalEntity.getType())
+ .withProvider(originalEntity.getProvider())
+ .withComment(originalEntity.getComment())
+ .withProperties(originalEntity.getProperties())
+ .withAuditInfo(originalEntity.auditInfo())
+ .build();
+ FieldUtils.writeField(freshCatalog, "entity", freshEntity, true);
+ Mockito.doReturn(freshCatalog).when(freshWrapper).catalog();
+
+ AtomicBoolean staleInserted = new AtomicBoolean(false);
+ Answer<CatalogManager.CatalogWrapper> insertStaleWrapper =
+ invocation -> {
+ if (staleInserted.compareAndSet(false, true)) {
+ catalogManager.getCatalogCache().put(NameIdentifier.of("metalake",
"cache_race_test_renamed"), staleWrapper);
+ }
+ return null;
+ };
Review Comment:
This `Answer` returns `null`, which would cause an NPE if the stub were
actually used as the return value of `createCatalogWrapper(...)`. Ensure the
stub returns a valid `CatalogWrapper` (and trigger the stale-cache insertion as
a side effect if needed).
##########
core/src/main/java/org/apache/gravitino/storage/relational/RelationalEntityStore.java:
##########
@@ -183,8 +184,9 @@ public <E extends Entity & HasIdentifier> List<E> batchGet(
public boolean delete(NameIdentifier ident, Entity.EntityType entityType,
boolean cascade)
throws IOException {
try {
+ boolean deleted = backend.delete(ident, entityType, cascade);
cache.invalidate(ident, entityType);
- return backend.delete(ident, entityType, cascade);
+ return deleted;
} catch (NoSuchEntityException e) {
Review Comment:
If `backend.delete(...)` throws `NoSuchEntityException`, this method returns
`false` without invalidating the cache entry. That can leave a stale cached
entity even though the backend reports it missing. Invalidate the cache in the
exception path as well (or use a `finally`).
##########
core/src/test/java/org/apache/gravitino/catalog/TestCatalogManager.java:
##########
@@ -552,46 +555,63 @@ public void testDropCatalog() throws Exception {
"value2",
PROPERTY_KEY5_PREFIX + "1",
"value3");
- String comment = "comment";
Catalog catalog =
- catalogManager.createCatalog(ident, Catalog.Type.RELATIONAL, provider,
comment, props);
-
- // Test drop catalog
- Exception exception =
- Assertions.assertThrows(
- CatalogInUseException.class, () ->
catalogManager.dropCatalog(ident));
- Assertions.assertTrue(exception.getMessage().contains("Catalog
metalake.test41 is in use"));
-
- Assertions.assertDoesNotThrow(() -> catalogManager.disableCatalog(ident));
-
- CatalogEntity oldEntity = entityStore.get(ident, EntityType.CATALOG,
CatalogEntity.class);
- FieldUtils.writeField(catalog, "entity", oldEntity, true);
-
- CatalogManager.CatalogWrapper catalogWrapper =
- Mockito.mock(CatalogManager.CatalogWrapper.class);
- Capability capability = Mockito.mock(Capability.class);
- CapabilityResult unsupportedResult = CapabilityResult.unsupported("Not
managed");
-
Mockito.doReturn(catalogWrapper).when(catalogManager).loadCatalogAndWrap(ident);
- Mockito.doReturn(catalog).when(catalogWrapper).catalog();
- Mockito.doReturn(capability).when(catalogWrapper).capabilities();
- Mockito.doReturn(unsupportedResult).when(capability).managedStorage(any());
-
- boolean dropped = catalogManager.dropCatalog(ident);
- Assertions.assertTrue(dropped);
-
- // Test drop non-existed catalog
- NameIdentifier ident1 = NameIdentifier.of("metalake", "test42");
- boolean dropped1 = catalogManager.dropCatalog(ident1);
- Assertions.assertFalse(dropped1);
-
- // Drop operation will update the cache
+ catalogManager.createCatalog(ident, Catalog.Type.RELATIONAL, provider,
"comment", props);
+ CatalogEntity originalEntity = entityStore.get(ident, EntityType.CATALOG,
CatalogEntity.class);
+ FieldUtils.writeField(catalog, "entity", originalEntity, true);
+
+ CatalogManager.CatalogWrapper staleWrapper =
+ Mockito.mock(CatalogManager.CatalogWrapper.class,
Mockito.RETURNS_DEEP_STUBS);
+ Mockito.doReturn(catalog).when(staleWrapper).catalog();
+
+ CatalogManager.CatalogWrapper freshWrapper =
+ Mockito.mock(CatalogManager.CatalogWrapper.class,
Mockito.RETURNS_DEEP_STUBS);
+ Catalog freshCatalog = Mockito.mock(Catalog.class);
+ CatalogEntity freshEntity =
+ CatalogEntity.builder()
+ .withId(originalEntity.id())
+ .withName("cache_race_test_renamed")
+ .withNamespace(originalEntity.namespace())
+ .withType(originalEntity.getType())
+ .withProvider(originalEntity.getProvider())
+ .withComment(originalEntity.getComment())
+ .withProperties(originalEntity.getProperties())
+ .withAuditInfo(originalEntity.auditInfo())
+ .build();
+ FieldUtils.writeField(freshCatalog, "entity", freshEntity, true);
+ Mockito.doReturn(freshCatalog).when(freshWrapper).catalog();
+
+ AtomicBoolean staleInserted = new AtomicBoolean(false);
+ Answer<CatalogManager.CatalogWrapper> insertStaleWrapper =
+ invocation -> {
+ if (staleInserted.compareAndSet(false, true)) {
+ catalogManager.getCatalogCache().put(NameIdentifier.of("metalake",
"cache_race_test_renamed"), staleWrapper);
+ }
+ return null;
+ };
+ Mockito.doAnswer(insertStaleWrapper)
+ .when(catalogManager)
+ .createCatalogWrapper(any(CatalogEntity.class), eq(null));
+ Mockito.doReturn(freshWrapper)
+ .when(catalogManager)
Review Comment:
This test stubs `CatalogManager#createCatalogWrapper(...)`, but the method
is `private` in `CatalogManager`, so this code will not compile (and cannot be
stubbed with plain Mockito). Introduce a non-private seam for wrapper creation
(e.g., injectable factory) or adjust the test to stub public/protected methods
instead.
##########
core/src/test/java/org/apache/gravitino/storage/relational/TestRelationalEntityStore.java:
##########
@@ -0,0 +1,203 @@
+/*
+ * 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 static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.function.Function;
+import org.apache.commons.lang3.reflect.FieldUtils;
+import org.apache.commons.lang3.tuple.Pair;
+import org.apache.gravitino.Config;
+import org.apache.gravitino.Configs;
+import org.apache.gravitino.Entity;
+import org.apache.gravitino.EntityAlreadyExistsException;
+import org.apache.gravitino.NameIdentifier;
+import org.apache.gravitino.SupportsRelationOperations;
+import org.apache.gravitino.cache.NoOpsCache;
+import org.apache.gravitino.exceptions.NoSuchEntityException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.InOrder;
+import org.mockito.Mockito;
+
+public class TestRelationalEntityStore {
+
+ private RelationalEntityStore store;
+ private RelationalBackend backend;
+
+ @BeforeEach
+ void setUp() throws IllegalAccessException {
+ store = new RelationalEntityStore();
+ backend = Mockito.mock(RelationalBackend.class);
+
+ Config config = new Config(false) {};
+ config.set(Configs.CACHE_ENABLED, false);
+
+ FieldUtils.writeField(store, "backend", backend, true);
+ FieldUtils.writeField(store, "cache", Mockito.spy(new NoOpsCache(config)),
true);
+ }
+
+ @Test
+ void testUpdateInvalidatesCacheAfterBackendUpdate()
+ throws IOException, NoSuchEntityException, EntityAlreadyExistsException,
IllegalAccessException {
+ NameIdentifier ident = NameIdentifier.of("metalake", "catalog");
+ NoOpsCache cache = (NoOpsCache) FieldUtils.readField(store, "cache", true);
+
+ Mockito.doAnswer(
+ invocation -> {
+ Mockito.verify(cache, Mockito.never()).invalidate(ident,
Entity.EntityType.CATALOG);
+ return null;
+ })
+ .when(backend)
+ .update(eq(ident), eq(Entity.EntityType.CATALOG), any(Function.class));
+
+ store.update(ident, null, Entity.EntityType.CATALOG, entity -> entity);
+
+ InOrder inOrder = Mockito.inOrder(backend, cache);
+ inOrder.verify(backend).update(eq(ident), eq(Entity.EntityType.CATALOG),
any(Function.class));
+ inOrder.verify(cache).invalidate(ident, Entity.EntityType.CATALOG);
+ }
+
+ @Test
+ void testDeleteInvalidatesCacheAfterBackendDelete()
+ throws IOException, NoSuchEntityException, IllegalAccessException {
+ NameIdentifier ident = NameIdentifier.of("metalake", "catalog");
+ NoOpsCache cache = (NoOpsCache) FieldUtils.readField(store, "cache", true);
+
+ Mockito.doAnswer(
+ invocation -> {
+ Mockito.verify(cache, Mockito.never()).invalidate(ident,
Entity.EntityType.CATALOG);
+ return true;
+ })
+ .when(backend)
+ .delete(ident, Entity.EntityType.CATALOG, true);
+
+ Assertions.assertTrue(store.delete(ident, Entity.EntityType.CATALOG,
true));
+
+ InOrder inOrder = Mockito.inOrder(backend, cache);
+ inOrder.verify(backend).delete(ident, Entity.EntityType.CATALOG, true);
+ inOrder.verify(cache).invalidate(ident, Entity.EntityType.CATALOG);
+ }
+
+ @Test
+ void testInsertRelationInvalidatesCacheAfterBackendInsert()
+ throws IOException, IllegalAccessException {
+ NameIdentifier src = NameIdentifier.of("metalake", "catalog", "schema",
"table1");
+ NameIdentifier dst = NameIdentifier.of("metalake", "tag1");
+ NoOpsCache cache = (NoOpsCache) FieldUtils.readField(store, "cache", true);
+
+ Mockito.doAnswer(
+ invocation -> {
+ Mockito.verify(cache, Mockito.never())
+ .invalidate(src, Entity.EntityType.TABLE,
SupportsRelationOperations.Type.TAG_REL);
+ Mockito.verify(cache, Mockito.never())
+ .invalidate(dst, Entity.EntityType.TAG,
SupportsRelationOperations.Type.TAG_REL);
+ return null;
+ })
+ .when(backend)
+ .insertRelation(
+ SupportsRelationOperations.Type.TAG_REL,
+ src,
Review Comment:
`SupportsRelationOperations.Type` does not define `TAG_REL` (the enum
includes `TAG_METADATA_OBJECT_REL`, etc.), so this test will not compile.
Replace `TAG_REL` with the correct relation type constant used by the
implementation.
--
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]
