Skip to content
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
import static org.apache.gravitino.catalog.kafka.KafkaCatalogPropertiesMetadata.BOOTSTRAP_SERVERS;
import static org.apache.gravitino.catalog.kafka.KafkaTopicPropertiesMetadata.PARTITION_COUNT;
import static org.apache.gravitino.catalog.kafka.KafkaTopicPropertiesMetadata.REPLICATION_FACTOR;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.when;

import com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -74,18 +73,16 @@
import org.apache.gravitino.messaging.Topic;
import org.apache.gravitino.messaging.TopicChange;
import org.apache.gravitino.meta.AuditInfo;
import org.apache.gravitino.meta.BaseMetalake;
import org.apache.gravitino.meta.CatalogEntity;
import org.apache.gravitino.meta.SchemaVersion;
import org.apache.gravitino.storage.IdGenerator;
import org.apache.gravitino.storage.RandomIdGenerator;
import org.apache.gravitino.storage.relational.helper.CatalogIds;
import org.apache.gravitino.storage.relational.service.CatalogMetaService;
import org.apache.gravitino.storage.relational.service.MetalakeMetaService;
import org.apache.kafka.common.config.TopicConfig;
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.MockedStatic;
import org.mockito.Mockito;

public class TestKafkaCatalogOperations extends KafkaClusterEmbedded {
Expand Down Expand Up @@ -141,7 +138,7 @@ public PropertiesMetadata modelVersionPropertiesMetadata()
private static KafkaCatalogOperations kafkaCatalogOperations;

@BeforeAll
public static void setUp() throws IllegalAccessException {
public static void setUp() throws IOException, IllegalAccessException {
Config config = Mockito.mock(Config.class);
Mockito.when(config.get(STORE_TRANSACTION_MAX_SKEW_TIME)).thenReturn(1000L);
Mockito.when(config.get(STORE_DELETE_AFTER_TIME)).thenReturn(20 * 60 * 1000L);
Expand Down Expand Up @@ -178,35 +175,23 @@ public static void setUp() throws IllegalAccessException {
Mockito.when(config.get(Configs.CACHE_IMPLEMENTATION)).thenReturn("caffeine");
Mockito.when(config.get(Configs.CACHE_LOCK_SEGMENTS)).thenReturn(16);

// Mock
MetalakeMetaService metalakeMetaService = MetalakeMetaService.getInstance();
MetalakeMetaService spyMetaservice = Mockito.spy(metalakeMetaService);
doReturn(1L).when(spyMetaservice).getMetalakeIdByName(Mockito.anyString());

CatalogMetaService catalogMetaService = CatalogMetaService.getInstance();
CatalogMetaService spyCatalogMetaService = Mockito.spy(catalogMetaService);
doReturn(1L)
.when(spyCatalogMetaService)
.getCatalogIdByMetalakeIdAndName(Mockito.anyLong(), Mockito.anyString());
doReturn(new CatalogIds(1L, 1L))
.when(spyCatalogMetaService)
.getCatalogIdByMetalakeAndCatalogName(Mockito.anyString(), Mockito.anyString());

MockedStatic<MetalakeMetaService> metalakeMetaServiceMockedStatic =
Mockito.mockStatic(MetalakeMetaService.class);
MockedStatic<CatalogMetaService> catalogMetaServiceMockedStatic =
Mockito.mockStatic(CatalogMetaService.class);

metalakeMetaServiceMockedStatic
.when(MetalakeMetaService::getInstance)
.thenReturn(spyMetaservice);
catalogMetaServiceMockedStatic
.when(CatalogMetaService::getInstance)
.thenReturn(spyCatalogMetaService);

store = EntityStoreFactory.createEntityStore(config);
store.initialize(config);
idGenerator = new RandomIdGenerator();

BaseMetalake metalake =
BaseMetalake.builder()
.withId(1L)
.withName(METALAKE_NAME)
.withVersion(SchemaVersion.V_0_1)
.withAuditInfo(
AuditInfo.builder()
.withCreator("testKafkaUser")
.withCreateTime(Instant.now())
.build())
.build();
store.put(metalake, false);

kafkaCatalogEntity =
CatalogEntity.builder()
.withId(1L)
Expand All @@ -221,6 +206,7 @@ public static void setUp() throws IllegalAccessException {
.withCreateTime(Instant.now())
.build())
.build();
store.put(kafkaCatalogEntity, false);

FieldUtils.writeField(GravitinoEnv.getInstance(), "config", config, true);

Expand All @@ -238,11 +224,11 @@ public static void tearDown() throws IOException {
}

@Test
public void testKafkaCatalogConfiguration() {
public void testKafkaCatalogConfiguration() throws IOException {
String catalogName = "test_kafka_catalog_configuration";
CatalogEntity catalogEntity =
CatalogEntity.builder()
.withId(2L)
.withId(idGenerator.nextId())
.withName(catalogName)
.withNamespace(Namespace.of(METALAKE_NAME))
.withType(MESSAGING)
Expand All @@ -254,6 +240,7 @@ public void testKafkaCatalogConfiguration() {
.build())
.withProperties(MOCK_CATALOG_PROPERTIES)
.build();
store.put(catalogEntity, false);
KafkaCatalogOperations ops = new KafkaCatalogOperations(store, idGenerator);
Assertions.assertNull(ops.adminClientConfig);

Expand All @@ -274,11 +261,11 @@ public void testKafkaCatalogConfiguration() {
}

@Test
public void testInitialization() {
public void testInitialization() throws IOException {
String catalogName = "test_kafka_catalog_initialization";
CatalogEntity catalogEntity =
CatalogEntity.builder()
.withId(2L)
.withId(idGenerator.nextId())
.withName(catalogName)
.withNamespace(Namespace.of(METALAKE_NAME))
.withType(MESSAGING)
Expand All @@ -290,6 +277,7 @@ public void testInitialization() {
.build())
.withProperties(MOCK_CATALOG_PROPERTIES)
.build();
store.put(catalogEntity, false);
KafkaCatalogOperations ops = new KafkaCatalogOperations(store, idGenerator);
ops.initialize(
MOCK_CATALOG_PROPERTIES, catalogEntity.toCatalogInfo(), KAFKA_PROPERTIES_METADATA);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -984,6 +984,12 @@ public boolean dropCatalog(NameIdentifier ident, boolean force)

} catch (NoSuchMetalakeException | NoSuchCatalogException ignored) {
return false;
} catch (NoSuchEntityException ignored) {
// Another server may have deleted the catalog after it was loaded but before this
// transaction reached the compare-and-set delete. Preserve the idempotent drop
// contract and discard the now-stale local cache entry.
catalogCache.invalidate(ident);
return false;
} catch (GravitinoRuntimeException e) {
throw e;
} catch (Exception e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.List;
import java.util.Map;
import org.apache.gravitino.Entity;
import org.apache.gravitino.EntityAlreadyExistsException;
import org.apache.gravitino.EntityStore;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.Namespace;
Expand Down Expand Up @@ -116,9 +117,11 @@ public Schema createSchema(NameIdentifier ident, String comment, Map<String, Str
.build())
.build();
try {
store().put(schemaEntity, true /* overwrite */);
store().put(schemaEntity, false /* overwrite */);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of changing this to false?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interface is an explicit create operation, and we need to change it to false to avoid concurrent insertion of data into the database.

} catch (IOException ioe) {
throw new RuntimeException("Failed to create schema " + ident, ioe);
} catch (EntityAlreadyExistsException e) {
throw new SchemaAlreadyExistsException(e, "Schema %s already exists", ident);
} catch (NoSuchEntityException e) {
throw new NoSuchCatalogException(e, "Catalog %s does not exist", ident.namespace());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,9 @@ public boolean dropMetalake(NameIdentifier ident, boolean force)
}

return store.delete(ident, EntityType.METALAKE, true);
} catch (NoSuchMetalakeException e) {
} catch (NoSuchMetalakeException | NoSuchEntityException e) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CatalogManager.dropCatalog's equivalent new catch block also invalidates catalogCache before returning false (to discard the now-stale cache entry). This branch doesn't do the same for whatever metalake-level cache exists — is that intentional (metalakes aren't cached the same way) or a gap?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, catalogCache is not an entity cache. It caches CatalogWrapper objects, which hold the live catalog instance and its class loader. A metalake has nothing like this, and MetalakeManager has no cache field
at all. The "preload all metalakes" comment in the constructor talks about the entity cache in the store, not a cache in the manager.

Second, the entity cache in the store is already invalidated for us. RelationalEntityStore.delete() calls cache.invalidate(ident, entityType) in a finally block, so it runs both when the delete succeeds and when
the entity is already gone. So there is no stale entry left for this branch to clean up.

Also, in this method the NoSuchMetalakeException / NoSuchEntityException mostly comes from metalakeInUse() and store.list(), which run before we reach the delete.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional, not a gap. There are two reasons.

First, catalogCache is not an entity cache. It caches CatalogWrapper
objects, which hold the live catalog instance and its class loader. A
metalake has nothing like this, and MetalakeManager has no cache field
at all. The "preload all metalakes" comment in the constructor talks
about the entity cache in the store, not a cache in the manager.

Second, the entity cache in the store is already invalidated for us.
RelationalEntityStore.delete() calls cache.invalidate(ident, entityType)
in a finally block, so it runs both when the delete succeeds and when
the entity is already gone. So there is no stale entry left for this
branch to clean up.

Also, in this method the NoSuchMetalakeException / NoSuchEntityException
mostly comes from metalakeInUse() and store.list(), which run before
we reach the delete.

// Another server may have completed the drop after the initial existence check.
// Dropping an already-removed metalake remains an idempotent false result.
return false;

} catch (IOException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ public interface CatalogMetaMapper {
@SelectProvider(type = CatalogMetaSQLProviderFactory.class, method = "listCatalogPOsByMetalakeId")
List<CatalogPO> listCatalogPOsByMetalakeId(@Param("metalakeId") Long metalakeId);

/** Selects and locks all active catalogs in a metalake for the current transaction. */
@SelectProvider(
type = CatalogMetaSQLProviderFactory.class,
method = "listCatalogPOsByMetalakeIdForUpdate")
List<CatalogPO> listCatalogPOsByMetalakeIdForUpdate(@Param("metalakeId") Long metalakeId);

@SelectProvider(type = CatalogMetaSQLProviderFactory.class, method = "listCatalogPOsByCatalogIds")
List<CatalogPO> listCatalogPOsByCatalogIds(@Param("catalogIds") List<Long> catalogIds);

Expand All @@ -73,6 +79,18 @@ CatalogPO selectCatalogMetaByName(
@SelectProvider(type = CatalogMetaSQLProviderFactory.class, method = "selectCatalogMetaById")
CatalogPO selectCatalogMetaById(@Param("catalogId") Long catalogId);

/** Selects and locks an active catalog by ID for the current transaction. */
@SelectProvider(
type = CatalogMetaSQLProviderFactory.class,
method = "selectCatalogMetaByIdForUpdate")
CatalogPO selectCatalogMetaByIdForUpdate(@Param("catalogId") Long catalogId);

/** Selects and share-locks an active catalog by ID for the current transaction. */
@SelectProvider(
type = CatalogMetaSQLProviderFactory.class,
method = "selectCatalogMetaByIdForShare")
CatalogPO selectCatalogMetaByIdForShare(@Param("catalogId") Long catalogId);

@InsertProvider(type = CatalogMetaSQLProviderFactory.class, method = "insertCatalogMeta")
void insertCatalogMeta(@Param("catalogMeta") CatalogPO catalogPO);

Expand All @@ -89,12 +107,18 @@ Integer updateCatalogMeta(
@UpdateProvider(
type = CatalogMetaSQLProviderFactory.class,
method = "softDeleteCatalogMetasByCatalogId")
Integer softDeleteCatalogMetasByCatalogId(@Param("catalogId") Long catalogId);

Integer softDeleteCatalogMetasByCatalogId(
@Param("catalogId") Long catalogId, @Param("currentVersion") Long currentVersion);

/**
* Soft-deletes catalogs whose identifiers and OCC versions still match.
*
* @return the number of deleted rows
*/
@UpdateProvider(
type = CatalogMetaSQLProviderFactory.class,
method = "softDeleteCatalogMetasByMetalakeId")
Integer softDeleteCatalogMetasByMetalakeId(@Param("metalakeId") Long metalakeId);
method = "softDeleteCatalogMetasWithVersion")
Integer softDeleteCatalogMetasWithVersion(@Param("catalogMetas") List<CatalogPO> catalogPOs);

@DeleteProvider(
type = CatalogMetaSQLProviderFactory.class,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,13 @@ public static CatalogMetaBaseSQLProvider getProvider() {

static class CatalogMetaMySQLProvider extends CatalogMetaBaseSQLProvider {}

static class CatalogMetaH2Provider extends CatalogMetaBaseSQLProvider {}
static class CatalogMetaH2Provider extends CatalogMetaBaseSQLProvider {
@Override
public String selectCatalogMetaByIdForShare(Long catalogId) {
// H2 has no shared row-lock syntax, so use an exclusive lock in tests.
return selectCatalogMetaByIdForUpdate(catalogId);
}
}

public static String listCatalogPOsByMetalakeName(@Param("metalakeName") String metalakeName) {
return getProvider().listCatalogPOsByMetalakeName(metalakeName);
Expand All @@ -61,6 +67,11 @@ public static String listCatalogPOsByMetalakeId(@Param("metalakeId") Long metala
return getProvider().listCatalogPOsByMetalakeId(metalakeId);
}

/** Returns SQL that lists and locks all active catalogs in a metalake. */
public static String listCatalogPOsByMetalakeIdForUpdate(@Param("metalakeId") Long metalakeId) {
return getProvider().listCatalogPOsByMetalakeIdForUpdate(metalakeId);
}

public static String listCatalogPOsByCatalogIds(@Param("catalogIds") List<Long> catalogIds) {
return getProvider().listCatalogPOsByCatalogIds(catalogIds);
}
Expand Down Expand Up @@ -94,6 +105,16 @@ public static String selectCatalogMetaById(@Param("catalogId") Long catalogId) {
return getProvider().selectCatalogMetaById(catalogId);
}

/** Returns SQL that selects and locks an active catalog by ID. */
public static String selectCatalogMetaByIdForUpdate(@Param("catalogId") Long catalogId) {
return getProvider().selectCatalogMetaByIdForUpdate(catalogId);
}

/** Returns SQL that selects and share-locks an active catalog by ID. */
public static String selectCatalogMetaByIdForShare(@Param("catalogId") Long catalogId) {
return getProvider().selectCatalogMetaByIdForShare(catalogId);
}

public static String insertCatalogMeta(@Param("catalogMeta") CatalogPO catalogPO) {
return getProvider().insertCatalogMeta(catalogPO);
}
Expand All @@ -109,12 +130,15 @@ public static String updateCatalogMeta(
return getProvider().updateCatalogMeta(newCatalogPO, oldCatalogPO);
}

public static String softDeleteCatalogMetasByCatalogId(@Param("catalogId") Long catalogId) {
return getProvider().softDeleteCatalogMetasByCatalogId(catalogId);
public static String softDeleteCatalogMetasByCatalogId(
@Param("catalogId") Long catalogId, @Param("currentVersion") Long currentVersion) {
return getProvider().softDeleteCatalogMetasByCatalogId(catalogId, currentVersion);
}

public static String softDeleteCatalogMetasByMetalakeId(@Param("metalakeId") Long metalakeId) {
return getProvider().softDeleteCatalogMetasByMetalakeId(metalakeId);
/** Returns SQL that soft-deletes catalogs using identifier-and-version pairs. */
public static String softDeleteCatalogMetasWithVersion(
@Param("catalogMetas") List<CatalogPO> catalogPOs) {
return getProvider().softDeleteCatalogMetasWithVersion(catalogPOs);
}

public static String deleteCatalogMetasByLegacyTimeline(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,18 @@ public interface MetalakeMetaMapper {
@SelectProvider(type = MetalakeMetaSQLProviderFactory.class, method = "selectMetalakeMetaById")
MetalakePO selectMetalakeMetaById(@Param("metalakeId") Long metalakeId);

/** Selects and locks an active metalake by ID for the current transaction. */
@SelectProvider(
type = MetalakeMetaSQLProviderFactory.class,
method = "selectMetalakeMetaByIdForUpdate")
MetalakePO selectMetalakeMetaByIdForUpdate(@Param("metalakeId") Long metalakeId);

/** Selects and share-locks an active metalake by ID for the current transaction. */
@SelectProvider(
type = MetalakeMetaSQLProviderFactory.class,
method = "selectMetalakeMetaByIdForShare")
MetalakePO selectMetalakeMetaByIdForShare(@Param("metalakeId") Long metalakeId);

@SelectProvider(
type = MetalakeMetaSQLProviderFactory.class,
method = "listMetalakePOsByMetalakeIds")
Expand All @@ -73,7 +85,8 @@ Integer updateMetalakeMeta(
@UpdateProvider(
type = MetalakeMetaSQLProviderFactory.class,
method = "softDeleteMetalakeMetaByMetalakeId")
Integer softDeleteMetalakeMetaByMetalakeId(@Param("metalakeId") Long metalakeId);
Integer softDeleteMetalakeMetaByMetalakeId(
@Param("metalakeId") Long metalakeId, @Param("currentVersion") Long currentVersion);

@DeleteProvider(
type = MetalakeMetaSQLProviderFactory.class,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,13 @@ public static MetalakeMetaBaseSQLProvider getProvider() {

static class MetalakeMetaMySQLProvider extends MetalakeMetaBaseSQLProvider {}

static class MetalakeMetaH2Provider extends MetalakeMetaBaseSQLProvider {}
static class MetalakeMetaH2Provider extends MetalakeMetaBaseSQLProvider {
@Override
public String selectMetalakeMetaByIdForShare(Long metalakeId) {
// H2 has no shared row-lock syntax, so use an exclusive lock in tests.
return selectMetalakeMetaByIdForUpdate(metalakeId);
}
}

public String listMetalakePOs() {
return getProvider().listMetalakePOs();
Expand All @@ -65,6 +71,16 @@ public static String selectMetalakeMetaById(@Param("metalakeId") Long metalakeId
return getProvider().selectMetalakeMetaById(metalakeId);
}

/** Returns SQL that selects and locks an active metalake by ID. */
public static String selectMetalakeMetaByIdForUpdate(@Param("metalakeId") Long metalakeId) {
return getProvider().selectMetalakeMetaByIdForUpdate(metalakeId);
}

/** Returns SQL that selects and share-locks an active metalake by ID. */
public static String selectMetalakeMetaByIdForShare(@Param("metalakeId") Long metalakeId) {
return getProvider().selectMetalakeMetaByIdForShare(metalakeId);
}

public static String selectMetalakeIdMetaByName(@Param("metalakeName") String metalakeName) {
return getProvider().selectMetalakeIdMetaByName(metalakeName);
}
Expand All @@ -88,8 +104,9 @@ public static String updateMetalakeMeta(
return getProvider().updateMetalakeMeta(newMetalakePO, oldMetalakePO);
}

public static String softDeleteMetalakeMetaByMetalakeId(@Param("metalakeId") Long metalakeId) {
return getProvider().softDeleteMetalakeMetaByMetalakeId(metalakeId);
public static String softDeleteMetalakeMetaByMetalakeId(
@Param("metalakeId") Long metalakeId, @Param("currentVersion") Long currentVersion) {
return getProvider().softDeleteMetalakeMetaByMetalakeId(metalakeId, currentVersion);
}

public static String deleteMetalakeMetasByLegacyTimeline(
Expand Down
Loading
Loading