diff --git a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/SchemaValidatorUtil.java b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/SchemaValidatorUtil.java index 422997a89..82ea7edfc 100644 --- a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/SchemaValidatorUtil.java +++ b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/SchemaValidatorUtil.java @@ -119,15 +119,8 @@ public boolean columnExists(@Nonnull String tableName, @Nonnull String columnNam if (sharedCache != null) { return sharedCache.columnExists(tableName, columnName); } - String lowerTable = tableName.toLowerCase(); - String lowerColumn = columnName.toLowerCase(); - - Set columns = columnCache.get(lowerTable, tbl -> { - log.info("Refreshing column cache for table '{}'", tbl); - return loadColumns(tbl); - }); - - return columns.contains(lowerColumn); + return getCachingNonEmpty(columnCache, tableName.toLowerCase(), "column", this::loadColumns) + .contains(columnName.toLowerCase()); } /** @@ -141,11 +134,7 @@ public Set getColumns(@Nonnull String tableName) { if (sharedCache != null) { return sharedCache.getColumns(tableName); } - String lowerTable = tableName.toLowerCase(); - return columnCache.get(lowerTable, tbl -> { - log.info("Refreshing column cache for table '{}'", tbl); - return loadColumns(tbl); - }); + return getCachingNonEmpty(columnCache, tableName.toLowerCase(), "column", this::loadColumns); } /** @@ -159,15 +148,31 @@ public boolean indexExists(@Nonnull String tableName, @Nonnull String indexName) if (sharedCache != null) { return sharedCache.indexExists(tableName, indexName); } - String lowerTable = tableName.toLowerCase(); - String lowerIndex = indexName.toLowerCase(); - - Set indexes = indexCache.get(lowerTable, tbl -> { - log.info("Refreshing index cache for table '{}'", tbl); - return loadIndexes(tbl); - }); + return getCachingNonEmpty(indexCache, tableName.toLowerCase(), "index", this::loadIndexes) + .contains(indexName.toLowerCase()); + } - return indexes.contains(lowerIndex); + /** + * Returns the cached set for {@code lowerTable}, loading it on a cache miss. A non-empty result is + * cached; an empty result is NOT cached. For columns and indexes an empty result means the table + * is not present yet (e.g. schema not migrated when this ran), so pinning it for the TTL would + * make {@code columnExists()}/{@code indexExists()} report false for entries that do exist. Not + * caching it lets a later call re-resolve. Mirrors the fix in {@link SharedSchemaCache} for the + * same regression (#618). + */ + @Nonnull + private Set getCachingNonEmpty(@Nonnull Cache> cache, @Nonnull String lowerTable, + @Nonnull String what, @Nonnull java.util.function.Function> loader) { + Set cached = cache.getIfPresent(lowerTable); + if (cached != null) { + return cached; + } + log.info("Refreshing {} cache for table '{}'", what, lowerTable); + Set loaded = loader.apply(lowerTable); + if (!loaded.isEmpty()) { + cache.put(lowerTable, loaded); + } + return loaded; } diff --git a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/SharedSchemaCache.java b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/SharedSchemaCache.java index dc5fb6d21..9f34623e4 100644 --- a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/SharedSchemaCache.java +++ b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/SharedSchemaCache.java @@ -15,6 +15,7 @@ import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ThreadLocalRandom; import java.util.concurrent.TimeUnit; +import java.util.function.Function; import javax.annotation.Nonnull; import javax.annotation.Nullable; import lombok.extern.slf4j.Slf4j; @@ -98,25 +99,45 @@ public void registerAndPreWarm(@Nonnull String tableName) { // ── cache access ───────────────────────────────────────────────────────────── public boolean columnExists(@Nonnull String tableName, @Nonnull String columnName) { - return columnCache.get(tableName.toLowerCase(), tbl -> { - log.info("Inline schema cache miss: loading columns for table '{}'", tbl); - return loadColumns(tbl); - }).contains(columnName.toLowerCase()); + return getCachingNonEmpty(columnCache, tableName.toLowerCase(), "columns", this::loadColumns) + .contains(columnName.toLowerCase()); } @Nonnull public Set getColumns(@Nonnull String tableName) { - return columnCache.get(tableName.toLowerCase(), tbl -> { - log.info("Inline schema cache miss: loading columns for table '{}'", tbl); - return loadColumns(tbl); - }); + return getCachingNonEmpty(columnCache, tableName.toLowerCase(), "columns", this::loadColumns); } public boolean indexExists(@Nonnull String tableName, @Nonnull String indexName) { - return indexCache.get(tableName.toLowerCase(), tbl -> { - log.info("Inline schema cache miss: loading indexes for table '{}'", tbl); - return loadIndexes(tbl); - }).contains(indexName.toLowerCase()); + return getCachingNonEmpty(indexCache, tableName.toLowerCase(), "indexes", this::loadIndexes) + .contains(indexName.toLowerCase()); + } + + /** + * Returns the cached metadata for {@code lowerTable}, loading it on a cache miss. A non-empty + * result is cached; an empty result is deliberately NOT cached. For columns and indexes + * an empty result means the table does not exist yet — e.g. the pre-warm in the + * {@link com.linkedin.metadata.dao.EbeanLocalAccess} constructor ran before schema migration + * created the table. Caching that empty set would pin a stale "table has nothing" view for the + * full {@value #CACHE_TTL_MINUTES}-minute TTL, so every later {@code columnExists()} returns false + * and {@code EbeanLocalAccess.batchGetUnion} drops the aspect from its query — surfacing as 404s + * on aspect reads, wrong autofill defaults, lost writes and wrong row counts (regression #618). + * Not caching the empty result lets a later call re-resolve once the schema is in place; for a + * genuinely absent table the extra information_schema query is cheap and self-correcting. + */ + @Nonnull + private static Set getCachingNonEmpty(@Nonnull Cache> cache, + @Nonnull String lowerTable, @Nonnull String what, @Nonnull Function> loader) { + Set cached = cache.getIfPresent(lowerTable); + if (cached != null) { + return cached; + } + log.info("Inline schema cache miss: loading {} for table '{}'", what, lowerTable); + Set loaded = loader.apply(lowerTable); + if (!loaded.isEmpty()) { + cache.put(lowerTable, loaded); + } + return loaded; } @Nullable @@ -159,8 +180,12 @@ private void refreshAll() { private void refreshTable(String tableName) { try { - columnCache.put(tableName, loadColumns(tableName)); - indexCache.put(tableName, loadIndexes(tableName)); + // Only cache non-empty column/index metadata: an empty result means the table does not exist + // yet (e.g. pre-warm runs before schema migration), and pinning that stale view for the TTL is + // the #618 regression. Index expressions may be legitimately empty (a table with no expression + // indexes), so they are cached as-is. + putIfNonEmpty(columnCache, tableName, loadColumns(tableName)); + putIfNonEmpty(indexCache, tableName, loadIndexes(tableName)); indexExpressionCache.put(tableName, loadIndexesAndExpressions(tableName)); log.info("Schema cache refreshed for table '{}'", tableName); } catch (Exception e) { @@ -168,6 +193,16 @@ private void refreshTable(String tableName) { } } + private static void putIfNonEmpty(@Nonnull Cache> cache, @Nonnull String tableName, + @Nonnull Set value) { + if (value.isEmpty()) { + // Drop any previously-cached empty/absent entry so the next access re-resolves. + cache.invalidate(tableName); + } else { + cache.put(tableName, value); + } + } + // ── DB loaders ──────────────────────────────────────────────────────────────── private Set loadColumns(String tableName) { diff --git a/dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/utils/SharedSchemaCacheTest.java b/dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/utils/SharedSchemaCacheTest.java index 0dd7b3dc0..bdd8c6caa 100644 --- a/dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/utils/SharedSchemaCacheTest.java +++ b/dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/utils/SharedSchemaCacheTest.java @@ -125,6 +125,64 @@ public void testSingletonPerUrl() { assertNotSame(cache, different); } + /** + * Regression test for #618 (datahub-gma 0.6.185): the SharedSchemaCache pre-warm was moved into + * the EbeanLocalAccess constructor so it always runs at startup. For consumers that run schema + * migrations as a separate job (LinkedIn's prod pattern) or in integration tests, the + * constructor — and therefore the pre-warm — can execute BEFORE the entity table is migrated + * into existence. loadColumns() then returns an empty set, and caching that empty set pinned a + * stale "table has no columns" view for the full TTL: every later columnExists() returned false, + * so EbeanLocalAccess.batchGetUnion dropped the aspect from its query and reads silently returned + * nothing (404 / wrong autofill defaults / lost writes / wrong counts). + * + *

An empty column load means "table not present yet" and must NOT be cached, so a later access + * re-resolves once the schema is in place.

+ */ + @Test + public void testPreWarmBeforeTableCreatedDoesNotPoisonColumnCache() { + final String table = "metadata_entity_prewarm_regression"; + server.execute(Ebean.createSqlUpdate("DROP TABLE IF EXISTS " + table)); + + // Pre-warm while the table does NOT yet exist (mimics constructor pre-warm before migration). + cache.registerAndPreWarm(table); + assertFalse(cache.columnExists(table, "a_aspectfoo")); + + // Schema migration runs AFTER construction: the table + aspect column now exist. + server.execute(Ebean.createSqlUpdate( + "CREATE TABLE " + table + " (urn VARCHAR(100) NOT NULL, a_aspectfoo JSON, " + + "CONSTRAINT pk_" + table + " PRIMARY KEY (urn))")); + + // The column added after pre-warm must now be visible — the empty pre-warm result must not be + // pinned. Before the fix this returned false because the empty set was cached for the TTL. + assertTrue("column added after pre-warm must be visible; empty pre-warm load must not be cached (regression #618)", + cache.columnExists(table, "a_aspectfoo")); + + server.execute(Ebean.createSqlUpdate("DROP TABLE IF EXISTS " + table)); + } + + /** + * Same #618 regression as {@link #testPreWarmBeforeTableCreatedDoesNotPoisonColumnCache} but via + * the lazy (cache-miss) read path rather than registerAndPreWarm: a columnExists() call issued + * before the table exists must not pin an empty result. + */ + @Test + public void testLazyReadBeforeTableCreatedDoesNotPoisonColumnCache() { + final String table = "metadata_entity_lazy_regression"; + server.execute(Ebean.createSqlUpdate("DROP TABLE IF EXISTS " + table)); + + // Lazy read while the table does NOT yet exist. + assertFalse(cache.columnExists(table, "a_aspectfoo")); + + server.execute(Ebean.createSqlUpdate( + "CREATE TABLE " + table + " (urn VARCHAR(100) NOT NULL, a_aspectfoo JSON, " + + "CONSTRAINT pk_" + table + " PRIMARY KEY (urn))")); + + assertTrue("column visible after table creation; prior empty read must not be cached (regression #618)", + cache.columnExists(table, "a_aspectfoo")); + + server.execute(Ebean.createSqlUpdate("DROP TABLE IF EXISTS " + table)); + } + /** * MariaDB does not support the EXPRESSION column in information_schema.STATISTICS, so we mock * the DB response to exercise the getIndexExpression happy path.