From fd1b9ec5405207ba1aab79baa5f16d5372e5f2e4 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Tue, 7 Jul 2026 19:44:28 -0400 Subject: [PATCH 01/14] Change deleteStore to check StoreHeader before bumping MetaDataVersionStamp In particular the catalog uses the MetaDataVersion-based store cache, and when you delete a schema (as part of a test cleanup), the old deleteStore would invalidate the metadataversion cache, which would cause any catalog operations (which opened the store) to conflict. By being more restrictive, deleting stores that aren't caching the header means that we won't force a refresh on stores that are using the store header cache. Closes #4335 --- .../provider/foundationdb/FDBRecordStore.java | 38 ++++-- .../FDBRecordStoreStateCacheTest.java | 126 ++++++++++++++++++ 2 files changed, 155 insertions(+), 9 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java index 2a135af001..8ea85bef91 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java @@ -1799,12 +1799,16 @@ public static void deleteStore(FDBRecordContext context, KeySpacePath path) { * the store existed. * *

- * This method does not read the underlying record store, so it does not validate - * that a record store exists in the given subspace. As it might be the case that - * this record store has a cacheable store state (see {@link #setStateCacheability(boolean)}), - * this method resets the database's - * {@linkplain FDBRecordContext#getMetaDataVersionStamp(IsolationLevel) meta-data version-stamp}. - * As a result, calling this method may cause other clients to invalidate their caches needlessly. + * This method reads only the record store's header key (see {@link #STORE_INFO_KEY}) in + * order to decide whether it needs to invalidate cached state. If a header is present and + * marks the store as {@linkplain #setStateCacheability(boolean) cacheable}, the database's + * {@linkplain FDBRecordContext#getMetaDataVersionStamp(IsolationLevel) meta-data + * version-stamp} is reset so that other clients drop their cached copies; if the header is + * missing (store doesn't exist) or marks the store as non-cacheable, the version-stamp is + * not touched. This means callers who only ever operate on non-cacheable stores do not + * contend on the single meta-data version-stamp key. The header read uses snapshot + * isolation and does not add a read conflict, so it does not narrow the write-conflict + * behavior of the clear that follows. *

* * @param context the transactional context in which to delete the record store @@ -1812,9 +1816,25 @@ public static void deleteStore(FDBRecordContext context, KeySpacePath path) { */ @SuppressWarnings("PMD.CloseResource") public static void deleteStore(FDBRecordContext context, Subspace subspace) { - // In theory, we only need to set the meta-data version stamp if the record store's - // meta-data is cacheable, but we can't know that from here. - context.setMetaDataVersionStamp(); + // Read the store header at snapshot isolation so we don't add a needless read conflict: + // the clear below already creates a write conflict on the whole range, which is what + // actually serialises us against any concurrent writer of this store's header. + final byte[] headerKey = subspace.pack(STORE_INFO_KEY); + final byte[] headerBytes = context.asyncToSync(FDBStoreTimer.Waits.WAIT_LOAD_RECORD_STORE_STATE, + context.readTransaction(true).get(headerKey)); + if (headerBytes != null) { + boolean shouldBump = true; + try { + shouldBump = RecordMetaDataProto.DataStoreInfo.parseFrom(headerBytes).getCacheable(); + } catch (InvalidProtocolBufferException e) { + // If we can't parse the header, fall back to the pre-change conservative + // behavior and bump. This preserves the "delete a store I know nothing about" + // contract this method advertises. + } + if (shouldBump) { + context.setMetaDataVersionStamp(); + } + } context.setDirtyStoreState(true); context.clear(subspace.range()); } diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java index 2e866a2a47..76b1e13599 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java @@ -45,6 +45,7 @@ import com.apple.foundationdb.record.provider.foundationdb.RecordStoreStaleMetaDataVersionException; import com.apple.foundationdb.record.provider.foundationdb.keyspace.KeySpacePath; import com.apple.foundationdb.record.test.FakeClusterFileUtil; +import com.apple.foundationdb.subspace.Subspace; import com.apple.foundationdb.tuple.ByteArrayUtil; import com.apple.foundationdb.tuple.ByteArrayUtil2; import com.apple.foundationdb.tuple.Tuple; @@ -766,6 +767,131 @@ public void storeDeletionAcrossContexts(@Nonnull StateCacheTestContext testConte } } + /** + * Deleting a non-cacheable store must NOT bump the meta-data version stamp: no other + * client can possibly hold a cached copy of a non-cacheable header, so the version stamp + * (a JVM-wide bottleneck on the {@code \xff/metadataVersion} key) shouldn't be touched. + * This is the low-level property that lets parallel tests share the SYS catalog without + * conflicting on catalog teardown. + */ + @Test + void deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp() throws Exception { + // Bootstrap: ensure the meta-data version stamp key has a value before the test runs, + // so we can distinguish "unchanged" from "was never set". The store below is opened + // with cacheability disabled (which is the default from setStateCacheability(false)). + try (FDBRecordContext context = fdb.openContext()) { + if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { + context.setMetaDataVersionStamp(); + } + commit(context); + } + + Subspace subspace; + try (FDBRecordContext context = openContext()) { + openSimpleRecordStore(context); + assertFalse(recordStore.getRecordStoreState().getStoreHeader().getCacheable(), + "test presumes the store is non-cacheable by default"); + subspace = recordStore.getSubspace(); + commit(context); + } + + // Snapshot the version stamp before deletion — reading via SNAPSHOT so this txn doesn't + // conflict with the delete-txn below and skew the result. + final byte[] beforeStamp; + try (FDBRecordContext context = fdb.openContext()) { + beforeStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); + } + assertNotNull(beforeStamp, "bootstrap should have populated the meta-data version stamp"); + + try (FDBRecordContext context = openContext()) { + FDBRecordStore.deleteStore(context, subspace); + commit(context); + } + + try (FDBRecordContext context = fdb.openContext()) { + final byte[] afterStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); + assertArrayEquals(beforeStamp, afterStamp, + "deleting a non-cacheable store should not have bumped the meta-data version stamp"); + } + } + + /** + * Complement of {@link #deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp()}: deleting + * a cacheable store MUST bump the stamp — otherwise sibling clients could keep serving + * reads out of a stale cached header long after the store is gone. + */ + @Test + void deleteCacheableStoreBumpsMetaDataVersionStamp() throws Exception { + try (FDBRecordContext context = fdb.openContext()) { + if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { + context.setMetaDataVersionStamp(); + } + commit(context); + } + + Subspace subspace; + try (FDBRecordContext context = openContext()) { + openSimpleRecordStore(context); + assertTrue(recordStore.setStateCacheability(true), "flipping to cacheable should have changed something"); + subspace = recordStore.getSubspace(); + commit(context); + } + // Commit above already bumped the stamp (transition to cacheable). Snapshot after that. + final byte[] beforeStamp; + try (FDBRecordContext context = fdb.openContext()) { + beforeStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); + } + assertNotNull(beforeStamp); + + try (FDBRecordContext context = openContext()) { + FDBRecordStore.deleteStore(context, subspace); + commit(context); + } + + try (FDBRecordContext context = fdb.openContext()) { + final byte[] afterStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); + assertNotNull(afterStamp); + assertFalse(java.util.Arrays.equals(beforeStamp, afterStamp), + "deleting a cacheable store should have bumped the meta-data version stamp"); + } + } + + /** + * Deleting an empty subspace (no store header present) must not bump the stamp either — + * there's no cached header to invalidate. + */ + @Test + void deleteMissingStoreDoesNotBumpMetaDataVersionStamp() throws Exception { + try (FDBRecordContext context = fdb.openContext()) { + if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { + context.setMetaDataVersionStamp(); + } + commit(context); + } + + // Use the test's per-instance path, but never open a store there. + final Subspace subspace; + try (FDBRecordContext context = openContext()) { + subspace = path.toSubspace(context); + } + final byte[] beforeStamp; + try (FDBRecordContext context = fdb.openContext()) { + beforeStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); + } + assertNotNull(beforeStamp); + + try (FDBRecordContext context = openContext()) { + FDBRecordStore.deleteStore(context, subspace); + commit(context); + } + + try (FDBRecordContext context = fdb.openContext()) { + final byte[] afterStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); + assertArrayEquals(beforeStamp, afterStamp, + "deleting an empty subspace (no header) should not have bumped the meta-data version stamp"); + } + } + /** * Verify that updating a header user field will be updated if the store state is cached. */ From fb747b2ed9f52a52edac60e1b321559102fba27a Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Mon, 13 Jul 2026 16:36:21 -0400 Subject: [PATCH 02/14] deleteStore: guard against concurrent setStateCacheability flip deleteStore reads STORE_INFO_KEY at snapshot isolation to avoid adding a read conflict on every delete. This leaves a race with a concurrent commit that flips the store to cacheable via setStateCacheability(true) and bumps the meta-data version stamp: our snapshot read misses the flip, we skip our own bump, and the delete lands. Sibling clients that populated their caches from the flipper committed state then keep serving reads out of a header for a store that no longer exists. Fix: only on the branches where we decide NOT to bump (header absent, or header parses as non-cacheable), add an explicit point read conflict on STORE_INFO_KEY. That way: - the common case (delete of a store that was and stays non-cacheable) still contributes zero read conflicts, and - the racy case (concurrent flip to cacheable) is detected: the writer SET on STORE_INFO_KEY overlaps our added read conflict and we lose the commit race. The cacheable-header branch and the invalid-header fallback continue to bump unconditionally without a read conflict -- bumping is safe regardless of what a concurrent writer does. Also updates the javadoc to describe the new semantics. Adds a regression test (concurrentSetCacheabilityAndDeleteDoesNotLoseTheBump) that fails against the pre-fix implementation: it runs the two racing transactions with pinned read versions, forces the writer to commit first, and asserts that if the delete then commits the stamp must have advanced past the writer post-commit value. Closes #4335 --- .../provider/foundationdb/FDBRecordStore.java | 66 +++++-- .../FDBRecordStoreStateCacheTest.java | 166 ++++++++++++++++++ 2 files changed, 219 insertions(+), 13 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java index 8ea85bef91..64b77c140d 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java @@ -1806,9 +1806,19 @@ public static void deleteStore(FDBRecordContext context, KeySpacePath path) { * version-stamp} is reset so that other clients drop their cached copies; if the header is * missing (store doesn't exist) or marks the store as non-cacheable, the version-stamp is * not touched. This means callers who only ever operate on non-cacheable stores do not - * contend on the single meta-data version-stamp key. The header read uses snapshot - * isolation and does not add a read conflict, so it does not narrow the write-conflict - * behavior of the clear that follows. + * contend on the single meta-data version-stamp key. + *

+ * + *

+ * The header read uses snapshot isolation to avoid adding a read-conflict range on every + * delete. Only when the on-disk header says the store is non-cacheable (or is missing) — + * i.e., the case in which we would skip the bump — do we add an explicit point read + * conflict on {@link #STORE_INFO_KEY}. That closes the race with a concurrent + * {@link #setStateCacheability(boolean)} that flips the store to cacheable: if such a + * writer commits before us, our commit fails with a serialisation error and the caller + * retries with a fresh snapshot that sees the new header and bumps the stamp. In the + * common case (delete of a store that was and stays non-cacheable), no read conflict is + * added at all. *

* * @param context the transactional context in which to delete the record store @@ -1816,25 +1826,55 @@ public static void deleteStore(FDBRecordContext context, KeySpacePath path) { */ @SuppressWarnings("PMD.CloseResource") public static void deleteStore(FDBRecordContext context, Subspace subspace) { - // Read the store header at snapshot isolation so we don't add a needless read conflict: - // the clear below already creates a write conflict on the whole range, which is what - // actually serialises us against any concurrent writer of this store's header. + // Read the store header at snapshot isolation so we don't add a read conflict on + // every delete. The clear below already creates a write conflict on the whole range, + // which serialises us against any concurrent writer that lands INSIDE this subspace. + // The single race the snapshot read leaves open is a concurrent commit that writes + // STORE_INFO_KEY (e.g. a setStateCacheability that flips cacheable=true and bumps + // the meta-data version stamp). If that writer commits first, our snapshot read + // misses the flip and we would skip our own bump — leaving sibling caches populated + // from the writer's committed state stale after our delete. + // + // We close that race narrowly: only on the branches where we decide NOT to bump + // (header absent, or header parses as non-cacheable), we add an explicit point read + // conflict on STORE_INFO_KEY. That way: + // - the common case (delete of a store that was and stays non-cacheable) still + // contributes zero read conflicts, and + // - the racy case (concurrent flip to cacheable) is detected: the writer's SET on + // STORE_INFO_KEY overlaps our added read conflict and we lose the commit race. final byte[] headerKey = subspace.pack(STORE_INFO_KEY); final byte[] headerBytes = context.asyncToSync(FDBStoreTimer.Waits.WAIT_LOAD_RECORD_STORE_STATE, context.readTransaction(true).get(headerKey)); - if (headerBytes != null) { - boolean shouldBump = true; + boolean shouldBump; + if (headerBytes == null) { + // Header absent: no cached state exists to invalidate — no bump needed. But guard + // against a concurrent creator/enabler flipping the store to cacheable underneath + // us: adding a read conflict on STORE_INFO_KEY makes such a writer's commit + // exclude ours. + context.ensureActive().addReadConflictKey(headerKey); + shouldBump = false; + } else { + boolean cacheable; try { - shouldBump = RecordMetaDataProto.DataStoreInfo.parseFrom(headerBytes).getCacheable(); + cacheable = RecordMetaDataProto.DataStoreInfo.parseFrom(headerBytes).getCacheable(); } catch (InvalidProtocolBufferException e) { // If we can't parse the header, fall back to the pre-change conservative - // behavior and bump. This preserves the "delete a store I know nothing about" - // contract this method advertises. + // behaviour and bump. Also skip the read conflict — bumping unconditionally + // is safe regardless of what a concurrent writer does. + cacheable = true; } - if (shouldBump) { - context.setMetaDataVersionStamp(); + if (cacheable) { + shouldBump = true; + } else { + // Header says non-cacheable AND parsed cleanly. If a concurrent writer flips + // it to cacheable, we must lose the commit race so the caller retries. + context.ensureActive().addReadConflictKey(headerKey); + shouldBump = false; } } + if (shouldBump) { + context.setMetaDataVersionStamp(); + } context.setDirtyStoreState(true); context.clear(subspace.range()); } diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java index 76b1e13599..80f041b080 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java @@ -76,6 +76,7 @@ import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; /** * Tests to make sure that caching {@link FDBRecordStoreStateCacheEntry} objects work. @@ -892,6 +893,123 @@ void deleteMissingStoreDoesNotBumpMetaDataVersionStamp() throws Exception { } } + /** + * Race check for {@link FDBRecordStore#deleteStore(FDBRecordContext, Subspace)}. Three + * transactions in this sequence — {@code T_flipper} and {@code T_deleter} race, then + * {@code T_writer} exercises the observable inconsistency: + * + *
    + *
  1. {@code T_deleter} pins its read version before any flip has committed.
    2. + *
  2. {@code T_flipper} (a separate transaction) flips the store to cacheable and + * commits.
    3. + *
  3. A warm-up transaction opens the store so the JVM-wide state cache is + * populated with a {@code (cacheable=true)} entry.
    4. + *
  4. {@code T_deleter} calls {@link FDBRecordStore#deleteStore} and commits. Its + * snapshot-isolation header read at the pinned pre-flip version sees + * {@code cacheable=false} and (in the buggy code) skips both the bump and any + * read conflict on {@code STORE_INFO_KEY}.
    5. + *
  5. {@code T_writer} — a separate transaction — opens the store, hits + * the (stale) cache entry, and saves a record. Because the writer never reads + * the disk header itself and only trusts the cache, it commits happily.
    6. + *
+ * + *

Acceptable outcomes:

+ *
    + *
  1. {@code T_deleter} conflicts with the flipper's {@code STORE_INFO_KEY} write + * and does not land. The store still exists on disk; the cache is consistent + * with reality.
    2. + *
  2. {@code T_deleter} commits but the meta-data version stamp advances (it also + * bumped) so the writer's cache lookup misses and reloads the fresh, absent + * header — reporting "no store" and refusing to save the orphan record.
    3. + *
+ * + *

The bug we're guarding against: {@code T_deleter} commits without bumping the + * stamp, the writer's cache lookup hits the stale entry, and the writer inserts a record + * into a subspace whose store header has been deleted. On-disk end state: no header, but + * records present — an orphan set of rows nothing can safely read again.

+ */ + @Test + void concurrentSetCacheabilityAndDeleteDoesNotLoseTheBump() throws Exception { + fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); + ensureMetaDataVersionStampInitialized(); + + // Setup: create the store as NON-cacheable. That is the trap for the deleter's + // snapshot header read. + FDBRecordStore.Builder storeBuilder; + Subspace subspace; + try (FDBRecordContext context = openContext()) { + openSimpleRecordStore(context); + assertFalse(recordStore.getRecordStoreState().getStoreHeader().getCacheable(), + "test presumes the store is non-cacheable by default"); + storeBuilder = recordStore.asBuilder(); + subspace = recordStore.getSubspace(); + commit(context); + } + + final boolean deleterCommitted; + try (FDBRecordContext deleterContext = fdb.openContext(null, new FDBStoreTimer())) { + // Pin the deleter's read version BEFORE the flip commits, so the deleter's + // snapshot header read sees the pre-flip (non-cacheable) state. + deleterContext.getReadVersion(); + + // Separate transaction: flip the store to cacheable. Kept separate from the + // record insert below so that the record-inserting transaction has no reason to + // read the header itself — it will pick up the cacheable header via the state + // cache. + try (FDBRecordContext flipperContext = fdb.openContext(null, new FDBStoreTimer())) { + FDBRecordStore flipperStore = storeBuilder.copyBuilder().setContext(flipperContext).open(); + assertTrue(flipperStore.setStateCacheability(true), + "flipping to cacheable should have changed the header"); + commit(flipperContext); + } + + // Warm the JVM-wide state cache: a fresh open goes through the cache's + // get() → cache-miss → load-fresh → addToCache(cacheable=true). Without this + // the writer below would miss the cache and read STORE_INFO_KEY directly (which + // would see the deleter's clear and correctly treat the store as absent), + // masking the bug. + try (FDBRecordContext warmup = fdb.openContext(null, new FDBStoreTimer())) { + storeBuilder.copyBuilder().setContext(warmup).open(); + } + + // T_deleter: reads STORE_INFO_KEY at snapshot at its pinned pre-flip RV, sees + // non-cacheable. In the fixed code, this branch also adds a point read conflict + // on STORE_INFO_KEY — which the flipper's committed write will conflict with. + FDBRecordStore.deleteStore(deleterContext, subspace); + deleterCommitted = tryCommitOrDetectConflict(deleterContext); + } + + if (deleterCommitted) { + // Bug reproduction: a separate transaction opens the store via the cache and + // saves a record. The writer never reads STORE_INFO_KEY itself — the cache + // entry (populated during warmup) still validates because the deleter did not + // bump the stamp. The record write commits successfully, producing an on-disk + // orphan: no store header, but records present. + try (FDBRecordContext writerContext = fdb.openContext(null, new FDBStoreTimer())) { + FDBRecordStore writerStore = storeBuilder.copyBuilder().setContext(writerContext).open(); + writerStore.saveRecord(TestRecords1Proto.MySimpleRecord.newBuilder() + .setRecNo(1) + .setStrValueIndexed("orphaned-writer") + .build()); + commit(writerContext); + } + // At this point without the fix: STORE_INFO_KEY is absent AND the record is + // present. Fail loudly and describe the failure mode — this is the exact + // "no header but not empty" orphan the fix is meant to prevent. With the fix, + // the deleter would have conflicted, we would not be here at all. + fail("delete committed after a concurrent setStateCacheability(true) flip " + + "and did not bump the meta-data version stamp; a subsequent writer " + + "trusted its (now-stale) cache entry and inserted a record into a " + + "subspace whose store header has been deleted — an on-disk orphan. " + + "This is the correctness bug deleteStore's read-conflict guard is meant " + + "to prevent."); + } else { + // Deleter correctly conflicted with the flipper. The store still exists (the + // flipper's cacheable header); caches remain consistent with disk. + assertStoreHeaderPresent(subspace); + } + } + /** * Verify that updating a header user field will be updated if the store state is cached. */ @@ -1380,4 +1498,52 @@ private void assertNotCacheable() { private boolean isStoreCachable() { return recordStore.getRecordStoreState().getStoreHeader().getCacheable(); } + + // ---- helpers for the concurrent-deleteStore race test ------------------------------- + + /** + * Bootstrap that guarantees the JVM-wide meta-data version stamp key exists, so callers + * can compare before/after snapshots without special-casing null. + */ + private void ensureMetaDataVersionStampInitialized() throws Exception { + try (FDBRecordContext context = fdb.openContext()) { + if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { + context.setMetaDataVersionStamp(); + } + commit(context); + } + } + + /** + * Attempt to commit the given context. Returns {@code true} on success, {@code false} if + * the commit failed with a conflict (any nested cause). Any other exception is re-raised + * so an unexpected failure mode doesn't silently masquerade as "conflict". + */ + private boolean tryCommitOrDetectConflict(@Nonnull FDBRecordContext context) throws Exception { + try { + commit(context); + return true; + } catch (Exception ex) { + for (Throwable cause = ex; cause != null; cause = cause.getCause()) { + if (cause instanceof FDBExceptions.FDBStoreTransactionConflictException) { + return false; + } + } + throw new AssertionError("unexpected exception from commit: " + ex, ex); + } + } + + /** + * Assert the store header is still present at the given subspace (used when the deleter + * loses the commit race and the store should still exist on disk). + */ + private void assertStoreHeaderPresent(@Nonnull Subspace subspace) throws Exception { + try (FDBRecordContext peek = fdb.openContext()) { + final byte[] header = peek.readTransaction(true) + .get(subspace.pack(FDBRecordStoreKeyspace.STORE_INFO.key())) + .join(); + assertNotNull(header, + "when the deleter conflicts, the store header should still be present on disk"); + } + } } From 756a08ddb595869b5bf222019b32cf06d2cfe617 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Tue, 14 Jul 2026 13:13:11 -0400 Subject: [PATCH 03/14] Simplify tests and inverted version of conflicting one A bunch of DRY and removing unnecessary logic. Also have a version of the conflict test with the commits in the other order --- .../FDBRecordStoreStateCacheTest.java | 276 ++++++++---------- 1 file changed, 116 insertions(+), 160 deletions(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java index 80f041b080..8a0629099c 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java @@ -25,8 +25,8 @@ import com.apple.foundationdb.record.RecordCoreException; import com.apple.foundationdb.record.RecordMetaData; import com.apple.foundationdb.record.RecordMetaDataBuilder; -import com.apple.foundationdb.record.expressions.RecordKeyExpressionProto; import com.apple.foundationdb.record.TestRecords1Proto; +import com.apple.foundationdb.record.expressions.RecordKeyExpressionProto; import com.apple.foundationdb.record.metadata.Key; import com.apple.foundationdb.record.metadata.expressions.KeyExpression; import com.apple.foundationdb.record.provider.foundationdb.FDBDatabase; @@ -57,6 +57,7 @@ import org.junit.jupiter.params.provider.MethodSource; import javax.annotation.Nonnull; +import javax.annotation.Nullable; import java.util.Arrays; import java.util.Collections; import java.util.UUID; @@ -772,48 +773,49 @@ public void storeDeletionAcrossContexts(@Nonnull StateCacheTestContext testConte * Deleting a non-cacheable store must NOT bump the meta-data version stamp: no other * client can possibly hold a cached copy of a non-cacheable header, so the version stamp * (a JVM-wide bottleneck on the {@code \xff/metadataVersion} key) shouldn't be touched. - * This is the low-level property that lets parallel tests share the SYS catalog without - * conflicting on catalog teardown. + * This means that deleting a store won't cause any interaction with any cacheable store to conflict. */ @Test void deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp() throws Exception { - // Bootstrap: ensure the meta-data version stamp key has a value before the test runs, - // so we can distinguish "unchanged" from "was never set". The store below is opened - // with cacheability disabled (which is the default from setStateCacheability(false)). - try (FDBRecordContext context = fdb.openContext()) { - if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { - context.setMetaDataVersionStamp(); - } - commit(context); - } + ensureMetaDataVersionStampInitialized(); Subspace subspace; try (FDBRecordContext context = openContext()) { openSimpleRecordStore(context); - assertFalse(recordStore.getRecordStoreState().getStoreHeader().getCacheable(), - "test presumes the store is non-cacheable by default"); + assertNotCacheable(); subspace = recordStore.getSubspace(); commit(context); } - // Snapshot the version stamp before deletion — reading via SNAPSHOT so this txn doesn't - // conflict with the delete-txn below and skew the result. - final byte[] beforeStamp; - try (FDBRecordContext context = fdb.openContext()) { - beforeStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); - } - assertNotNull(beforeStamp, "bootstrap should have populated the meta-data version stamp"); + deleteStoreDoesNotBumpMetaDataVersion(subspace); + } + + /** + * Deleting an empty subspace (no store header present) must not bump the stamp either — + * there's no cached header to invalidate. + */ + @Test + void deleteMissingStoreDoesNotBumpMetaDataVersionStamp() { + ensureMetaDataVersionStampInitialized(); + // Use the test's per-instance path, but never open a store there. + final Subspace subspace; try (FDBRecordContext context = openContext()) { - FDBRecordStore.deleteStore(context, subspace); - commit(context); + subspace = path.toSubspace(context); } + deleteStoreDoesNotBumpMetaDataVersion(subspace); + } - try (FDBRecordContext context = fdb.openContext()) { - final byte[] afterStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); - assertArrayEquals(beforeStamp, afterStamp, - "deleting a non-cacheable store should not have bumped the meta-data version stamp"); - } + private void deleteStoreDoesNotBumpMetaDataVersion(final Subspace subspace) { + final byte[] beforeStamp = getMetaDataVersionStamp(); + assertNotNull(beforeStamp); + + deleteStore(subspace); + + final byte[] afterStamp = getMetaDataVersionStamp(); + assertNotNull(afterStamp); + assertArrayEquals(beforeStamp, afterStamp, + "deleting a cacheable store should have bumped the meta-data version stamp"); } /** @@ -823,12 +825,7 @@ void deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp() throws Exception { */ @Test void deleteCacheableStoreBumpsMetaDataVersionStamp() throws Exception { - try (FDBRecordContext context = fdb.openContext()) { - if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { - context.setMetaDataVersionStamp(); - } - commit(context); - } + ensureMetaDataVersionStampInitialized(); Subspace subspace; try (FDBRecordContext context = openContext()) { @@ -838,98 +835,27 @@ void deleteCacheableStoreBumpsMetaDataVersionStamp() throws Exception { commit(context); } // Commit above already bumped the stamp (transition to cacheable). Snapshot after that. - final byte[] beforeStamp; - try (FDBRecordContext context = fdb.openContext()) { - beforeStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); - } + final byte[] beforeStamp = getMetaDataVersionStamp(); assertNotNull(beforeStamp); - try (FDBRecordContext context = openContext()) { - FDBRecordStore.deleteStore(context, subspace); - commit(context); - } + deleteStore(subspace); try (FDBRecordContext context = fdb.openContext()) { final byte[] afterStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); assertNotNull(afterStamp); - assertFalse(java.util.Arrays.equals(beforeStamp, afterStamp), + assertFalse(Arrays.equals(beforeStamp, afterStamp), "deleting a cacheable store should have bumped the meta-data version stamp"); } } /** - * Deleting an empty subspace (no store header present) must not bump the stamp either — - * there's no cached header to invalidate. - */ - @Test - void deleteMissingStoreDoesNotBumpMetaDataVersionStamp() throws Exception { - try (FDBRecordContext context = fdb.openContext()) { - if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { - context.setMetaDataVersionStamp(); - } - commit(context); - } - - // Use the test's per-instance path, but never open a store there. - final Subspace subspace; - try (FDBRecordContext context = openContext()) { - subspace = path.toSubspace(context); - } - final byte[] beforeStamp; - try (FDBRecordContext context = fdb.openContext()) { - beforeStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); - } - assertNotNull(beforeStamp); - - try (FDBRecordContext context = openContext()) { - FDBRecordStore.deleteStore(context, subspace); - commit(context); - } - - try (FDBRecordContext context = fdb.openContext()) { - final byte[] afterStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); - assertArrayEquals(beforeStamp, afterStamp, - "deleting an empty subspace (no header) should not have bumped the meta-data version stamp"); - } - } - - /** - * Race check for {@link FDBRecordStore#deleteStore(FDBRecordContext, Subspace)}. Three - * transactions in this sequence — {@code T_flipper} and {@code T_deleter} race, then - * {@code T_writer} exercises the observable inconsistency: - * - *
    - *
  1. {@code T_deleter} pins its read version before any flip has committed.
    2. - *
  2. {@code T_flipper} (a separate transaction) flips the store to cacheable and - * commits.
    3. - *
  3. A warm-up transaction opens the store so the JVM-wide state cache is - * populated with a {@code (cacheable=true)} entry.
    4. - *
  4. {@code T_deleter} calls {@link FDBRecordStore#deleteStore} and commits. Its - * snapshot-isolation header read at the pinned pre-flip version sees - * {@code cacheable=false} and (in the buggy code) skips both the bump and any - * read conflict on {@code STORE_INFO_KEY}.
    5. - *
  5. {@code T_writer} — a separate transaction — opens the store, hits - * the (stale) cache entry, and saves a record. Because the writer never reads - * the disk header itself and only trusts the cache, it commits happily.
    6. - *
- * - *

Acceptable outcomes:

- *
    - *
  1. {@code T_deleter} conflicts with the flipper's {@code STORE_INFO_KEY} write - * and does not land. The store still exists on disk; the cache is consistent - * with reality.
    2. - *
  2. {@code T_deleter} commits but the meta-data version stamp advances (it also - * bumped) so the writer's cache lookup misses and reloads the fresh, absent - * header — reporting "no store" and refusing to save the orphan record.
    3. - *
- * - *

The bug we're guarding against: {@code T_deleter} commits without bumping the - * stamp, the writer's cache lookup hits the stale entry, and the writer inserts a record - * into a subspace whose store header has been deleted. On-disk end state: no header, but - * records present — an orphan set of rows nothing can safely read again.

+ * Race check for {@link FDBRecordStore#deleteStore(FDBRecordContext, Subspace)}. + *

The bug we're guarding against: {@code deleteStore} commits without bumping the + * stamp, concurrently with another transaction that sets the store as cacheable. + * The delete should conflict.

*/ @Test - void concurrentSetCacheabilityAndDeleteDoesNotLoseTheBump() throws Exception { + void concurrentSetCacheabilityPreventsDeleteStore() throws Exception { fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); ensureMetaDataVersionStampInitialized(); @@ -939,8 +865,7 @@ void concurrentSetCacheabilityAndDeleteDoesNotLoseTheBump() throws Exception { Subspace subspace; try (FDBRecordContext context = openContext()) { openSimpleRecordStore(context); - assertFalse(recordStore.getRecordStoreState().getStoreHeader().getCacheable(), - "test presumes the store is non-cacheable by default"); + assertNotCacheable(); storeBuilder = recordStore.asBuilder(); subspace = recordStore.getSubspace(); commit(context); @@ -963,40 +888,14 @@ void concurrentSetCacheabilityAndDeleteDoesNotLoseTheBump() throws Exception { commit(flipperContext); } - // Warm the JVM-wide state cache: a fresh open goes through the cache's - // get() → cache-miss → load-fresh → addToCache(cacheable=true). Without this - // the writer below would miss the cache and read STORE_INFO_KEY directly (which - // would see the deleter's clear and correctly treat the store as absent), - // masking the bug. - try (FDBRecordContext warmup = fdb.openContext(null, new FDBStoreTimer())) { - storeBuilder.copyBuilder().setContext(warmup).open(); - } - - // T_deleter: reads STORE_INFO_KEY at snapshot at its pinned pre-flip RV, sees - // non-cacheable. In the fixed code, this branch also adds a point read conflict + // deleterContext: reads STORE_INFO_KEY at snapshot at its pinned pre-flip RV, sees + // non-cacheable. The deleteStore also adds a point read conflict // on STORE_INFO_KEY — which the flipper's committed write will conflict with. FDBRecordStore.deleteStore(deleterContext, subspace); deleterCommitted = tryCommitOrDetectConflict(deleterContext); } if (deleterCommitted) { - // Bug reproduction: a separate transaction opens the store via the cache and - // saves a record. The writer never reads STORE_INFO_KEY itself — the cache - // entry (populated during warmup) still validates because the deleter did not - // bump the stamp. The record write commits successfully, producing an on-disk - // orphan: no store header, but records present. - try (FDBRecordContext writerContext = fdb.openContext(null, new FDBStoreTimer())) { - FDBRecordStore writerStore = storeBuilder.copyBuilder().setContext(writerContext).open(); - writerStore.saveRecord(TestRecords1Proto.MySimpleRecord.newBuilder() - .setRecNo(1) - .setStrValueIndexed("orphaned-writer") - .build()); - commit(writerContext); - } - // At this point without the fix: STORE_INFO_KEY is absent AND the record is - // present. Fail loudly and describe the failure mode — this is the exact - // "no header but not empty" orphan the fix is meant to prevent. With the fix, - // the deleter would have conflicted, we would not be here at all. fail("delete committed after a concurrent setStateCacheability(true) flip " + "and did not bump the meta-data version stamp; a subsequent writer " + "trusted its (now-stale) cache entry and inserted a record into a " + @@ -1006,7 +905,64 @@ void concurrentSetCacheabilityAndDeleteDoesNotLoseTheBump() throws Exception { } else { // Deleter correctly conflicted with the flipper. The store still exists (the // flipper's cacheable header); caches remain consistent with disk. - assertStoreHeaderPresent(subspace); + try (FDBRecordContext contextUsingCache = fdb.openContext(null, new FDBStoreTimer())) { + // ensure that we can open the store, guaranteeing the store header still exists + recordStore = storeBuilder.copyBuilder().setContext(contextUsingCache).open(); + assertCacheable(); + } + } + } + + /** + * Race check for {@link FDBRecordStore#deleteStore(FDBRecordContext, Subspace)}. + *

The bug we're guarding against: {@code deleteStore} commits without bumping the + * stamp, concurrently with another transaction that sets the store as cacheable. + * The context trying to set cacheability should conflict.

+ */ + @Test + void concurrentSetCacheabilityConflictsWithDeleteStore() throws Exception { + fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); + ensureMetaDataVersionStampInitialized(); + + // Setup: create the store as NON-cacheable. That is the trap for the deleter's + // snapshot header read. + FDBRecordStore.Builder storeBuilder; + Subspace subspace; + try (FDBRecordContext context = openContext()) { + openSimpleRecordStore(context); + assertNotCacheable(); + storeBuilder = recordStore.asBuilder(); + subspace = recordStore.getSubspace(); + commit(context); + } + + final boolean flipperCommitted; + try (FDBRecordContext flipperContext = fdb.openContext(null, new FDBStoreTimer())) { + FDBRecordStore flipperStore = storeBuilder.copyBuilder().setContext(flipperContext).open(); + try (FDBRecordContext deleterContext = fdb.openContext(null, new FDBStoreTimer())) { + FDBRecordStore.deleteStore(deleterContext, subspace); + commit(deleterContext); + } + assertTrue(flipperStore.setStateCacheability(true), + "flipping to cacheable should have changed the header"); + flipperCommitted = tryCommitOrDetectConflict(flipperContext); + } + + if (flipperCommitted) { + fail("delete committed after a concurrent setStateCacheability(true) flip " + + "and did not bump the meta-data version stamp; a subsequent writer " + + "trusted its (now-stale) cache entry and inserted a record into a " + + "subspace whose store header has been deleted — an on-disk orphan. " + + "This is the correctness bug deleteStore's read-conflict guard is meant " + + "to prevent."); + } else { + // Deleter correctly conflicted with the flipper. The store still should have been deleted. + try (FDBRecordContext contextUsingCache = fdb.openContext(null, new FDBStoreTimer())) { + assertTrue(contextUsingCache.ensureActive().getRange(subspace.range()).asList().join().isEmpty(), + "The store should have been deleted"); + recordStore = storeBuilder.copyBuilder().setContext(contextUsingCache).create(); + assertNotCacheable(); + } } } @@ -1502,10 +1458,10 @@ private boolean isStoreCachable() { // ---- helpers for the concurrent-deleteStore race test ------------------------------- /** - * Bootstrap that guarantees the JVM-wide meta-data version stamp key exists, so callers + * Bootstrap that guarantees the cluster-wide meta-data version stamp key exists, so callers * can compare before/after snapshots without special-casing null. */ - private void ensureMetaDataVersionStampInitialized() throws Exception { + private void ensureMetaDataVersionStampInitialized() { try (FDBRecordContext context = fdb.openContext()) { if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { context.setMetaDataVersionStamp(); @@ -1514,6 +1470,20 @@ private void ensureMetaDataVersionStampInitialized() throws Exception { } } + @Nullable + private byte[] getMetaDataVersionStamp() { + try (FDBRecordContext context = fdb.openContext()) { + return context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); + } + } + + private void deleteStore(final Subspace subspace) { + try (FDBRecordContext context = openContext()) { + FDBRecordStore.deleteStore(context, subspace); + commit(context); + } + } + /** * Attempt to commit the given context. Returns {@code true} on success, {@code false} if * the commit failed with a conflict (any nested cause). Any other exception is re-raised @@ -1532,18 +1502,4 @@ private boolean tryCommitOrDetectConflict(@Nonnull FDBRecordContext context) thr throw new AssertionError("unexpected exception from commit: " + ex, ex); } } - - /** - * Assert the store header is still present at the given subspace (used when the deleter - * loses the commit race and the store should still exist on disk). - */ - private void assertStoreHeaderPresent(@Nonnull Subspace subspace) throws Exception { - try (FDBRecordContext peek = fdb.openContext()) { - final byte[] header = peek.readTransaction(true) - .get(subspace.pack(FDBRecordStoreKeyspace.STORE_INFO.key())) - .join(); - assertNotNull(header, - "when the deleter conflicts, the store header should still be present on disk"); - } - } } From 5dc8ed31f7080dc409411b561b81d4073e2e6613 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Tue, 14 Jul 2026 14:50:26 -0400 Subject: [PATCH 04/14] Cleanup comments / assertion messages --- .../provider/foundationdb/FDBRecordStore.java | 8 +++--- .../FDBRecordStoreStateCacheTest.java | 28 ++++++++----------- 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java index 64b77c140d..e67bc86d44 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java @@ -1815,7 +1815,7 @@ public static void deleteStore(FDBRecordContext context, KeySpacePath path) { * i.e., the case in which we would skip the bump — do we add an explicit point read * conflict on {@link #STORE_INFO_KEY}. That closes the race with a concurrent * {@link #setStateCacheability(boolean)} that flips the store to cacheable: if such a - * writer commits before us, our commit fails with a serialisation error and the caller + * writer commits before us, our commit fails with a serialization error and the caller * retries with a fresh snapshot that sees the new header and bumps the stamp. In the * common case (delete of a store that was and stays non-cacheable), no read conflict is * added at all. @@ -1828,7 +1828,7 @@ public static void deleteStore(FDBRecordContext context, KeySpacePath path) { public static void deleteStore(FDBRecordContext context, Subspace subspace) { // Read the store header at snapshot isolation so we don't add a read conflict on // every delete. The clear below already creates a write conflict on the whole range, - // which serialises us against any concurrent writer that lands INSIDE this subspace. + // which serializes us against any concurrent writer that lands INSIDE this subspace. // The single race the snapshot read leaves open is a concurrent commit that writes // STORE_INFO_KEY (e.g. a setStateCacheability that flips cacheable=true and bumps // the meta-data version stamp). If that writer commits first, our snapshot read @@ -1858,8 +1858,8 @@ public static void deleteStore(FDBRecordContext context, Subspace subspace) { try { cacheable = RecordMetaDataProto.DataStoreInfo.parseFrom(headerBytes).getCacheable(); } catch (InvalidProtocolBufferException e) { - // If we can't parse the header, fall back to the pre-change conservative - // behaviour and bump. Also skip the read conflict — bumping unconditionally + // If we can't parse the header, fall back to the conservative + // behavior and bump. Also skip the read conflict — bumping unconditionally // is safe regardless of what a concurrent writer does. cacheable = true; } diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java index 8a0629099c..eddf156ac7 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java @@ -815,7 +815,7 @@ private void deleteStoreDoesNotBumpMetaDataVersion(final Subspace subspace) { final byte[] afterStamp = getMetaDataVersionStamp(); assertNotNull(afterStamp); assertArrayEquals(beforeStamp, afterStamp, - "deleting a cacheable store should have bumped the meta-data version stamp"); + "deleting a cacheable store should not have bumped the meta-data version stamp"); } /** @@ -898,7 +898,7 @@ void concurrentSetCacheabilityPreventsDeleteStore() throws Exception { if (deleterCommitted) { fail("delete committed after a concurrent setStateCacheability(true) flip " + "and did not bump the meta-data version stamp; a subsequent writer " + - "trusted its (now-stale) cache entry and inserted a record into a " + + "could have trusted its (now-stale) cache entry and inserted a record into a " + "subspace whose store header has been deleted — an on-disk orphan. " + "This is the correctness bug deleteStore's read-conflict guard is meant " + "to prevent."); @@ -914,18 +914,19 @@ void concurrentSetCacheabilityPreventsDeleteStore() throws Exception { } /** - * Race check for {@link FDBRecordStore#deleteStore(FDBRecordContext, Subspace)}. - *

The bug we're guarding against: {@code deleteStore} commits without bumping the - * stamp, concurrently with another transaction that sets the store as cacheable. - * The context trying to set cacheability should conflict.

+ * Companion invariant to {@link #concurrentSetCacheabilityPreventsDeleteStore()}: with the + * commit order flipped (deleter first, then flipper), the flipper must conflict. Unlike its + * companion, this test relies on general read/write conflict machinery — specifically, that + * setStateCacheability's own SERIALIZABLE header read (via open() → loadRecordStoreStateAsync + * or handleCachedState) puts STORE_INFO_KEY in the flipper's read set, so any concurrent + * write to that key by a preceding delete forces a conflict. */ @Test void concurrentSetCacheabilityConflictsWithDeleteStore() throws Exception { fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); ensureMetaDataVersionStampInitialized(); - // Setup: create the store as NON-cacheable. That is the trap for the deleter's - // snapshot header read. + // Setup: create the store as NON-cacheable (matches the companion test's starting state). FDBRecordStore.Builder storeBuilder; Subspace subspace; try (FDBRecordContext context = openContext()) { @@ -949,12 +950,9 @@ void concurrentSetCacheabilityConflictsWithDeleteStore() throws Exception { } if (flipperCommitted) { - fail("delete committed after a concurrent setStateCacheability(true) flip " + - "and did not bump the meta-data version stamp; a subsequent writer " + - "trusted its (now-stale) cache entry and inserted a record into a " + - "subspace whose store header has been deleted — an on-disk orphan. " + - "This is the correctness bug deleteStore's read-conflict guard is meant " + - "to prevent."); + fail("a setStateCacheability(true) flip committed after a successful deleteStore; " + + "another transaction could have used the cached state to write after the " + + "delete store without there being a store header"); } else { // Deleter correctly conflicted with the flipper. The store still should have been deleted. try (FDBRecordContext contextUsingCache = fdb.openContext(null, new FDBStoreTimer())) { @@ -1455,8 +1453,6 @@ private boolean isStoreCachable() { return recordStore.getRecordStoreState().getStoreHeader().getCacheable(); } - // ---- helpers for the concurrent-deleteStore race test ------------------------------- - /** * Bootstrap that guarantees the cluster-wide meta-data version stamp key exists, so callers * can compare before/after snapshots without special-casing null. From 0f1eefd89ef14ff2fc1b2a85a0bbd26dbb98d57b Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Tue, 14 Jul 2026 15:07:29 -0400 Subject: [PATCH 05/14] Slightly more test DRY --- .../FDBRecordStoreStateCacheTest.java | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java index eddf156ac7..e66f47849f 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java @@ -840,12 +840,10 @@ void deleteCacheableStoreBumpsMetaDataVersionStamp() throws Exception { deleteStore(subspace); - try (FDBRecordContext context = fdb.openContext()) { - final byte[] afterStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); - assertNotNull(afterStamp); - assertFalse(Arrays.equals(beforeStamp, afterStamp), - "deleting a cacheable store should have bumped the meta-data version stamp"); - } + final byte[] afterStamp = getMetaDataVersionStamp(); + assertNotNull(afterStamp); + assertFalse(Arrays.equals(beforeStamp, afterStamp), + "deleting a cacheable store should have bumped the meta-data version stamp"); } /** @@ -1180,12 +1178,7 @@ void setCacheabilityDuringStoreOpening() { .getCache(fdb); fdb.setStoreStateCache(storeStateCache); - try (FDBRecordContext context = fdb.openContext()) { - if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { - context.setMetaDataVersionStamp(); - } - commit(context); - } + ensureMetaDataVersionStampInitialized(); // Create the store, initially not cacheable FDBStoreTimer timer = new FDBStoreTimer(); From a7d32d3ac103f98637636e5a2daa274b4b5baad9 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Wed, 15 Jul 2026 16:53:50 -0400 Subject: [PATCH 06/14] DRY more of the test --- .../FDBRecordStoreStateCacheTest.java | 243 +++++++++--------- 1 file changed, 120 insertions(+), 123 deletions(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java index e66f47849f..223eba82f6 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java @@ -221,7 +221,7 @@ public void cacheByReadVersion() throws Exception { // Open a record store but do not commit to make sure that the updated value is not cached try (FDBRecordContext context = openContext()) { openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); assertTrue(context.hasDirtyStoreState()); readVersion = context.getReadVersion(); metaDataVersion = recordStore.getRecordMetaData().getVersion(); @@ -235,7 +235,7 @@ public void cacheByReadVersion() throws Exception { openSimpleRecordStore(context); // For this specific case, we hit the cache, but then we need to validate that the store is empty // in order to match its null store header. - assertEquals(0, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 0); assertTrue(context.hasDirtyStoreState()); assertEquals(metaDataVersion, recordStore.getRecordMetaData().getVersion()); commit(context); // commit so a stable value is put into the database @@ -244,7 +244,7 @@ public void cacheByReadVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); assertFalse(context.hasDirtyStoreState()); assertEquals(metaDataVersion, recordStore.getRecordMetaData().getVersion()); readVersion = context.getReadVersion(); @@ -255,7 +255,7 @@ public void cacheByReadVersion() throws Exception { context.setReadVersion(readVersion); openSimpleRecordStore(context); assertFalse(context.hasDirtyStoreState()); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); assertEquals(metaDataVersion, recordStore.getRecordMetaData().getVersion()); } @@ -265,7 +265,7 @@ public void cacheByReadVersion() throws Exception { context.setReadVersion(readVersion); openSimpleRecordStore(context); assertFalse(context.hasDirtyStoreState()); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); recordStore.markIndexWriteOnly("MySimpleRecord$str_value_indexed").get(); assertTrue(context.hasDirtyStoreState()); assertFalse(recordStore.isIndexReadable("MySimpleRecord$str_value_indexed")); @@ -273,7 +273,7 @@ public void cacheByReadVersion() throws Exception { // Reopen the store with the same context and ensure the index is still not readable openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); assertNotSame(initialRecordStore, recordStore); assertNotSame(initialRecordStore.getRecordStoreState(), recordStore.getRecordStoreState()); assertFalse(recordStore.isIndexReadable("MySimpleRecord$str_value_indexed")); @@ -286,7 +286,7 @@ public void cacheByReadVersion() throws Exception { context.getTimer().reset(); context.setReadVersion(readVersion); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); assertTrue(recordStore.isIndexReadable("MySimpleRecord$str_value_indexed")); // Add a random write-conflict range to ensure conflicts are actually checked @@ -300,7 +300,7 @@ public void cacheByReadVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); long newReadVersion = context.getReadVersion(); assertThat(newReadVersion, greaterThan(readVersion)); readVersion = newReadVersion; @@ -311,7 +311,7 @@ public void cacheByReadVersion() throws Exception { context.getTimer().reset(); context.setReadVersion(readVersion); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); assertFalse(recordStore.isIndexReadable("MySimpleRecord$str_value_indexed")); } } @@ -335,7 +335,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); metaDataVersionStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); assertNotNull(metaDataVersionStamp); } @@ -343,7 +343,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); assertArrayEquals(metaDataVersionStamp, context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT)); recordStore.markIndexWriteOnly("MySimpleRecord$str_value_indexed").get(); assertTrue(context.hasDirtyStoreState()); @@ -355,7 +355,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); assertArrayEquals(metaDataVersionStamp, context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT)); assertTrue(recordStore.isIndexWriteOnly("MySimpleRecord$str_value_indexed")); commit(context); @@ -365,7 +365,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); assertTrue(recordStore.setStateCacheability(true)); assertTrue(context.hasDirtyStoreState()); assertArrayEquals(metaDataVersionStamp, context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT)); @@ -377,7 +377,7 @@ public void cacheByMetaDataVersion() throws Exception { context.getTimer().reset(); openSimpleRecordStore(context); assertArrayEquals(metaDataVersionStamp, context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT)); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); assertTrue(recordStore.isIndexWriteOnly("MySimpleRecord$str_value_indexed")); // don't need to commit } @@ -386,7 +386,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); assertTrue(recordStore.isIndexWriteOnly("MySimpleRecord$str_value_indexed")); recordStore.markIndexReadable("MySimpleRecord$str_value_indexed").get(); assertTrue(context.hasDirtyStoreState()); @@ -398,7 +398,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); assertTrue(recordStore.isIndexReadable("MySimpleRecord$str_value_indexed")); byte[] trMetaDataVersionStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); assertNotNull(trMetaDataVersionStamp); @@ -410,7 +410,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); assertTrue(recordStore.isIndexReadable("MySimpleRecord$str_value_indexed")); assertArrayEquals(metaDataVersionStamp, context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT)); } @@ -421,7 +421,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); readVersion = context.getReadVersion(); assertTrue(recordStore.setStateCacheability(false)); assertTrue(context.hasDirtyStoreState()); @@ -436,7 +436,7 @@ public void cacheByMetaDataVersion() throws Exception { context.getTimer().reset(); context.setReadVersion(readVersion); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); assertTrue(recordStore.getRecordStoreState().getStoreHeader().getCacheable()); } @@ -444,7 +444,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); byte[] trMetaDataVersionStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); assertNotNull(trMetaDataVersionStamp); assertThat(ByteArrayUtil.compareUnsigned(metaDataVersionStamp, trMetaDataVersionStamp), lessThan(0)); @@ -455,7 +455,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); byte[] trMetaDataVersionStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); assertNotNull(trMetaDataVersionStamp); assertArrayEquals(metaDataVersionStamp, trMetaDataVersionStamp); @@ -479,7 +479,7 @@ public void cacheByMetaDataVersionFirstTimeEver() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); assertNull(context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT)); recordStore.setStateCacheability(true); @@ -494,14 +494,14 @@ public void cacheByMetaDataVersionFirstTimeEver() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); assertArrayEquals(commitVersionStamp, context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT)); } try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); assertArrayEquals(commitVersionStamp, context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT)); } } @@ -532,7 +532,7 @@ public void conflictWhenCachedChanged(@Nonnull StateCacheTestContext testContext .setMetaDataProvider(metaData1) .setKeySpacePath(path) .create(); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); assertEquals(metaData1.getVersion(), recordStore.getRecordStoreState().getStoreHeader().getMetaDataversion()); commit(context); @@ -545,7 +545,7 @@ public void conflictWhenCachedChanged(@Nonnull StateCacheTestContext testContext .setContext(context1) .setMetaDataProvider(metaData1) .open(); - assertEquals(1, context1.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context1.getTimer(), 1); assertEquals(metaData1.getVersion(), recordStore1.getRecordMetaData().getVersion()); assertEquals(metaData1.getVersion(), recordStore1.getRecordStoreState().getStoreHeader().getMetaDataversion()); @@ -554,7 +554,7 @@ public void conflictWhenCachedChanged(@Nonnull StateCacheTestContext testContext .setContext(context2) .setMetaDataProvider(metaData2) .open(); - assertEquals(1, context2.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context2.getTimer(), 1); assertEquals(Collections.singletonList(recordStore2.getRecordMetaData().getRecordType("MySimpleRecord")), recordStore2.getRecordMetaData().recordTypesForIndex(recordStore2.getRecordMetaData().getIndex("MySimpleRecord$num_value_2"))); assertEquals(metaData2.getVersion(), recordStore2.getRecordMetaData().getVersion()); @@ -580,14 +580,14 @@ public void conflictWhenCachedChanged(@Nonnull StateCacheTestContext testContext .setContext(context) .setMetaDataProvider(metaData1) .open()); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); // Trying to load with the new meta-data should succeed FDBRecordStore recordStore = storeBuilder.copyBuilder() .setContext(context) .setMetaDataProvider(metaData2) .open(); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); assertEquals(metaData2.getVersion(), recordStore.getRecordStoreState().getStoreHeader().getMetaDataversion()); } } @@ -604,7 +604,7 @@ public void existenceCheckOnCachedStoreStates(@Nonnull StateCacheTestContext tes FDBRecordStore.Builder storeBuilder; try (FDBRecordContext context = openContext()) { openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); assertTrue(context.hasDirtyStoreState()); // Save a record so that when the store header is deleted, it won't be an empty record store recordStore.saveRecord(TestRecords1Proto.MySimpleRecord.newBuilder() @@ -620,7 +620,7 @@ public void existenceCheckOnCachedStoreStates(@Nonnull StateCacheTestContext tes assertThrows(RecordStoreAlreadyExistsException.class, storeBuilder::create); context.getTimer().reset(); FDBRecordStore store = storeBuilder.open(); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); // Delete the store header storeInfoKey = store.getSubspace().pack(FDBRecordStoreKeyspace.STORE_INFO.key()); @@ -656,24 +656,24 @@ public void storeDeletionInSameContext(@Nonnull StateCacheTestContext testContex try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder)) { openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); context.getTimer().reset(); FDBRecordStore.deleteStore(context, recordStore.getSubspace()); recordStore.asBuilder().create(); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); commit(context); } try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder)) { openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); path.deleteAllData(context); context.getTimer().reset(); recordStore.asBuilder().create(); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); } // Deleting all records should not disable the index, so the result should still be cacheable. @@ -681,20 +681,20 @@ public void storeDeletionInSameContext(@Nonnull StateCacheTestContext testContex final String disabledIndex = "MySimpleRecord$str_value_indexed"; try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); recordStore.markIndexDisabled(disabledIndex).get(); commit(context); } try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); assertTrue(recordStore.isIndexDisabled(disabledIndex)); recordStore.deleteAllRecords(); context.getTimer().reset(); recordStore = recordStore.asBuilder().open(); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); assertTrue(recordStore.isIndexDisabled(disabledIndex)); commit(context); } @@ -719,7 +719,7 @@ public void storeDeletionAcrossContexts(@Nonnull StateCacheTestContext testConte // Delete by calling deleteStore. try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); FDBRecordStore.deleteStore(context, recordStore.getSubspace()); commit(context); } @@ -727,7 +727,7 @@ public void storeDeletionAcrossContexts(@Nonnull StateCacheTestContext testConte // After deleting it, when opening the same store again, it shouldn't be cached. try (FDBRecordContext context = fdb.openContext(null, new FDBStoreTimer())) { FDBRecordStore store = storeBuilder.setContext(context).create(); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); assertTrue(store.setStateCacheability(true)); commit(context); } @@ -735,7 +735,7 @@ public void storeDeletionAcrossContexts(@Nonnull StateCacheTestContext testConte // Delete by calling path.deleteAllData try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { openSimpleRecordStore(context); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); path.deleteAllData(context); commit(context); } @@ -743,7 +743,7 @@ public void storeDeletionAcrossContexts(@Nonnull StateCacheTestContext testConte try (FDBRecordContext context = fdb.openContext(null, new FDBStoreTimer())) { FDBRecordStore store = storeBuilder.setContext(context).create(); store.setStateCacheabilityAsync(true).get(); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); commit(context); } @@ -778,16 +778,7 @@ public void storeDeletionAcrossContexts(@Nonnull StateCacheTestContext testConte @Test void deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp() throws Exception { ensureMetaDataVersionStampInitialized(); - - Subspace subspace; - try (FDBRecordContext context = openContext()) { - openSimpleRecordStore(context); - assertNotCacheable(); - subspace = recordStore.getSubspace(); - commit(context); - } - - deleteStoreDoesNotBumpMetaDataVersion(subspace); + deleteStoreDoesNotBumpMetaDataVersion(createStore(false).subspace()); } /** @@ -827,13 +818,7 @@ private void deleteStoreDoesNotBumpMetaDataVersion(final Subspace subspace) { void deleteCacheableStoreBumpsMetaDataVersionStamp() throws Exception { ensureMetaDataVersionStampInitialized(); - Subspace subspace; - try (FDBRecordContext context = openContext()) { - openSimpleRecordStore(context); - assertTrue(recordStore.setStateCacheability(true), "flipping to cacheable should have changed something"); - subspace = recordStore.getSubspace(); - commit(context); - } + final Subspace subspace = createStore(true).subspace(); // Commit above already bumped the stamp (transition to cacheable). Snapshot after that. final byte[] beforeStamp = getMetaDataVersionStamp(); assertNotNull(beforeStamp); @@ -859,15 +844,7 @@ void concurrentSetCacheabilityPreventsDeleteStore() throws Exception { // Setup: create the store as NON-cacheable. That is the trap for the deleter's // snapshot header read. - FDBRecordStore.Builder storeBuilder; - Subspace subspace; - try (FDBRecordContext context = openContext()) { - openSimpleRecordStore(context); - assertNotCacheable(); - storeBuilder = recordStore.asBuilder(); - subspace = recordStore.getSubspace(); - commit(context); - } + final StoreSetup setup = createStore(false); final boolean deleterCommitted; try (FDBRecordContext deleterContext = fdb.openContext(null, new FDBStoreTimer())) { @@ -880,7 +857,7 @@ void concurrentSetCacheabilityPreventsDeleteStore() throws Exception { // read the header itself — it will pick up the cacheable header via the state // cache. try (FDBRecordContext flipperContext = fdb.openContext(null, new FDBStoreTimer())) { - FDBRecordStore flipperStore = storeBuilder.copyBuilder().setContext(flipperContext).open(); + FDBRecordStore flipperStore = setup.builder().copyBuilder().setContext(flipperContext).open(); assertTrue(flipperStore.setStateCacheability(true), "flipping to cacheable should have changed the header"); commit(flipperContext); @@ -889,7 +866,7 @@ void concurrentSetCacheabilityPreventsDeleteStore() throws Exception { // deleterContext: reads STORE_INFO_KEY at snapshot at its pinned pre-flip RV, sees // non-cacheable. The deleteStore also adds a point read conflict // on STORE_INFO_KEY — which the flipper's committed write will conflict with. - FDBRecordStore.deleteStore(deleterContext, subspace); + FDBRecordStore.deleteStore(deleterContext, setup.subspace()); deleterCommitted = tryCommitOrDetectConflict(deleterContext); } @@ -905,7 +882,7 @@ void concurrentSetCacheabilityPreventsDeleteStore() throws Exception { // flipper's cacheable header); caches remain consistent with disk. try (FDBRecordContext contextUsingCache = fdb.openContext(null, new FDBStoreTimer())) { // ensure that we can open the store, guaranteeing the store header still exists - recordStore = storeBuilder.copyBuilder().setContext(contextUsingCache).open(); + recordStore = setup.builder().copyBuilder().setContext(contextUsingCache).open(); assertCacheable(); } } @@ -925,21 +902,13 @@ void concurrentSetCacheabilityConflictsWithDeleteStore() throws Exception { ensureMetaDataVersionStampInitialized(); // Setup: create the store as NON-cacheable (matches the companion test's starting state). - FDBRecordStore.Builder storeBuilder; - Subspace subspace; - try (FDBRecordContext context = openContext()) { - openSimpleRecordStore(context); - assertNotCacheable(); - storeBuilder = recordStore.asBuilder(); - subspace = recordStore.getSubspace(); - commit(context); - } + final StoreSetup setup = createStore(false); final boolean flipperCommitted; try (FDBRecordContext flipperContext = fdb.openContext(null, new FDBStoreTimer())) { - FDBRecordStore flipperStore = storeBuilder.copyBuilder().setContext(flipperContext).open(); + FDBRecordStore flipperStore = setup.builder().copyBuilder().setContext(flipperContext).open(); try (FDBRecordContext deleterContext = fdb.openContext(null, new FDBStoreTimer())) { - FDBRecordStore.deleteStore(deleterContext, subspace); + FDBRecordStore.deleteStore(deleterContext, setup.subspace()); commit(deleterContext); } assertTrue(flipperStore.setStateCacheability(true), @@ -954,9 +923,9 @@ void concurrentSetCacheabilityConflictsWithDeleteStore() throws Exception { } else { // Deleter correctly conflicted with the flipper. The store still should have been deleted. try (FDBRecordContext contextUsingCache = fdb.openContext(null, new FDBStoreTimer())) { - assertTrue(contextUsingCache.ensureActive().getRange(subspace.range()).asList().join().isEmpty(), + assertTrue(contextUsingCache.ensureActive().getRange(setup.subspace().range()).asList().join().isEmpty(), "The store should have been deleted"); - recordStore = storeBuilder.copyBuilder().setContext(contextUsingCache).create(); + recordStore = setup.builder().copyBuilder().setContext(contextUsingCache).create(); assertNotCacheable(); } } @@ -1037,11 +1006,11 @@ public void cacheTwoSubspaces(@Nonnull StateCacheTestContext testContext) throws long readVersion; try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder1, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { FDBRecordStore store1 = storeBuilder1.setContext(context).open(); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); assertTrue(store1.isIndexWriteOnly("MySimpleRecord$str_value_indexed")); assertTrue(store1.isIndexReadable("MySimpleRecord$num_value_3_indexed")); FDBRecordStore store2 = storeBuilder2.setContext(context).open(); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(context.getTimer(), 1); assertTrue(store2.isIndexReadable("MySimpleRecord$str_value_indexed")); assertTrue(store2.isIndexDisabled("MySimpleRecord$num_value_3_indexed")); @@ -1053,11 +1022,11 @@ public void cacheTwoSubspaces(@Nonnull StateCacheTestContext testContext) throws context.getTimer().reset(); context.setReadVersion(readVersion); FDBRecordStore store1 = storeBuilder1.setContext(context).open(); - assertEquals(1, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 1); assertTrue(store1.isIndexWriteOnly("MySimpleRecord$str_value_indexed")); assertTrue(store1.isIndexReadable("MySimpleRecord$num_value_3_indexed")); FDBRecordStore store2 = storeBuilder2.setContext(context).open(); - assertEquals(2, context.getTimer().getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(context.getTimer(), 2); assertTrue(store2.isIndexReadable("MySimpleRecord$str_value_indexed")); assertTrue(store2.isIndexDisabled("MySimpleRecord$num_value_3_indexed")); } @@ -1086,7 +1055,7 @@ public void cacheWithVersionTracking(@Nonnull StateCacheTestContext testContext) try (FDBRecordContext context = fdb.openContext(null, timer, readSemantics)) { openSimpleRecordStore(context); recordStore.setStateCacheabilityAsync(true).get(); - assertEquals(1, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(timer, 1); recordStore.markIndexDisabled("MySimpleRecord$str_value_indexed").get(); commit(context); commitVersion = context.getCommittedVersion(); @@ -1097,7 +1066,7 @@ public void cacheWithVersionTracking(@Nonnull StateCacheTestContext testContext) try (FDBRecordContext context = fdb.openContext(null, timer, readSemantics)) { assertEquals(commitVersion, context.getReadVersion()); openSimpleRecordStore(context); - assertEquals(1, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(timer, 1); assertTrue(recordStore.isIndexDisabled("MySimpleRecord$str_value_indexed")); commit(context); // should be read only-so won't change commit version } @@ -1107,7 +1076,7 @@ public void cacheWithVersionTracking(@Nonnull StateCacheTestContext testContext) try (FDBRecordContext context = fdb.openContext(null, timer, readSemantics)) { assertEquals(commitVersion, context.getReadVersion()); openSimpleRecordStore(context); - assertEquals(1, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(timer, 1); assertTrue(recordStore.isIndexDisabled("MySimpleRecord$str_value_indexed")); // Add a dummy write to increase the DB version @@ -1124,9 +1093,9 @@ public void cacheWithVersionTracking(@Nonnull StateCacheTestContext testContext) assertEquals(commitVersion, context.getReadVersion()); openSimpleRecordStore(context); if (testContext instanceof ReadVersionStateCacheTestContext) { - assertEquals(1, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(timer, 1); } else { - assertEquals(1, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(timer, 1); } assertTrue(recordStore.isIndexDisabled("MySimpleRecord$str_value_indexed")); @@ -1145,9 +1114,9 @@ public void cacheWithVersionTracking(@Nonnull StateCacheTestContext testContext) assertThat(readVersion, greaterThanOrEqualTo(commitVersion)); openSimpleRecordStore(context); if (testContext instanceof ReadVersionStateCacheTestContext) { - assertEquals(1, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + assertCacheMiss(timer, 1); } else { - assertEquals(1, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(timer, 1); } assertTrue(recordStore.isIndexDisabled("MySimpleRecord$str_value_indexed")); } @@ -1157,7 +1126,7 @@ public void cacheWithVersionTracking(@Nonnull StateCacheTestContext testContext) try (FDBRecordContext context = fdb.openContext(null, timer, readSemantics)) { assertEquals(readVersion, context.getReadVersion()); openSimpleRecordStore(context); - assertEquals(1, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + assertCacheHit(timer, 1); assertTrue(recordStore.isIndexDisabled("MySimpleRecord$str_value_indexed")); } } @@ -1189,9 +1158,7 @@ void setCacheabilityDuringStoreOpening() { metaDataVersionStamp1 = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); commit(context); } - assertEquals(1L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); - assertEquals(0L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); - timer.reset(); + assertCacheMissAndHitAndReset(timer, 1, 0); // Open the store, this time changing to make cacheable try (FDBRecordContext context = fdb.openContext(null, timer)) { @@ -1200,9 +1167,7 @@ void setCacheabilityDuringStoreOpening() { assertNotCacheable(); commit(context); } - assertEquals(1L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); - assertEquals(0L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); - timer.reset(); + assertCacheMissAndHitAndReset(timer, 1, 0); // Open the store, this time changing to make cacheable try (FDBRecordContext context = fdb.openContext(null, timer)) { @@ -1211,9 +1176,7 @@ void setCacheabilityDuringStoreOpening() { assertCacheable(); commit(context); } - assertEquals(1L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); - assertEquals(0L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); - timer.reset(); + assertCacheMissAndHitAndReset(timer, 1, 0); // Open the store, again with DEFAULT behavior. It should now be marked cacheable try (FDBRecordContext context = fdb.openContext(null, timer)) { @@ -1222,9 +1185,7 @@ void setCacheabilityDuringStoreOpening() { assertCacheable(); commit(context); } - assertEquals(1L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); - assertEquals(0L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); - timer.reset(); + assertCacheMissAndHitAndReset(timer, 1, 0); // Turn off caching during check version. The actual opening should be a hit, but the next one // should miss as it turns off state cacheability @@ -1234,9 +1195,7 @@ void setCacheabilityDuringStoreOpening() { assertNotCacheable(); commit(context); } - assertEquals(0L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); - assertEquals(1L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); - timer.reset(); + assertCacheMissAndHitAndReset(timer, 0, 1); // Opening the store again should be a cache miss byte[] metaDataVersionStamp2; @@ -1248,9 +1207,7 @@ void setCacheabilityDuringStoreOpening() { assertNotCacheable(); commit(context); } - assertEquals(1L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); - assertEquals(0L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); - timer.reset(); + assertCacheMissAndHitAndReset(timer, 1, 0); // Open again. This time, using NOT_CACHEABLE should not induce any changes (including to the meta-data versionstamp) try (FDBRecordContext context = fdb.openContext(null, timer)) { @@ -1259,9 +1216,7 @@ void setCacheabilityDuringStoreOpening() { assertNotCacheable(); commit(context); } - assertEquals(1L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); - assertEquals(0L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); - timer.reset(); + assertCacheMissAndHitAndReset(timer, 1, 0); try (FDBRecordContext context = fdb.openContext(null, timer)) { assertArrayEquals(context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT), metaDataVersionStamp2, "Meta-data version stamp should not be changed if the store state was originally not cacheable"); @@ -1294,18 +1249,14 @@ void setCacheabilityOnStoreCreation() { openSimpleStoreWithCacheabilityOnOpen(context, FDBRecordStore.StateCacheabilityOnOpen.DEFAULT); assertCacheable(); } - assertEquals(1L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); - assertEquals(0L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); - timer.reset(); + assertCacheMissAndHitAndReset(timer, 1, 0); // Open the store a third time, this time using the cache try (FDBRecordContext context = fdb.openContext(null, timer)) { openSimpleStoreWithCacheabilityOnOpen(context, FDBRecordStore.StateCacheabilityOnOpen.DEFAULT); assertCacheable(); } - assertEquals(0L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); - assertEquals(1L, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); - timer.reset(); + assertCacheMissAndHitAndReset(timer, 0, 1); } @@ -1459,6 +1410,21 @@ private void ensureMetaDataVersionStampInitialized() { } } + private static void assertCacheMissAndHitAndReset(final FDBStoreTimer timer, final int expectedMissCount, + final int expectedHitCount) { + assertCacheMiss(timer, expectedMissCount); + assertCacheHit(timer, expectedHitCount); + timer.reset(); + } + + private static void assertCacheHit(final FDBStoreTimer timer, int expected) { + assertEquals(expected, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + } + + private static void assertCacheMiss(final FDBStoreTimer timer, int expected) { + assertEquals(expected, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + } + @Nullable private byte[] getMetaDataVersionStamp() { try (FDBRecordContext context = fdb.openContext()) { @@ -1473,6 +1439,37 @@ private void deleteStore(final Subspace subspace) { } } + /** + * Small tuple returned by {@link #createStore(boolean)} for tests that need both a + * pre-configured {@link FDBRecordStore.Builder builder} (to rebind to fresh contexts) and + * the {@link Subspace subspace} the store lives in. Both are captured while the setup + * transaction is still open, so callers don't have to open another context just to + * resolve the subspace. + */ + private record StoreSetup(@Nonnull FDBRecordStore.Builder builder, @Nonnull Subspace subspace) { + } + + /** + * Create the standard simple record store, optionally flipped to cacheable, and return + * a re-usable builder plus the store's subspace. Used by the {@code deleteStore} tests + * to share the "open, configure cacheability, commit" boilerplate. + */ + private StoreSetup createStore(boolean cacheable) throws Exception { + try (FDBRecordContext context = openContext()) { + openSimpleRecordStore(context); + if (cacheable) { + assertTrue(recordStore.setStateCacheability(true), + "flipping to cacheable should have changed the header"); + } else { + assertNotCacheable(); + } + final FDBRecordStore.Builder builder = recordStore.asBuilder(); + final Subspace subspace = recordStore.getSubspace(); + commit(context); + return new StoreSetup(builder, subspace); + } + } + /** * Attempt to commit the given context. Returns {@code true} on success, {@code false} if * the commit failed with a conflict (any nested cause). Any other exception is re-raised From e6ec8b1bcaab12c7528589d2f05ae77e1e0b7439 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Fri, 24 Jul 2026 09:34:50 -0400 Subject: [PATCH 07/14] new deleteStoreAsync that has simpler conflict semantics This reverts `deleteStore`, and adds `deleteStoreAsync` with the new implementation that does not bump the MetaDataVersionStamp excessively. Additionally it changes the new implementation to just read the store header at serializable isolation, rather than trying to do the trickery where it re-adds the range conflict. --- .../provider/foundationdb/FDBRecordStore.java | 135 ++++++++++-------- .../foundationdb/FDBRecordStoreBase.java | 8 +- .../provider/foundationdb/FDBStoreTimer.java | 3 +- .../FDBRecordStoreOpeningTest.java | 2 +- .../FDBRecordStorePerformanceTest.java | 2 +- .../foundationdb/FDBRecordStoreTest.java | 2 +- .../OnlineIndexerUniqueIndexTest.java | 10 +- .../foundationdb/indexes/TextIndexTest.java | 4 +- .../indexes/VersionIndexTest.java | 1 + .../FDBRecordStoreStateCacheTest.java | 1 + .../ddl/DropSchemaConstantAction.java | 4 +- .../RecordLayerStoreCatalogImplTest.java | 5 +- ...reCatalogWithNoTemplateOperationsTest.java | 5 +- 13 files changed, 97 insertions(+), 85 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java index e67bc86d44..bed4d2e5c3 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java @@ -1780,19 +1780,65 @@ public void deleteRecordSplits(final @Nonnull Tuple primaryK } /** - * Delete the record store at the given {@link KeySpacePath}. This behaves like + * Delete the record store at the given {@link KeySpacePath}; deprecated, use + * {@link #deleteStoreAsync(FDBRecordContext, KeySpacePath)} instead. + * This behaves like * {@link #deleteStore(FDBRecordContext, Subspace)} on the record store saved * at {@link KeySpacePath#toSubspace(FDBRecordContext)}. + * This is a blocking call that calls {@link FDBRecordContext#asyncToSync}. * * @param context the transactional context in which to delete the record store * @param path the path to the record store - * @see #deleteStore(FDBRecordContext, Subspace) */ + @API(API.Status.DEPRECATED) public static void deleteStore(FDBRecordContext context, KeySpacePath path) { final Subspace subspace = path.toSubspace(context); deleteStore(context, subspace); } + /** + * Delete the record store at the given {@link Subspace}; deprecated, use + * {@link #deleteStoreAsync(FDBRecordContext, Subspace)} instead. In addition to the store's + * data this will delete the store's header and therefore will remove any evidence that + * the store existed. + * + *

+ * This method does not read the underlying record store, so it does not validate + * that a record store exists in the given subspace. As it might be the case that + * this record store has a cacheable store state (see {@link #setStateCacheability(boolean)}), + * this method resets the database's + * {@linkplain FDBRecordContext#getMetaDataVersionStamp(IsolationLevel) meta-data version-stamp}. + * As a result, calling this method may cause other clients to invalidate their caches needlessly. + *

+ * + * @param context the transactional context in which to delete the record store + * @param subspace the subspace containing the record store + */ + @API(API.Status.DEPRECATED) + @SuppressWarnings("PMD.CloseResource") + public static void deleteStore(FDBRecordContext context, Subspace subspace) { + // In theory, we only need to set the meta-data version stamp if the record store's + // meta-data is cacheable, deleteStoreAsync checks that + context.setMetaDataVersionStamp(); + context.setDirtyStoreState(true); + context.clear(subspace.range()); + } + + + /** + * Delete the record store at the given {@link KeySpacePath}. This behaves like + * {@link #deleteStoreAsync(FDBRecordContext, Subspace)} on the record store saved + * at {@link KeySpacePath#toSubspaceAsync(FDBRecordContext)}. + * + * @param context the transactional context in which to delete the record store + * @param path the path to the record store + * @return A future that will be completed once the store is deleted + * @see #deleteStoreAsync(FDBRecordContext, Subspace) + */ + public static CompletableFuture deleteStoreAsync(FDBRecordContext context, KeySpacePath path) { + return path.toSubspaceAsync(context).thenCompose(subspace -> deleteStoreAsync(context, subspace)); + } + /** * Delete the record store at the given {@link Subspace}. In addition to the store's * data this will delete the store's header and therefore will remove any evidence that @@ -1809,74 +1855,37 @@ public static void deleteStore(FDBRecordContext context, KeySpacePath path) { * contend on the single meta-data version-stamp key. *

* - *

- * The header read uses snapshot isolation to avoid adding a read-conflict range on every - * delete. Only when the on-disk header says the store is non-cacheable (or is missing) — - * i.e., the case in which we would skip the bump — do we add an explicit point read - * conflict on {@link #STORE_INFO_KEY}. That closes the race with a concurrent - * {@link #setStateCacheability(boolean)} that flips the store to cacheable: if such a - * writer commits before us, our commit fails with a serialization error and the caller - * retries with a fresh snapshot that sees the new header and bumps the stamp. In the - * common case (delete of a store that was and stays non-cacheable), no read conflict is - * added at all. - *

- * * @param context the transactional context in which to delete the record store * @param subspace the subspace containing the record store + * @return A future that will be completed once the store is deleted */ @SuppressWarnings("PMD.CloseResource") - public static void deleteStore(FDBRecordContext context, Subspace subspace) { - // Read the store header at snapshot isolation so we don't add a read conflict on - // every delete. The clear below already creates a write conflict on the whole range, - // which serializes us against any concurrent writer that lands INSIDE this subspace. - // The single race the snapshot read leaves open is a concurrent commit that writes - // STORE_INFO_KEY (e.g. a setStateCacheability that flips cacheable=true and bumps - // the meta-data version stamp). If that writer commits first, our snapshot read - // misses the flip and we would skip our own bump — leaving sibling caches populated - // from the writer's committed state stale after our delete. - // - // We close that race narrowly: only on the branches where we decide NOT to bump - // (header absent, or header parses as non-cacheable), we add an explicit point read - // conflict on STORE_INFO_KEY. That way: - // - the common case (delete of a store that was and stays non-cacheable) still - // contributes zero read conflicts, and - // - the racy case (concurrent flip to cacheable) is detected: the writer's SET on - // STORE_INFO_KEY overlaps our added read conflict and we lose the commit race. + public static CompletableFuture deleteStoreAsync(FDBRecordContext context, Subspace subspace) { + // Every writer to the store must read the store header, if it is not cached, otherwise they must check + // the MetaDataVersionStamp if it is cacheable, thus we must bump the MetaDataVersionStamp if it is cacheable, + // but otherwise we don't need to do so. final byte[] headerKey = subspace.pack(STORE_INFO_KEY); - final byte[] headerBytes = context.asyncToSync(FDBStoreTimer.Waits.WAIT_LOAD_RECORD_STORE_STATE, - context.readTransaction(true).get(headerKey)); - boolean shouldBump; - if (headerBytes == null) { - // Header absent: no cached state exists to invalidate — no bump needed. But guard - // against a concurrent creator/enabler flipping the store to cacheable underneath - // us: adding a read conflict on STORE_INFO_KEY makes such a writer's commit - // exclude ours. - context.ensureActive().addReadConflictKey(headerKey); - shouldBump = false; - } else { - boolean cacheable; - try { - cacheable = RecordMetaDataProto.DataStoreInfo.parseFrom(headerBytes).getCacheable(); - } catch (InvalidProtocolBufferException e) { - // If we can't parse the header, fall back to the conservative - // behavior and bump. Also skip the read conflict — bumping unconditionally - // is safe regardless of what a concurrent writer does. - cacheable = true; - } - if (cacheable) { - shouldBump = true; - } else { - // Header says non-cacheable AND parsed cleanly. If a concurrent writer flips - // it to cacheable, we must lose the commit race so the caller retries. - context.ensureActive().addReadConflictKey(headerKey); + return context.readTransaction(false).get(headerKey).thenAccept(headerBytes -> { + boolean shouldBump; + if (headerBytes == null) { + // Header absent: no cached state exists to invalidate — no bump needed. shouldBump = false; + } else { + try { + // If the header says it is not cacheable, we don't need to bump the MetaDataVersion. + // If the header says it is cacheable, we need to bump the MetaDataVersion. + shouldBump = RecordMetaDataProto.DataStoreInfo.parseFrom(headerBytes).getCacheable(); + } catch (InvalidProtocolBufferException e) { + // If we can't parse the header, fall back to the conservative behavior and bump. + shouldBump = true; + } } - } - if (shouldBump) { - context.setMetaDataVersionStamp(); - } - context.setDirtyStoreState(true); - context.clear(subspace.range()); + if (shouldBump) { + context.setMetaDataVersionStamp(); + } + context.setDirtyStoreState(true); + context.clear(subspace.range()); + }); } @Override diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStoreBase.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStoreBase.java index 83aefca819..d46b67f392 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStoreBase.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStoreBase.java @@ -1661,12 +1661,12 @@ default boolean deleteRecord(@Nonnull Tuple primaryKey) { * Delete all the data in the record store. *

* Everything except the store header and index state information is cleared from the database. - * This is is an efficient operation as all data are contiguous. + * This is an efficient operation as all data are contiguous. * This means that any {@linkplain IndexState#DISABLED disabled} or {@linkplain IndexState#WRITE_ONLY write-only} * index will remain in its disabled or write-only state after all of the data are cleared. If one also wants * to reset all index states, one can call {@link FDBRecordStore#rebuildAllIndexes()}, which should complete * quickly on an empty record store. If one wants to remove the record store entirely (including the store - * header and all index states), one should call {@link FDBRecordStore#deleteStore(FDBRecordContext, KeySpacePath)} + * header and all index states), one should call {@link FDBRecordStore#deleteStoreAsync(FDBRecordContext, KeySpacePath)} * instead of this method. * *

@@ -1676,8 +1676,8 @@ default boolean deleteRecord(@Nonnull Tuple primaryKey) { * See: Issue #398. *

* - * @see FDBRecordStore#deleteStore(FDBRecordContext, KeySpacePath) - * @see FDBRecordStore#deleteStore(FDBRecordContext, Subspace) + * @see FDBRecordStore#deleteStoreAsync(FDBRecordContext, KeySpacePath) + * @see FDBRecordStore#deleteStoreAsync(FDBRecordContext, Subspace) */ void deleteAllRecords(); diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBStoreTimer.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBStoreTimer.java index 3554250890..42a34eb9a1 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBStoreTimer.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBStoreTimer.java @@ -476,7 +476,8 @@ public enum Waits implements Wait { WAIT_LOAD_LUCENE_PARTITION_METADATA("wait to load lucene partition metadata"), /** Wait to run all the postClose hooks. */ WAIT_RUN_CLOSE_HOOKS("Wait to run all postClose hooks"), - ; + /** Wait to delete a store. */ + WAIT_DELETE_STORE("wait to delete store"); private final String title; private final String logKey; diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStoreOpeningTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStoreOpeningTest.java index fe4c65be5d..1f1a355f25 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStoreOpeningTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStoreOpeningTest.java @@ -581,7 +581,7 @@ void storeExistenceChecksWithNoRecords() throws Exception { // Delete the record store, then insert a key at an unknown keyspace try (FDBRecordContext context = openContext()) { - FDBRecordStore.deleteStore(context, path); + FDBRecordStore.deleteStoreAsync(context, path).join(); Subspace subspace = path.toSubspace(context); context.ensureActive().set(subspace.pack("unknown_keyspace"), Tuple.from("doesn't matter").pack()); commit(context); diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStorePerformanceTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStorePerformanceTest.java index acd969cb26..e0f8faab5b 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStorePerformanceTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStorePerformanceTest.java @@ -189,7 +189,7 @@ public void populate() { .setKeySpacePath(keyspacePath); final FDBRecordStore recordStore; if (n == 0) { - FDBRecordStore.deleteStore(context, keyspacePath); + FDBRecordStore.deleteStoreAsync(context, keyspacePath).join(); recordStore = storeBuilder.create(); } else { recordStore = storeBuilder.open(); diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStoreTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStoreTest.java index b1ea25e679..b9dc0af577 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStoreTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStoreTest.java @@ -926,7 +926,7 @@ public void testUserVersionDeterminesMetaData() { final SwitchingProvider newProvider2 = new SwitchingProvider(102, metaData1, metaData2); try (FDBRecordContext context = openContext()) { - FDBRecordStore.deleteStore(context, path); + FDBRecordStore.deleteStoreAsync(context, path).join(); recordStore = FDBRecordStore.newBuilder() .setContext(context).setKeySpacePath(path).setMetaDataProvider(newProvider2).setUserVersionChecker(newProvider2) .create(); diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexerUniqueIndexTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexerUniqueIndexTest.java index 2da7f1812b..18e12e6428 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexerUniqueIndexTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/OnlineIndexerUniqueIndexTest.java @@ -105,7 +105,7 @@ void uniquenessViolations() { // Case 3: Some in write-only mode. fdb.run(context -> { - FDBRecordStore.deleteStore(context, path); + FDBRecordStore.deleteStoreAsync(context, path).join(); return null; }); openSimpleMetaData(); @@ -129,7 +129,7 @@ void uniquenessViolations() { // Case 4: Some in write-only mode with an initial range build that shouldn't affect anything. fdb.run(context -> { - FDBRecordStore.deleteStore(context, path); + FDBRecordStore.deleteStoreAsync(context, path).join(); return null; }); openSimpleMetaData(); @@ -153,7 +153,7 @@ void uniquenessViolations() { // Case 5: Should be caught by write-only writes after build. fdb.run(context -> { - FDBRecordStore.deleteStore(context, path); + FDBRecordStore.deleteStoreAsync(context, path).join(); return null; }); openSimpleMetaData(); @@ -183,7 +183,7 @@ void uniquenessViolations() { // Case 6: Should be caught by write-only writes after partial build. fdb.run(context -> { - FDBRecordStore.deleteStore(context, path); + FDBRecordStore.deleteStoreAsync(context, path).join(); return null; }); openSimpleMetaData(); @@ -215,7 +215,7 @@ void uniquenessViolations() { // Case 7: The second of these two transactions should fail on not_committed, and then // there should be a uniqueness violation. fdb.run(context -> { - FDBRecordStore.deleteStore(context, path); + FDBRecordStore.deleteStoreAsync(context, path).join(); return null; }); openSimpleMetaData(); diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/TextIndexTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/TextIndexTest.java index f60874a5d9..e468c077cd 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/TextIndexTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/TextIndexTest.java @@ -586,7 +586,7 @@ void saveSimpleDocumentsWithPrefix() throws Exception { commit(context); } try (FDBRecordContext context = openContext()) { - FDBRecordStore.deleteStore(context, recordStore.getSubspace()); + FDBRecordStore.deleteStoreAsync(context, recordStore.getSubspace()).join(); commit(context); } try (FDBRecordContext context = openContext()) { @@ -864,7 +864,7 @@ void saveComplexMultiDocuments() throws Exception { commit(context); } try (FDBRecordContext context = openContext()) { - FDBRecordStore.deleteStore(context, recordStore.getSubspace()); + FDBRecordStore.deleteStoreAsync(context, recordStore.getSubspace()).join(); commit(context); } // Then, repeated key comes *after* text. diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/VersionIndexTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/VersionIndexTest.java index b0227434ed..775122e2d1 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/VersionIndexTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/VersionIndexTest.java @@ -3041,6 +3041,7 @@ static Stream deleteStoreWithUncommittedVersionData() { @ParameterizedTest(name = "deleteStoreWithUncommittedVersionData [" + ARGUMENTS_PLACEHOLDER + "]") @MethodSource void deleteStoreWithUncommittedVersionData(FormatVersion testFormatVersion, boolean testSplitLongRecords, boolean clearPath) throws Exception { + // TODO parameterize this based on deleteStore & deleteStoreAsync formatVersion = testFormatVersion; splitLongRecords = testSplitLongRecords; diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java index 223eba82f6..105b89f2ee 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java @@ -645,6 +645,7 @@ public void existenceCheckOnCachedStoreStates(@Nonnull StateCacheTestContext tes @ParameterizedTest(name = "storeDeletionInSameContext (test context = {0})") @MethodSource("testContextSource") public void storeDeletionInSameContext(@Nonnull StateCacheTestContext testContext) throws Exception { + // TODO parameterize these tests on whether the use `deleteStore` or `deleteStoreAsync` fdb.setStoreStateCache(testContext.getCache(fdb)); FDBRecordStore.Builder storeBuilder; diff --git a/fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/ddl/DropSchemaConstantAction.java b/fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/ddl/DropSchemaConstantAction.java index c6560d9b5e..0baa6333ae 100644 --- a/fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/ddl/DropSchemaConstantAction.java +++ b/fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/ddl/DropSchemaConstantAction.java @@ -25,6 +25,7 @@ import com.apple.foundationdb.record.RecordCoreException; import com.apple.foundationdb.record.provider.foundationdb.FDBRecordContext; import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStore; +import com.apple.foundationdb.record.provider.foundationdb.FDBStoreTimer; import com.apple.foundationdb.record.provider.foundationdb.keyspace.KeySpace; import com.apple.foundationdb.relational.api.Transaction; import com.apple.foundationdb.relational.api.catalog.StoreCatalog; @@ -61,7 +62,8 @@ public void execute(Transaction txn) throws RelationalException { } FDBRecordContext ctx = txn.unwrap(FDBRecordContext.class); try { - FDBRecordStore.deleteStore(ctx, RelationalKeyspaceProvider.toDatabasePath(dbUri, keySpace).schemaPath(schemaName)); + ctx.asyncToSync(FDBStoreTimer.Waits.WAIT_DELETE_STORE, + FDBRecordStore.deleteStoreAsync(ctx, RelationalKeyspaceProvider.toDatabasePath(dbUri, keySpace).schemaPath(schemaName))); } catch (RecordCoreException ex) { throw ExceptionUtil.toRelationalException(ex); } diff --git a/fdb-relational-core/src/test/java/com/apple/foundationdb/relational/recordlayer/catalog/RecordLayerStoreCatalogImplTest.java b/fdb-relational-core/src/test/java/com/apple/foundationdb/relational/recordlayer/catalog/RecordLayerStoreCatalogImplTest.java index 1adbea789b..d8378b7e89 100644 --- a/fdb-relational-core/src/test/java/com/apple/foundationdb/relational/recordlayer/catalog/RecordLayerStoreCatalogImplTest.java +++ b/fdb-relational-core/src/test/java/com/apple/foundationdb/relational/recordlayer/catalog/RecordLayerStoreCatalogImplTest.java @@ -24,7 +24,6 @@ import com.apple.foundationdb.record.provider.foundationdb.FDBRecordContext; import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStore; import com.apple.foundationdb.record.provider.foundationdb.keyspace.KeySpacePath; -import com.apple.foundationdb.test.FDBTestEnvironment; import com.apple.foundationdb.relational.api.Transaction; import com.apple.foundationdb.relational.api.exceptions.ErrorCode; import com.apple.foundationdb.relational.api.exceptions.RelationalException; @@ -34,7 +33,7 @@ import com.apple.foundationdb.relational.api.metadata.View; import com.apple.foundationdb.relational.recordlayer.RecordContextTransaction; import com.apple.foundationdb.relational.recordlayer.RelationalKeyspaceProvider; - +import com.apple.foundationdb.test.FDBTestEnvironment; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; @@ -62,7 +61,7 @@ void deleteAllRecords() throws RelationalException { try (Transaction txn = new RecordContextTransaction(fdb.openContext())) { final KeySpacePath keySpacePath = RelationalKeyspaceProvider.toDatabasePath(URI.create("/__SYS"), keySpace).schemaPath("CATALOG"); - FDBRecordStore.deleteStore(txn.unwrap(FDBRecordContext.class), keySpacePath); + FDBRecordStore.deleteStoreAsync(txn.unwrap(FDBRecordContext.class), keySpacePath).join(); txn.commit(); } } diff --git a/fdb-relational-core/src/test/java/com/apple/foundationdb/relational/recordlayer/catalog/RecordLayerStoreCatalogWithNoTemplateOperationsTest.java b/fdb-relational-core/src/test/java/com/apple/foundationdb/relational/recordlayer/catalog/RecordLayerStoreCatalogWithNoTemplateOperationsTest.java index a36f6bbc87..3693b34613 100644 --- a/fdb-relational-core/src/test/java/com/apple/foundationdb/relational/recordlayer/catalog/RecordLayerStoreCatalogWithNoTemplateOperationsTest.java +++ b/fdb-relational-core/src/test/java/com/apple/foundationdb/relational/recordlayer/catalog/RecordLayerStoreCatalogWithNoTemplateOperationsTest.java @@ -24,7 +24,6 @@ import com.apple.foundationdb.record.provider.foundationdb.FDBRecordContext; import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStore; import com.apple.foundationdb.record.provider.foundationdb.keyspace.KeySpacePath; -import com.apple.foundationdb.test.FDBTestEnvironment; import com.apple.foundationdb.relational.api.Transaction; import com.apple.foundationdb.relational.api.exceptions.ErrorCode; import com.apple.foundationdb.relational.api.exceptions.RelationalException; @@ -32,7 +31,7 @@ import com.apple.foundationdb.relational.api.metadata.SchemaTemplate; import com.apple.foundationdb.relational.recordlayer.RecordContextTransaction; import com.apple.foundationdb.relational.recordlayer.RelationalKeyspaceProvider; - +import com.apple.foundationdb.test.FDBTestEnvironment; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; @@ -56,7 +55,7 @@ void setUpCatalog() throws RelationalException { void deleteAllRecords() throws RelationalException { try (Transaction txn = new RecordContextTransaction(fdb.openContext())) { final KeySpacePath keySpacePath = RelationalKeyspaceProvider.instance().toDatabasePath(URI.create("/__SYS")).schemaPath("CATALOG"); - FDBRecordStore.deleteStore(txn.unwrap(FDBRecordContext.class), keySpacePath); + FDBRecordStore.deleteStoreAsync(txn.unwrap(FDBRecordContext.class), keySpacePath).join(); txn.commit(); } } From bdc6f026b3686022305096ecfe3578cff7455dc6 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Fri, 24 Jul 2026 10:10:47 -0400 Subject: [PATCH 08/14] Parameterize the some tests to run with old & new deleteStore methods --- .../indexes/VersionIndexTest.java | 17 +- .../FDBRecordStoreStateCacheTest.java | 158 ++++++++++++------ .../foundationdb/DeleteStoreMode.java | 62 +++++++ 3 files changed, 177 insertions(+), 60 deletions(-) create mode 100644 fdb-record-layer-core/src/testFixtures/java/com/apple/foundationdb/record/provider/foundationdb/DeleteStoreMode.java diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/VersionIndexTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/VersionIndexTest.java index 775122e2d1..7c2ef24b04 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/VersionIndexTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/VersionIndexTest.java @@ -51,6 +51,7 @@ import com.apple.foundationdb.record.metadata.expressions.KeyExpression; import com.apple.foundationdb.record.metadata.expressions.VersionKeyExpression; import com.apple.foundationdb.record.provider.foundationdb.APIVersion; +import com.apple.foundationdb.record.provider.foundationdb.DeleteStoreMode; import com.apple.foundationdb.record.provider.foundationdb.FDBDatabase; import com.apple.foundationdb.record.provider.foundationdb.FDBIndexedRecord; import com.apple.foundationdb.record.provider.foundationdb.FDBQueriedRecord; @@ -3031,17 +3032,19 @@ void deleteAllRecordsWithUncommittedData(FormatVersion testFormatVersion, boolea static Stream deleteStoreWithUncommittedVersionData() { return formatVersionsOfInterest().flatMap(testFormatVersion -> Stream.of(false, true).flatMap(testSplitLongRecords -> - Stream.of(false, true).map(clearPath -> - Arguments.of(testFormatVersion, testSplitLongRecords, clearPath) + Stream.of(false, true).flatMap(clearPath -> + Stream.of(DeleteStoreMode.values()).map(deleteStoreMode -> + Arguments.of(testFormatVersion, testSplitLongRecords, clearPath, deleteStoreMode) + ) ) ) ); } - @ParameterizedTest(name = "deleteStoreWithUncommittedVersionData [" + ARGUMENTS_PLACEHOLDER + "]") + @ParameterizedTest @MethodSource - void deleteStoreWithUncommittedVersionData(FormatVersion testFormatVersion, boolean testSplitLongRecords, boolean clearPath) throws Exception { - // TODO parameterize this based on deleteStore & deleteStoreAsync + void deleteStoreWithUncommittedVersionData(FormatVersion testFormatVersion, boolean testSplitLongRecords, boolean clearPath, + DeleteStoreMode deleteStoreMode) throws Exception { formatVersion = testFormatVersion; splitLongRecords = testSplitLongRecords; @@ -3099,7 +3102,7 @@ void deleteStoreWithUncommittedVersionData(FormatVersion testFormatVersion, bool if (clearPath) { path1.deleteAllData(context); } else { - FDBRecordStore.deleteStore(context, path1); + deleteStoreMode.deleteStore(context, path1); } i = 0; @@ -3116,7 +3119,7 @@ void deleteStoreWithUncommittedVersionData(FormatVersion testFormatVersion, bool if (clearPath) { path.deleteAllData(context); } else { - FDBRecordStore.deleteStore(context, path); + deleteStoreMode.deleteStore(context, path); } } else { FDBRecordStore pathStore = recordStore.asBuilder() diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java index 105b89f2ee..20630b202f 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java @@ -29,6 +29,7 @@ import com.apple.foundationdb.record.expressions.RecordKeyExpressionProto; import com.apple.foundationdb.record.metadata.Key; import com.apple.foundationdb.record.metadata.expressions.KeyExpression; +import com.apple.foundationdb.record.provider.foundationdb.DeleteStoreMode; import com.apple.foundationdb.record.provider.foundationdb.FDBDatabase; import com.apple.foundationdb.record.provider.foundationdb.FDBDatabaseFactory; import com.apple.foundationdb.record.provider.foundationdb.FDBExceptions; @@ -54,6 +55,8 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.parallel.Isolated; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.EnumSource; import org.junit.jupiter.params.provider.MethodSource; import javax.annotation.Nonnull; @@ -100,6 +103,12 @@ public static Stream testContextSource() { return Stream.of(new ReadVersionStateCacheTestContext(), new MetaDataVersionStampStateCacheTestContext()); } + @Nonnull + public static Stream testContextAndDeleteStoreModeSource() { + return testContextSource().flatMap(testContext -> + Stream.of(DeleteStoreMode.values()).map(mode -> Arguments.of(testContext, mode))); + } + /** * A wrapper interface for dealing with the differences between the different {@link FDBRecordStoreStateCache} * implementations. @@ -642,10 +651,10 @@ public void existenceCheckOnCachedStoreStates(@Nonnull StateCacheTestContext tes * Validate that deleting a record store causes the record store to go back to the database as it's possible the * cached stuff is what was deleted. */ - @ParameterizedTest(name = "storeDeletionInSameContext (test context = {0})") - @MethodSource("testContextSource") - public void storeDeletionInSameContext(@Nonnull StateCacheTestContext testContext) throws Exception { - // TODO parameterize these tests on whether the use `deleteStore` or `deleteStoreAsync` + @ParameterizedTest(name = "storeDeletionInSameContext (test context = {0}, deleteMode = {1})") + @MethodSource("testContextAndDeleteStoreModeSource") + public void storeDeletionInSameContext(@Nonnull StateCacheTestContext testContext, + @Nonnull DeleteStoreMode deleteStoreMode) throws Exception { fdb.setStoreStateCache(testContext.getCache(fdb)); FDBRecordStore.Builder storeBuilder; @@ -660,7 +669,7 @@ public void storeDeletionInSameContext(@Nonnull StateCacheTestContext testContex assertCacheHit(context.getTimer(), 1); context.getTimer().reset(); - FDBRecordStore.deleteStore(context, recordStore.getSubspace()); + deleteStoreMode.deleteStore(context, recordStore.getSubspace()); recordStore.asBuilder().create(); assertCacheMiss(context.getTimer(), 1); @@ -704,9 +713,10 @@ public void storeDeletionInSameContext(@Nonnull StateCacheTestContext testContex /** * After a store is deleted, validate that future transactions need to reload it from cache. */ - @ParameterizedTest(name = "storeDeletionAcrossContexts (test context = {0})") - @MethodSource("testContextSource") - public void storeDeletionAcrossContexts(@Nonnull StateCacheTestContext testContext) throws Exception { + @ParameterizedTest(name = "storeDeletionAcrossContexts (test context = {0}, deleteMode = {1})") + @MethodSource("testContextAndDeleteStoreModeSource") + public void storeDeletionAcrossContexts(@Nonnull StateCacheTestContext testContext, + @Nonnull DeleteStoreMode deleteStoreMode) throws Exception { fdb.setStoreStateCache(testContext.getCache(fdb)); FDBRecordStore.Builder storeBuilder; @@ -721,7 +731,7 @@ public void storeDeletionAcrossContexts(@Nonnull StateCacheTestContext testConte try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { openSimpleRecordStore(context); assertCacheHit(context.getTimer(), 1); - FDBRecordStore.deleteStore(context, recordStore.getSubspace()); + deleteStoreMode.deleteStore(context, recordStore.getSubspace()); commit(context); } @@ -771,23 +781,38 @@ public void storeDeletionAcrossContexts(@Nonnull StateCacheTestContext testConte } /** - * Deleting a non-cacheable store must NOT bump the meta-data version stamp: no other - * client can possibly hold a cached copy of a non-cacheable header, so the version stamp - * (a JVM-wide bottleneck on the {@code \xff/metadataVersion} key) shouldn't be touched. - * This means that deleting a store won't cause any interaction with any cacheable store to conflict. + * Behavior of {@link FDBRecordStore#deleteStore}/{@link FDBRecordStore#deleteStoreAsync} on a non-cacheable store, + * across both modes: + * + *
    + *
  • {@link DeleteStoreMode#ASYNC ASYNC} (new): reads the header, sees non-cacheable, and skips the meta-data + * version-stamp bump. No other client can hold a cached copy of a non-cacheable header, so bumping the + * cluster-wide bottleneck on the {@code \xff/metadataVersion} key would be pure waste. This is key to + * reducing cache invalidations and conflicts if a cluster is using the meta-data version-stamp, and is + * frequently deleting non-cacheable stores. For, example, if you have the relational catalog, and are + * frequently creating/deleting unrelated schemas, they would have conflicted with each other, because the + * catalog is a store with a cached header.
    • + *
  • {@link DeleteStoreMode#SYNC SYNC} (deprecated): bumps the meta-data version stamp unconditionally without + * reading the header — the pre-{@code deleteStoreAsync} behavior. Documented here so the deprecated method's + * cost is not silently forgotten.
    • + *
*/ - @Test - void deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp() throws Exception { + @ParameterizedTest(name = "deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp [{0}]") + @EnumSource(DeleteStoreMode.class) + void deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { ensureMetaDataVersionStampInitialized(); - deleteStoreDoesNotBumpMetaDataVersion(createStore(false).subspace()); + assertMetaDataVersionStampBehaviorOnNonCacheableOrMissingStore(createStore(false).subspace(), deleteStoreMode); } /** - * Deleting an empty subspace (no store header present) must not bump the stamp either — - * there's no cached header to invalidate. + * Behavior of {@link FDBRecordStore#deleteStore} on an empty subspace with no store + * header, mirroring the {@link #deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp} + * split: {@link DeleteStoreMode#ASYNC ASYNC} sees the missing header and skips the bump; + * {@link DeleteStoreMode#SYNC SYNC} bumps unconditionally. */ - @Test - void deleteMissingStoreDoesNotBumpMetaDataVersionStamp() { + @ParameterizedTest(name = "deleteMissingStoreDoesNotBumpMetaDataVersionStamp [{0}]") + @EnumSource(DeleteStoreMode.class) + void deleteMissingStoreDoesNotBumpMetaDataVersionStamp(@Nonnull DeleteStoreMode deleteStoreMode) { ensureMetaDataVersionStampInitialized(); // Use the test's per-instance path, but never open a store there. @@ -795,28 +820,44 @@ void deleteMissingStoreDoesNotBumpMetaDataVersionStamp() { try (FDBRecordContext context = openContext()) { subspace = path.toSubspace(context); } - deleteStoreDoesNotBumpMetaDataVersion(subspace); + assertMetaDataVersionStampBehaviorOnNonCacheableOrMissingStore(subspace, deleteStoreMode); } - private void deleteStoreDoesNotBumpMetaDataVersion(final Subspace subspace) { + /** + * Snapshot the meta-data version stamp, run a delete of the given subspace via {@code mode}, + * and assert the stamp behaved as the mode's contract requires: {@link DeleteStoreMode#SYNC} + * bumps unconditionally, {@link DeleteStoreMode#ASYNC} leaves the stamp untouched when the + * header is absent or marks the store non-cacheable. + */ + private void assertMetaDataVersionStampBehaviorOnNonCacheableOrMissingStore(final Subspace subspace, + @Nonnull DeleteStoreMode deleteStoreMode) { final byte[] beforeStamp = getMetaDataVersionStamp(); assertNotNull(beforeStamp); - deleteStore(subspace); + deleteStore(subspace, deleteStoreMode); final byte[] afterStamp = getMetaDataVersionStamp(); assertNotNull(afterStamp); - assertArrayEquals(beforeStamp, afterStamp, - "deleting a cacheable store should not have bumped the meta-data version stamp"); + if (deleteStoreMode == DeleteStoreMode.SYNC) { + assertFalse(Arrays.equals(beforeStamp, afterStamp), + "deprecated sync deleteStore always bumps the meta-data version stamp, " + + "even when the store header is absent or non-cacheable"); + } else { + assertArrayEquals(beforeStamp, afterStamp, + "async deleteStoreAsync must not bump the meta-data version stamp when the " + + "store header is absent or non-cacheable — nothing to invalidate"); + } } /** - * Complement of {@link #deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp()}: deleting - * a cacheable store MUST bump the stamp — otherwise sibling clients could keep serving - * reads out of a stale cached header long after the store is gone. + * Complement of {@link #deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp(DeleteStoreMode)}: + * deleting a cacheable store MUST bump the stamp — otherwise sibling clients could keep serving + * reads out of a stale cached header long after the store is gone. Both modes bump: SYNC does + * so unconditionally, ASYNC does so because the header parses as cacheable. */ - @Test - void deleteCacheableStoreBumpsMetaDataVersionStamp() throws Exception { + @ParameterizedTest(name = "deleteCacheableStoreBumpsMetaDataVersionStamp [{0}]") + @EnumSource(DeleteStoreMode.class) + void deleteCacheableStoreBumpsMetaDataVersionStamp(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { ensureMetaDataVersionStampInitialized(); final Subspace subspace = createStore(true).subspace(); @@ -824,7 +865,7 @@ void deleteCacheableStoreBumpsMetaDataVersionStamp() throws Exception { final byte[] beforeStamp = getMetaDataVersionStamp(); assertNotNull(beforeStamp); - deleteStore(subspace); + deleteStore(subspace, deleteStoreMode); final byte[] afterStamp = getMetaDataVersionStamp(); assertNotNull(afterStamp); @@ -833,13 +874,25 @@ void deleteCacheableStoreBumpsMetaDataVersionStamp() throws Exception { } /** - * Race check for {@link FDBRecordStore#deleteStore(FDBRecordContext, Subspace)}. - *

The bug we're guarding against: {@code deleteStore} commits without bumping the - * stamp, concurrently with another transaction that sets the store as cacheable. - * The delete should conflict.

+ * Race check for {@link FDBRecordStore#deleteStore(FDBRecordContext, Subspace)} vs a concurrent + * {@link FDBRecordStore#setStateCacheability(boolean) setStateCacheability(true)} flip that commits first. The two + * variants observe different outcomes: + * + *
    + *
  • {@link DeleteStoreMode#ASYNC ASYNC} (new): the SERIALIZABLE header read inside + * {@link FDBRecordStore#deleteStoreAsync(FDBRecordContext, Subspace) deleteStoreAsync} adds a read-conflict + * range on {@code STORE_INFO_KEY}, so the flipper's committed write forces the deleter to conflict and abort. + * Correctness invariant: a cacheable store cannot be silently deleted with the stamp still pointing at the + * flipper's potentially cached value.
    • + *
  • {@link DeleteStoreMode#SYNC SYNC} (deprecated): does not read the header and does not add any read-conflict + * range, so the deleter commits regardless of the flipper. This is ok, because the SYNC method always bump + * the meta-data version-stamp, and thus, any cached header would be invalidated + * (see {@link #deleteCacheableStoreBumpsMetaDataVersionStamp}).
    • + *
*/ - @Test - void concurrentSetCacheabilityPreventsDeleteStore() throws Exception { + @ParameterizedTest(name = "concurrentSetCacheabilityPreventsDeleteStore [{0}]") + @EnumSource(DeleteStoreMode.class) + void concurrentSetCacheabilityPreventsDeleteStore(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); ensureMetaDataVersionStampInitialized(); @@ -867,17 +920,19 @@ void concurrentSetCacheabilityPreventsDeleteStore() throws Exception { // deleterContext: reads STORE_INFO_KEY at snapshot at its pinned pre-flip RV, sees // non-cacheable. The deleteStore also adds a point read conflict // on STORE_INFO_KEY — which the flipper's committed write will conflict with. - FDBRecordStore.deleteStore(deleterContext, setup.subspace()); + deleteStoreMode.deleteStore(deleterContext, setup.subspace()); deleterCommitted = tryCommitOrDetectConflict(deleterContext); } - if (deleterCommitted) { + if (deleteStoreMode == DeleteStoreMode.SYNC) { + assertTrue(deleterCommitted, + "deprecated sync deleteStore does not add a read conflict on STORE_INFO_KEY, " + + "so it does not detect a concurrent setStateCacheability(true) flip"); + } else if (deleterCommitted) { fail("delete committed after a concurrent setStateCacheability(true) flip " + "and did not bump the meta-data version stamp; a subsequent writer " + "could have trusted its (now-stale) cache entry and inserted a record into a " + - "subspace whose store header has been deleted — an on-disk orphan. " + - "This is the correctness bug deleteStore's read-conflict guard is meant " + - "to prevent."); + "subspace whose store header has been deleted — an on-disk orphan. "); } else { // Deleter correctly conflicted with the flipper. The store still exists (the // flipper's cacheable header); caches remain consistent with disk. @@ -890,15 +945,12 @@ void concurrentSetCacheabilityPreventsDeleteStore() throws Exception { } /** - * Companion invariant to {@link #concurrentSetCacheabilityPreventsDeleteStore()}: with the - * commit order flipped (deleter first, then flipper), the flipper must conflict. Unlike its - * companion, this test relies on general read/write conflict machinery — specifically, that - * setStateCacheability's own SERIALIZABLE header read (via open() → loadRecordStoreStateAsync - * or handleCachedState) puts STORE_INFO_KEY in the flipper's read set, so any concurrent - * write to that key by a preceding delete forces a conflict. + * Companion invariant to {@link #concurrentSetCacheabilityPreventsDeleteStore(DeleteStoreMode)}: + * with the commit order flipped (deleter first, then flipper), the flipper must conflict. */ - @Test - void concurrentSetCacheabilityConflictsWithDeleteStore() throws Exception { + @ParameterizedTest(name = "concurrentSetCacheabilityConflictsWithDeleteStore [{0}]") + @EnumSource(DeleteStoreMode.class) + void concurrentSetCacheabilityConflictsWithDeleteStore(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); ensureMetaDataVersionStampInitialized(); @@ -909,7 +961,7 @@ void concurrentSetCacheabilityConflictsWithDeleteStore() throws Exception { try (FDBRecordContext flipperContext = fdb.openContext(null, new FDBStoreTimer())) { FDBRecordStore flipperStore = setup.builder().copyBuilder().setContext(flipperContext).open(); try (FDBRecordContext deleterContext = fdb.openContext(null, new FDBStoreTimer())) { - FDBRecordStore.deleteStore(deleterContext, setup.subspace()); + deleteStoreMode.deleteStore(deleterContext, setup.subspace()); commit(deleterContext); } assertTrue(flipperStore.setStateCacheability(true), @@ -1433,9 +1485,9 @@ private byte[] getMetaDataVersionStamp() { } } - private void deleteStore(final Subspace subspace) { + private void deleteStore(final Subspace subspace, @Nonnull DeleteStoreMode deleteStoreMode) { try (FDBRecordContext context = openContext()) { - FDBRecordStore.deleteStore(context, subspace); + deleteStoreMode.deleteStore(context, subspace); commit(context); } } diff --git a/fdb-record-layer-core/src/testFixtures/java/com/apple/foundationdb/record/provider/foundationdb/DeleteStoreMode.java b/fdb-record-layer-core/src/testFixtures/java/com/apple/foundationdb/record/provider/foundationdb/DeleteStoreMode.java new file mode 100644 index 0000000000..1e77eb4fe1 --- /dev/null +++ b/fdb-record-layer-core/src/testFixtures/java/com/apple/foundationdb/record/provider/foundationdb/DeleteStoreMode.java @@ -0,0 +1,62 @@ +/* + * DeleteStoreMode.java + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2015-2026 Apple Inc. and the FoundationDB project authors + * + * Licensed 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 com.apple.foundationdb.record.provider.foundationdb; + +import com.apple.foundationdb.record.provider.foundationdb.keyspace.KeySpacePath; +import com.apple.foundationdb.subspace.Subspace; + +import javax.annotation.Nonnull; + +/** + * Enum for parameterizing tests over the two variants of {@link FDBRecordStore#deleteStore}: + * the (deprecated) synchronous {@code deleteStore} and the newer asynchronous + * {@code deleteStoreAsync}. Callers dispatch via {@link #deleteStore(FDBRecordContext, KeySpacePath)} + * or {@link #deleteStore(FDBRecordContext, Subspace)}; in the ASYNC case the returned future is + * joined so callers observe the same synchronous shape regardless of variant. + */ +public enum DeleteStoreMode { + SYNC { + @Override + public void deleteStore(@Nonnull FDBRecordContext context, @Nonnull KeySpacePath path) { + FDBRecordStore.deleteStore(context, path); + } + + @Override + public void deleteStore(@Nonnull FDBRecordContext context, @Nonnull Subspace subspace) { + FDBRecordStore.deleteStore(context, subspace); + } + }, + ASYNC { + @Override + public void deleteStore(@Nonnull FDBRecordContext context, @Nonnull KeySpacePath path) { + FDBRecordStore.deleteStoreAsync(context, path).join(); + } + + @Override + public void deleteStore(@Nonnull FDBRecordContext context, @Nonnull Subspace subspace) { + FDBRecordStore.deleteStoreAsync(context, subspace).join(); + } + }; + + public abstract void deleteStore(@Nonnull FDBRecordContext context, @Nonnull KeySpacePath path); + + public abstract void deleteStore(@Nonnull FDBRecordContext context, @Nonnull Subspace subspace); +} From 445fd40208908f05dd843818650fdcf9041a413e Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Fri, 24 Jul 2026 12:36:40 -0400 Subject: [PATCH 09/14] Add more coverage of concurrent ops with deleteStore --- .../FDBRecordStoreStateCacheTest.java | 251 ++++++++++++------ 1 file changed, 176 insertions(+), 75 deletions(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java index 20630b202f..4ae3f2c01a 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java @@ -50,6 +50,7 @@ import com.apple.foundationdb.tuple.ByteArrayUtil; import com.apple.foundationdb.tuple.ByteArrayUtil2; import com.apple.foundationdb.tuple.Tuple; +import com.apple.test.ParameterizedTestUtils; import com.apple.test.Tags; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; @@ -109,6 +110,72 @@ public static Stream testContextAndDeleteStoreModeSource() { Stream.of(DeleteStoreMode.values()).map(mode -> Arguments.of(testContext, mode))); } + /** + * An operation applied on an open {@link FDBRecordStore} concurrently with a + * {@link FDBRecordStore#deleteStore}/{@link FDBRecordStore#deleteStoreAsync}. The interesting + * axis is whether the operation writes {@code STORE_INFO_KEY} — that is what determines + * whether {@link DeleteStoreMode#ASYNC deleteStoreAsync}'s SERIALIZABLE header read + * catches a concurrent commit and forces the deleter to conflict. + */ + private enum ConcurrentOperation { + /** + * Flips the store's cacheability flag (toggles the current value). Writes + * {@code STORE_INFO_KEY} via {@code updateStoreHeaderAsync} regardless of direction. + */ + SET_CACHEABILITY(true) { + @Override + void apply(@Nonnull FDBRecordStore store) { + final boolean currentlyCacheable = store.getRecordStoreState().getStoreHeader().getCacheable(); + assertTrue(store.setStateCacheability(!currentlyCacheable), + "flipping cacheability should have changed the header"); + } + }, + /** + * Sets a header user field. Writes {@code STORE_INFO_KEY} via + * {@code updateStoreHeaderAsync}, but does not touch the cacheable flag — a + * representative "other part of the header" mutation. + */ + SET_HEADER_USER_FIELD(true) { + @Override + void apply(@Nonnull FDBRecordStore store) { + store.setHeaderUserField("concurrent-op", new byte[]{1, 2, 3}); + } + }, + /** + * Saves a record. Writes only inside the records subspace — {@code STORE_INFO_KEY} + * is untouched, so a concurrent {@code deleteStoreAsync} does NOT observe any + * write in its read set. + */ + SAVE_RECORD(false) { + @Override + void apply(@Nonnull FDBRecordStore store) { + store.saveRecord(TestRecords1Proto.MySimpleRecord.newBuilder() + .setRecNo(1L) + .setStrValueIndexed("racy") + .build()); + } + }; + + private final boolean writesStoreHeader; + + ConcurrentOperation(boolean writesStoreHeader) { + this.writesStoreHeader = writesStoreHeader; + } + + /** + * Whether or not this operation writes to the the store header. + * @return {@code true} iff the operation writes {@code STORE_INFO_KEY} on commit. + * The write-set of the concurrent operation is what {@code deleteStoreAsync}'s + * header read conflicts with; operations that never touch {@code STORE_INFO_KEY} + * cannot cause the deleter to abort. + */ + boolean writesStoreHeader() { + return writesStoreHeader; + } + + abstract void apply(@Nonnull FDBRecordStore store); + } + /** * A wrapper interface for dealing with the differences between the different {@link FDBRecordStoreStateCache} * implementations. @@ -782,7 +849,7 @@ public void storeDeletionAcrossContexts(@Nonnull StateCacheTestContext testConte /** * Behavior of {@link FDBRecordStore#deleteStore}/{@link FDBRecordStore#deleteStoreAsync} on a non-cacheable store, - * across both modes: + * across both modes. * *
    *
  • {@link DeleteStoreMode#ASYNC ASYNC} (new): reads the header, sees non-cacheable, and skips the meta-data @@ -805,8 +872,8 @@ void deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp(@Nonnull DeleteStore } /** - * Behavior of {@link FDBRecordStore#deleteStore} on an empty subspace with no store - * header, mirroring the {@link #deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp} + * Behavior of {@link FDBRecordStore#deleteStore}/{@link FDBRecordStore#deleteStoreAsync} on an empty subspace with + * no store header, mirroring the {@link #deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp} * split: {@link DeleteStoreMode#ASYNC ASYNC} sees the missing header and skips the bump; * {@link DeleteStoreMode#SYNC SYNC} bumps unconditionally. */ @@ -873,114 +940,148 @@ void deleteCacheableStoreBumpsMetaDataVersionStamp(@Nonnull DeleteStoreMode dele "deleting a cacheable store should have bumped the meta-data version stamp"); } + + @Nonnull + public static Stream concurrentOperationPreventsDeleteStore() { + return ParameterizedTestUtils.cartesianProduct( + Stream.of(ConcurrentOperation.values()), + Stream.of(DeleteStoreMode.values()), + ParameterizedTestUtils.booleans("startCacheable") + ); + } + /** - * Race check for {@link FDBRecordStore#deleteStore(FDBRecordContext, Subspace)} vs a concurrent - * {@link FDBRecordStore#setStateCacheability(boolean) setStateCacheability(true)} flip that commits first. The two - * variants observe different outcomes: + * The concurrent operation commits BEFORE the delete. What matters is whether the + * delete's own commit machinery catches the concurrent modification and forces the + * deleter to abort. * + *

    The key expectatian is that either:

    *
      - *
    • {@link DeleteStoreMode#ASYNC ASYNC} (new): the SERIALIZABLE header read inside - * {@link FDBRecordStore#deleteStoreAsync(FDBRecordContext, Subspace) deleteStoreAsync} adds a read-conflict - * range on {@code STORE_INFO_KEY}, so the flipper's committed write forces the deleter to conflict and abort. - * Correctness invariant: a cacheable store cannot be silently deleted with the stamp still pointing at the - * flipper's potentially cached value.
      • - *
    • {@link DeleteStoreMode#SYNC SYNC} (deprecated): does not read the header and does not add any read-conflict - * range, so the deleter commits regardless of the flipper. This is ok, because the SYNC method always bump - * the meta-data version-stamp, and thus, any cached header would be invalidated - * (see {@link #deleteCacheableStoreBumpsMetaDataVersionStamp}).
      • + *
    • The delete succeeds, and the range is empty, and if the store was even cacheable, the meta-data + * version-stamp was bumped.
      • + *
    • The delete conflicts.
      • *
    */ - @ParameterizedTest(name = "concurrentSetCacheabilityPreventsDeleteStore [{0}]") - @EnumSource(DeleteStoreMode.class) - void concurrentSetCacheabilityPreventsDeleteStore(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { + @ParameterizedTest + @MethodSource + void concurrentOperationPreventsDeleteStore(@Nonnull ConcurrentOperation op, + @Nonnull DeleteStoreMode deleteStoreMode, + boolean startCacheable) throws Exception { fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); ensureMetaDataVersionStampInitialized(); - // Setup: create the store as NON-cacheable. That is the trap for the deleter's - // snapshot header read. - final StoreSetup setup = createStore(false); + final StoreSetup setup = createStore(startCacheable); final boolean deleterCommitted; try (FDBRecordContext deleterContext = fdb.openContext(null, new FDBStoreTimer())) { - // Pin the deleter's read version BEFORE the flip commits, so the deleter's - // snapshot header read sees the pre-flip (non-cacheable) state. + // Pin the deleter's read version BEFORE the concurrent op commits. deleterContext.getReadVersion(); - // Separate transaction: flip the store to cacheable. Kept separate from the - // record insert below so that the record-inserting transaction has no reason to - // read the header itself — it will pick up the cacheable header via the state - // cache. - try (FDBRecordContext flipperContext = fdb.openContext(null, new FDBStoreTimer())) { - FDBRecordStore flipperStore = setup.builder().copyBuilder().setContext(flipperContext).open(); - assertTrue(flipperStore.setStateCacheability(true), - "flipping to cacheable should have changed the header"); - commit(flipperContext); + // Separate transaction: apply the concurrent operation and commit it. + try (FDBRecordContext opContext = fdb.openContext(null, new FDBStoreTimer())) { + FDBRecordStore opStore = setup.builder().copyBuilder().setContext(opContext).open(); + op.apply(opStore); + commit(opContext); } - // deleterContext: reads STORE_INFO_KEY at snapshot at its pinned pre-flip RV, sees - // non-cacheable. The deleteStore also adds a point read conflict - // on STORE_INFO_KEY — which the flipper's committed write will conflict with. + // Deleter deletes & commits. deleteStoreMode.deleteStore(deleterContext, setup.subspace()); deleterCommitted = tryCommitOrDetectConflict(deleterContext); } - if (deleteStoreMode == DeleteStoreMode.SYNC) { - assertTrue(deleterCommitted, - "deprecated sync deleteStore does not add a read conflict on STORE_INFO_KEY, " + - "so it does not detect a concurrent setStateCacheability(true) flip"); - } else if (deleterCommitted) { - fail("delete committed after a concurrent setStateCacheability(true) flip " + - "and did not bump the meta-data version stamp; a subsequent writer " + - "could have trusted its (now-stale) cache entry and inserted a record into a " + - "subspace whose store header has been deleted — an on-disk orphan. "); + final boolean expectDeleterConflicts = + deleteStoreMode == DeleteStoreMode.ASYNC && op.writesStoreHeader(); + if (expectDeleterConflicts) { + if (deleterCommitted) { + fail("expected deleteStoreAsync to conflict with a concurrent " + op + + " that wrote STORE_INFO_KEY " + + ", but it committed. The read-conflict range added by the header " + + "read did not fire."); + } + // Op's committed state should still be on disk. + try (FDBRecordContext peek = fdb.openContext(null, new FDBStoreTimer())) { + assertFalse(peek.ensureActive().getRange(setup.subspace().range()).asList().join().isEmpty(), + "op's committed state should still be on disk after the deleter conflicted"); + } } else { - // Deleter correctly conflicted with the flipper. The store still exists (the - // flipper's cacheable header); caches remain consistent with disk. - try (FDBRecordContext contextUsingCache = fdb.openContext(null, new FDBStoreTimer())) { - // ensure that we can open the store, guaranteeing the store header still exists - recordStore = setup.builder().copyBuilder().setContext(contextUsingCache).open(); - assertCacheable(); + assertTrue(deleterCommitted, + "expected the deleter to commit for op=" + op + " mode=" + deleteStoreMode + + " startCacheable=" + startCacheable); + // Deleter's clear(subspace.range()) supersedes whatever the op wrote. + try (FDBRecordContext peek = fdb.openContext(null, new FDBStoreTimer())) { + assertTrue(peek.ensureActive().getRange(setup.subspace().range()).asList().join().isEmpty(), + "deleter committed, so the subspace should have been cleared"); } } } + @Nonnull + public static Stream concurrentOperationConflictsCases() { + return ParameterizedTestUtils.cartesianProduct( + Stream.of(ConcurrentOperation.values()), + Stream.of(DeleteStoreMode.values()), + ParameterizedTestUtils.booleans("startCacheable"), + // for a store that is not cacheable, pre-warming the cache should be a no-op + ParameterizedTestUtils.booleans("cachePreWarmed") + ); + } + /** - * Companion invariant to {@link #concurrentSetCacheabilityPreventsDeleteStore(DeleteStoreMode)}: - * with the commit order flipped (deleter first, then flipper), the flipper must conflict. + * The delete commits BEFORE the concurrent operation. In all combinations, the operation's own {@code open()} adds + * {@code STORE_INFO_KEY} to its read set. That read-conflict range catches the prior + * delete's write via {@code clear(subspace.range())}, so the operation always aborts + * and the delete always sticks. */ - @ParameterizedTest(name = "concurrentSetCacheabilityConflictsWithDeleteStore [{0}]") - @EnumSource(DeleteStoreMode.class) - void concurrentSetCacheabilityConflictsWithDeleteStore(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { + @ParameterizedTest + @MethodSource("concurrentOperationConflictsCases") + void concurrentOperationConflictsWithDeleteStore(@Nonnull ConcurrentOperation op, + @Nonnull DeleteStoreMode deleteStoreMode, + boolean startCacheable, + boolean cachePreWarmed) throws Exception { fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); ensureMetaDataVersionStampInitialized(); - // Setup: create the store as NON-cacheable (matches the companion test's starting state). - final StoreSetup setup = createStore(false); + final StoreSetup setup = createStore(startCacheable); - final boolean flipperCommitted; - try (FDBRecordContext flipperContext = fdb.openContext(null, new FDBStoreTimer())) { - FDBRecordStore flipperStore = setup.builder().copyBuilder().setContext(flipperContext).open(); + if (cachePreWarmed) { + // Open the store once in a separate transaction so the state cache has an entry + // for this subspace before opContext.open() runs. For a cacheable store this + // makes the subsequent open() take the handleCachedState path; for a + // non-cacheable store the cache doesn't populate and this is effectively a no-op. + try (FDBRecordContext warmup = fdb.openContext(null, new FDBStoreTimer())) { + setup.builder().copyBuilder().setContext(warmup).open(); + } + } + + final boolean opCommitted; + try (FDBRecordContext opContext = fdb.openContext(null, new FDBStoreTimer())) { + // Open the store first — this adds STORE_INFO_KEY to opContext's read set at + // opContext's pinned read version, either via a SERIALIZABLE header load + // (cache miss) or via handleCachedState (cache hit). + FDBRecordStore opStore = setup.builder().copyBuilder().setContext(opContext).open(); + + // Delete and commit in a nested transaction, writing STORE_INFO_KEY (via clear). try (FDBRecordContext deleterContext = fdb.openContext(null, new FDBStoreTimer())) { deleteStoreMode.deleteStore(deleterContext, setup.subspace()); commit(deleterContext); } - assertTrue(flipperStore.setStateCacheability(true), - "flipping to cacheable should have changed the header"); - flipperCommitted = tryCommitOrDetectConflict(flipperContext); + + // Apply the concurrent operation on the already-open opStore and try to commit. + // The read-conflict range on STORE_INFO_KEY from the initial open() overlaps + // the deleter's committed write, so opContext must abort. + op.apply(opStore); + opCommitted = tryCommitOrDetectConflict(opContext); } - if (flipperCommitted) { - fail("a setStateCacheability(true) flip committed after a successful deleteStore; " + - "another transaction could have used the cached state to write after the " + - "delete store without there being a store header"); - } else { - // Deleter correctly conflicted with the flipper. The store still should have been deleted. - try (FDBRecordContext contextUsingCache = fdb.openContext(null, new FDBStoreTimer())) { - assertTrue(contextUsingCache.ensureActive().getRange(setup.subspace().range()).asList().join().isEmpty(), - "The store should have been deleted"); - recordStore = setup.builder().copyBuilder().setContext(contextUsingCache).create(); - assertNotCacheable(); - } + assertFalse(opCommitted, "op should have conflicted with the preceding deleteStore"); + + // Delete correctly landed. Subspace should be empty; a subsequent create() succeeds. + try (FDBRecordContext contextUsingCache = fdb.openContext(null, new FDBStoreTimer())) { + assertTrue(contextUsingCache.ensureActive() + .getRange(setup.subspace().range()).asList().join().isEmpty(), + "The store should have been deleted"); + recordStore = setup.builder().copyBuilder().setContext(contextUsingCache).create(); + assertNotCacheable(); } } From af83f908e633554af795f0a328700e12e1720467 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Fri, 24 Jul 2026 13:07:43 -0400 Subject: [PATCH 10/14] Move deleteStore tests out of FDBRecordStoreStateCacheTest Some of the new additions around delete store, seemed to really be going beyond the cache, so I moved all the delete store tests out. This was pretty mechanical, I just cut+pasted the tests, move the static objects to the utils class, and updated references. I did not move VersionIndexTest.deleteStoreWithUncommittedVersionData because that depended on a lot more inner details of the VersionIndexTest --- .../storestate/DeleteStoreTest.java | 592 +++++++++++++ .../FDBRecordStoreStateCacheTest.java | 777 ++---------------- .../FDBRecordStoreStateCacheTestUtils.java | 175 ++++ 3 files changed, 834 insertions(+), 710 deletions(-) create mode 100644 fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/DeleteStoreTest.java create mode 100644 fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTestUtils.java diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/DeleteStoreTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/DeleteStoreTest.java new file mode 100644 index 0000000000..0ab967a640 --- /dev/null +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/DeleteStoreTest.java @@ -0,0 +1,592 @@ +/* + * DeleteStoreTest.java + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2015-2026 Apple Inc. and the FoundationDB project authors + * + * Licensed 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 com.apple.foundationdb.record.provider.foundationdb.storestate; + +import com.apple.foundationdb.record.IsolationLevel; +import com.apple.foundationdb.record.TestRecords1Proto; +import com.apple.foundationdb.record.provider.foundationdb.DeleteStoreMode; +import com.apple.foundationdb.record.provider.foundationdb.FDBExceptions; +import com.apple.foundationdb.record.provider.foundationdb.FDBRecordContext; +import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStore; +import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStoreBase; +import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStoreTestBase; +import com.apple.foundationdb.record.provider.foundationdb.FDBStoreTimer; +import com.apple.foundationdb.subspace.Subspace; +import com.apple.test.ParameterizedTestUtils; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.EnumSource; +import org.junit.jupiter.params.provider.MethodSource; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.util.Arrays; +import java.util.stream.Stream; + +import static com.apple.foundationdb.record.provider.foundationdb.storestate.FDBRecordStoreStateCacheTestUtils.metaDataVersionStampCacheFactory; +import static com.apple.foundationdb.record.provider.foundationdb.storestate.FDBRecordStoreStateCacheTestUtils.testContextSource; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +public class DeleteStoreTest extends FDBRecordStoreTestBase { + + /** + * Validate that deleting a record store causes the record store to go back to the database as it's possible the + * cached stuff is what was deleted. + */ + @ParameterizedTest(name = "storeDeletionInSameContext (test context = {0}, deleteMode = {1})") + @MethodSource("testContextAndDeleteStoreModeSource") + public void storeDeletionInSameContext(@Nonnull FDBRecordStoreStateCacheTestUtils.StateCacheTestContext testContext, + @Nonnull DeleteStoreMode deleteStoreMode) throws Exception { + fdb.setStoreStateCache(testContext.getCache(fdb)); + + FDBRecordStore.Builder storeBuilder; + try (FDBRecordContext context = openContext()) { + openSimpleRecordStore(context); + storeBuilder = recordStore.asBuilder(); + commit(context); + } + + try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder)) { + openSimpleRecordStore(context); + assertCacheHit(context.getTimer()); + + context.getTimer().reset(); + deleteStoreMode.deleteStore(context, recordStore.getSubspace()); + recordStore.asBuilder().create(); + assertCacheMiss(context.getTimer()); + + commit(context); + } + + try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder)) { + openSimpleRecordStore(context); + assertCacheHit(context.getTimer()); + path.deleteAllData(context); + + context.getTimer().reset(); + recordStore.asBuilder().create(); + assertCacheMiss(context.getTimer()); + } + + // Deleting all records should not disable the index, so the result should still be cacheable. + // See: https://github.com/FoundationDB/fdb-record-layer/issues/399 + final String disabledIndex = "MySimpleRecord$str_value_indexed"; + try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { + openSimpleRecordStore(context); + assertCacheHit(context.getTimer()); + recordStore.markIndexDisabled(disabledIndex).get(); + commit(context); + } + + try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { + openSimpleRecordStore(context); + assertCacheHit(context.getTimer()); + assertTrue(recordStore.isIndexDisabled(disabledIndex)); + recordStore.deleteAllRecords(); + + context.getTimer().reset(); + recordStore = recordStore.asBuilder().open(); + assertCacheHit(context.getTimer()); + assertTrue(recordStore.isIndexDisabled(disabledIndex)); + commit(context); + } + } + + /** + * After a store is deleted, validate that future transactions need to reload it from cache. + */ + @ParameterizedTest(name = "storeDeletionAcrossContexts (test context = {0}, deleteMode = {1})") + @MethodSource("testContextAndDeleteStoreModeSource") + public void storeDeletionAcrossContexts(@Nonnull FDBRecordStoreStateCacheTestUtils.StateCacheTestContext testContext, + @Nonnull DeleteStoreMode deleteStoreMode) throws Exception { + fdb.setStoreStateCache(testContext.getCache(fdb)); + + FDBRecordStore.Builder storeBuilder; + try (FDBRecordContext context = openContext()) { + openSimpleRecordStore(context); + assertTrue(recordStore.setStateCacheability(true)); + storeBuilder = recordStore.asBuilder(); + commit(context); + } + + // Delete by calling deleteStore. + try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { + openSimpleRecordStore(context); + assertCacheHit(context.getTimer()); + deleteStoreMode.deleteStore(context, recordStore.getSubspace()); + commit(context); + } + + // After deleting it, when opening the same store again, it shouldn't be cached. + try (FDBRecordContext context = fdb.openContext(null, new FDBStoreTimer())) { + FDBRecordStore store = storeBuilder.setContext(context).create(); + assertCacheMiss(context.getTimer()); + assertTrue(store.setStateCacheability(true)); + commit(context); + } + + // Delete by calling path.deleteAllData + try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { + openSimpleRecordStore(context); + assertCacheHit(context.getTimer()); + path.deleteAllData(context); + commit(context); + } + + try (FDBRecordContext context = fdb.openContext(null, new FDBStoreTimer())) { + FDBRecordStore store = storeBuilder.setContext(context).create(); + store.setStateCacheabilityAsync(true).get(); + assertCacheMiss(context.getTimer()); + commit(context); + } + + // Deleting all records should not disable the index state. + final String disabledIndex = "MySimpleRecord$str_value_indexed"; + try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { + openSimpleRecordStore(context); + recordStore.markIndexDisabled(disabledIndex).get(); + commit(context); + } + + try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { + openSimpleRecordStore(context); + assertTrue(recordStore.isIndexDisabled(disabledIndex)); + recordStore.deleteAllRecords(); + commit(context); + } + + try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { + openSimpleRecordStore(context); + assertTrue(recordStore.isIndexDisabled(disabledIndex)); + commit(context); + } + } + + /** + * Behavior of {@link FDBRecordStore#deleteStore}/{@link FDBRecordStore#deleteStoreAsync} on a non-cacheable store, + * across both modes. + * + *
      + *
    • {@link DeleteStoreMode#ASYNC ASYNC} (new): reads the header, sees non-cacheable, and skips the meta-data + * version-stamp bump. No other client can hold a cached copy of a non-cacheable header, so bumping the + * cluster-wide bottleneck on the {@code \xff/metadataVersion} key would be pure waste. This is key to + * reducing cache invalidations and conflicts if a cluster is using the meta-data version-stamp, and is + * frequently deleting non-cacheable stores. For, example, if you have the relational catalog, and are + * frequently creating/deleting unrelated schemas, they would have conflicted with each other, because the + * catalog is a store with a cached header.
      • + *
    • {@link DeleteStoreMode#SYNC SYNC} (deprecated): bumps the meta-data version stamp unconditionally without + * reading the header — the pre-{@code deleteStoreAsync} behavior. Documented here so the deprecated method's + * cost is not silently forgotten.
      • + *
    + */ + @ParameterizedTest(name = "deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp [{0}]") + @EnumSource(DeleteStoreMode.class) + void deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { + ensureMetaDataVersionStampInitialized(); + assertMetaDataVersionStampBehaviorOnNonCacheableOrMissingStore(createStore(false).subspace(), deleteStoreMode); + } + + /** + * Behavior of {@link FDBRecordStore#deleteStore}/{@link FDBRecordStore#deleteStoreAsync} on an empty subspace with + * no store header, mirroring the {@link #deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp} + * split: {@link DeleteStoreMode#ASYNC ASYNC} sees the missing header and skips the bump; + * {@link DeleteStoreMode#SYNC SYNC} bumps unconditionally. + */ + @ParameterizedTest(name = "deleteMissingStoreDoesNotBumpMetaDataVersionStamp [{0}]") + @EnumSource(DeleteStoreMode.class) + void deleteMissingStoreDoesNotBumpMetaDataVersionStamp(@Nonnull DeleteStoreMode deleteStoreMode) { + ensureMetaDataVersionStampInitialized(); + + // Use the test's per-instance path, but never open a store there. + final Subspace subspace; + try (FDBRecordContext context = openContext()) { + subspace = path.toSubspace(context); + } + assertMetaDataVersionStampBehaviorOnNonCacheableOrMissingStore(subspace, deleteStoreMode); + } + + /** + * Snapshot the meta-data version stamp, run a delete of the given subspace via {@code mode}, + * and assert the stamp behaved as the mode's contract requires: {@link DeleteStoreMode#SYNC} + * bumps unconditionally, {@link DeleteStoreMode#ASYNC} leaves the stamp untouched when the + * header is absent or marks the store non-cacheable. + */ + private void assertMetaDataVersionStampBehaviorOnNonCacheableOrMissingStore(final Subspace subspace, + @Nonnull DeleteStoreMode deleteStoreMode) { + final byte[] beforeStamp = getMetaDataVersionStamp(); + assertNotNull(beforeStamp); + + deleteStore(subspace, deleteStoreMode); + + final byte[] afterStamp = getMetaDataVersionStamp(); + assertNotNull(afterStamp); + if (deleteStoreMode == DeleteStoreMode.SYNC) { + assertFalse(Arrays.equals(beforeStamp, afterStamp), + "deprecated sync deleteStore always bumps the meta-data version stamp, " + + "even when the store header is absent or non-cacheable"); + } else { + assertArrayEquals(beforeStamp, afterStamp, + "async deleteStoreAsync must not bump the meta-data version stamp when the " + + "store header is absent or non-cacheable — nothing to invalidate"); + } + } + + /** + * Complement of {@link #deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp(DeleteStoreMode)}: + * deleting a cacheable store MUST bump the stamp — otherwise sibling clients could keep serving + * reads out of a stale cached header long after the store is gone. Both modes bump: SYNC does + * so unconditionally, ASYNC does so because the header parses as cacheable. + */ + @ParameterizedTest(name = "deleteCacheableStoreBumpsMetaDataVersionStamp [{0}]") + @EnumSource(DeleteStoreMode.class) + void deleteCacheableStoreBumpsMetaDataVersionStamp(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { + ensureMetaDataVersionStampInitialized(); + + final Subspace subspace = createStore(true).subspace(); + // Commit above already bumped the stamp (transition to cacheable). Snapshot after that. + final byte[] beforeStamp = getMetaDataVersionStamp(); + assertNotNull(beforeStamp); + + deleteStore(subspace, deleteStoreMode); + + final byte[] afterStamp = getMetaDataVersionStamp(); + assertNotNull(afterStamp); + assertFalse(Arrays.equals(beforeStamp, afterStamp), + "deleting a cacheable store should have bumped the meta-data version stamp"); + } + + + @Nonnull + public static Stream concurrentOperationPreventsDeleteStore() { + return ParameterizedTestUtils.cartesianProduct( + Stream.of(ConcurrentOperation.values()), + Stream.of(DeleteStoreMode.values()), + ParameterizedTestUtils.booleans("startCacheable") + ); + } + + /** + * The concurrent operation commits BEFORE the delete. What matters is whether the + * delete's own commit machinery catches the concurrent modification and forces the + * deleter to abort. + * + *

    The key expectatian is that either:

    + *
      + *
    • The delete succeeds, and the range is empty, and if the store was even cacheable, the meta-data + * version-stamp was bumped.
      • + *
    • The delete conflicts.
      • + *
    + */ + @ParameterizedTest + @MethodSource + void concurrentOperationPreventsDeleteStore(@Nonnull ConcurrentOperation op, + @Nonnull DeleteStoreMode deleteStoreMode, + boolean startCacheable) throws Exception { + fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); + ensureMetaDataVersionStampInitialized(); + + final StoreSetup setup = createStore(startCacheable); + + final boolean deleterCommitted; + try (FDBRecordContext deleterContext = fdb.openContext(null, new FDBStoreTimer())) { + // Pin the deleter's read version BEFORE the concurrent op commits. + deleterContext.getReadVersion(); + + // Separate transaction: apply the concurrent operation and commit it. + try (FDBRecordContext opContext = fdb.openContext(null, new FDBStoreTimer())) { + FDBRecordStore opStore = setup.builder().copyBuilder().setContext(opContext).open(); + op.apply(opStore); + commit(opContext); + } + + // Deleter deletes & commits. + deleteStoreMode.deleteStore(deleterContext, setup.subspace()); + deleterCommitted = tryCommitOrDetectConflict(deleterContext); + } + + final boolean expectDeleterConflicts = + deleteStoreMode == DeleteStoreMode.ASYNC && op.writesStoreHeader(); + if (expectDeleterConflicts) { + if (deleterCommitted) { + fail("expected deleteStoreAsync to conflict with a concurrent " + op + + " that wrote STORE_INFO_KEY " + + ", but it committed. The read-conflict range added by the header " + + "read did not fire."); + } + // Op's committed state should still be on disk. + try (FDBRecordContext peek = fdb.openContext(null, new FDBStoreTimer())) { + assertFalse(peek.ensureActive().getRange(setup.subspace().range()).asList().join().isEmpty(), + "op's committed state should still be on disk after the deleter conflicted"); + } + } else { + assertTrue(deleterCommitted, + "expected the deleter to commit for op=" + op + " mode=" + deleteStoreMode + + " startCacheable=" + startCacheable); + // Deleter's clear(subspace.range()) supersedes whatever the op wrote. + try (FDBRecordContext peek = fdb.openContext(null, new FDBStoreTimer())) { + assertTrue(peek.ensureActive().getRange(setup.subspace().range()).asList().join().isEmpty(), + "deleter committed, so the subspace should have been cleared"); + } + } + } + + @Nonnull + public static Stream concurrentOperationConflictsCases() { + return ParameterizedTestUtils.cartesianProduct( + Stream.of(ConcurrentOperation.values()), + Stream.of(DeleteStoreMode.values()), + ParameterizedTestUtils.booleans("startCacheable"), + // for a store that is not cacheable, pre-warming the cache should be a no-op + ParameterizedTestUtils.booleans("cachePreWarmed") + ); + } + + /** + * The delete commits BEFORE the concurrent operation. In all combinations, the operation's own {@code open()} adds + * {@code STORE_INFO_KEY} to its read set. That read-conflict range catches the prior + * delete's write via {@code clear(subspace.range())}, so the operation always aborts + * and the delete always sticks. + */ + @ParameterizedTest + @MethodSource("concurrentOperationConflictsCases") + void concurrentOperationConflictsWithDeleteStore(@Nonnull ConcurrentOperation op, + @Nonnull DeleteStoreMode deleteStoreMode, + boolean startCacheable, + boolean cachePreWarmed) throws Exception { + fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); + ensureMetaDataVersionStampInitialized(); + + final StoreSetup setup = createStore(startCacheable); + + if (cachePreWarmed) { + // Open the store once in a separate transaction so the state cache has an entry + // for this subspace before opContext.open() runs. For a cacheable store this + // makes the subsequent open() take the handleCachedState path; for a + // non-cacheable store the cache doesn't populate and this is effectively a no-op. + try (FDBRecordContext warmup = fdb.openContext(null, new FDBStoreTimer())) { + setup.builder().copyBuilder().setContext(warmup).open(); + } + } + + final boolean opCommitted; + try (FDBRecordContext opContext = fdb.openContext(null, new FDBStoreTimer())) { + // Open the store first — this adds STORE_INFO_KEY to opContext's read set at + // opContext's pinned read version, either via a SERIALIZABLE header load + // (cache miss) or via handleCachedState (cache hit). + FDBRecordStore opStore = setup.builder().copyBuilder().setContext(opContext).open(); + + // Delete and commit in a nested transaction, writing STORE_INFO_KEY (via clear). + try (FDBRecordContext deleterContext = fdb.openContext(null, new FDBStoreTimer())) { + deleteStoreMode.deleteStore(deleterContext, setup.subspace()); + commit(deleterContext); + } + + // Apply the concurrent operation on the already-open opStore and try to commit. + // The read-conflict range on STORE_INFO_KEY from the initial open() overlaps + // the deleter's committed write, so opContext must abort. + op.apply(opStore); + opCommitted = tryCommitOrDetectConflict(opContext); + } + + assertFalse(opCommitted, "op should have conflicted with the preceding deleteStore"); + + // Delete correctly landed. Subspace should be empty; a subsequent create() succeeds. + try (FDBRecordContext contextUsingCache = fdb.openContext(null, new FDBStoreTimer())) { + assertTrue(contextUsingCache.ensureActive() + .getRange(setup.subspace().range()).asList().join().isEmpty(), + "The store should have been deleted"); + recordStore = setup.builder().copyBuilder().setContext(contextUsingCache).create(); + assertNotCacheable(); + } + } + + + @Nonnull + public static Stream testContextAndDeleteStoreModeSource() { + return testContextSource().flatMap(testContext -> + Stream.of(DeleteStoreMode.values()).map(mode -> Arguments.of(testContext, mode))); + } + + + @Nullable + private byte[] getMetaDataVersionStamp() { + try (FDBRecordContext context = fdb.openContext()) { + return context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); + } + } + + private void deleteStore(final Subspace subspace, @Nonnull DeleteStoreMode deleteStoreMode) { + try (FDBRecordContext context = openContext()) { + deleteStoreMode.deleteStore(context, subspace); + commit(context); + } + } + + private void assertNotCacheable() { + assertFalse(isStoreCachable(), "Store state should not be cacheable"); + } + + private boolean isStoreCachable() { + return recordStore.getRecordStoreState().getStoreHeader().getCacheable(); + } + + /** + * Bootstrap that guarantees the cluster-wide meta-data version stamp key exists, so callers + * can compare before/after snapshots without special-casing null. + */ + private void ensureMetaDataVersionStampInitialized() { + try (FDBRecordContext context = fdb.openContext()) { + if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { + context.setMetaDataVersionStamp(); + } + commit(context); + } + } + + private static void assertCacheHit(final FDBStoreTimer timer) { + FDBRecordStoreStateCacheTestUtils.assertCacheHit(timer, 1); + } + + private static void assertCacheMiss(final FDBStoreTimer timer) { + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(timer, 1); + } + + /** + * Small tuple returned by {@link #createStore(boolean)} for tests that need both a + * pre-configured {@link FDBRecordStore.Builder builder} (to rebind to fresh contexts) and + * the {@link Subspace subspace} the store lives in. Both are captured while the setup + * transaction is still open, so callers don't have to open another context just to + * resolve the subspace. + */ + private record StoreSetup(@Nonnull FDBRecordStore.Builder builder, @Nonnull Subspace subspace) { + } + + /** + * Create the standard simple record store, optionally flipped to cacheable, and return + * a re-usable builder plus the store's subspace. Used by the {@code deleteStore} tests + * to share the "open, configure cacheability, commit" boilerplate. + */ + private StoreSetup createStore(boolean cacheable) throws Exception { + try (FDBRecordContext context = openContext()) { + openSimpleRecordStore(context); + if (cacheable) { + assertTrue(recordStore.setStateCacheability(true), + "flipping to cacheable should have changed the header"); + } else { + assertNotCacheable(); + } + final FDBRecordStore.Builder builder = recordStore.asBuilder(); + final Subspace subspace = recordStore.getSubspace(); + commit(context); + return new StoreSetup(builder, subspace); + } + } + + /** + * Attempt to commit the given context. Returns {@code true} on success, {@code false} if + * the commit failed with a conflict (any nested cause). Any other exception is re-raised + * so an unexpected failure mode doesn't silently masquerade as "conflict". + */ + private boolean tryCommitOrDetectConflict(@Nonnull FDBRecordContext context) { + try { + commit(context); + return true; + } catch (Exception ex) { + for (Throwable cause = ex; cause != null; cause = cause.getCause()) { + if (cause instanceof FDBExceptions.FDBStoreTransactionConflictException) { + return false; + } + } + throw new AssertionError("unexpected exception from commit: " + ex, ex); + } + } + + /** + * An operation applied on an open {@link FDBRecordStore} concurrently with a + * {@link FDBRecordStore#deleteStore}/{@link FDBRecordStore#deleteStoreAsync}. The interesting + * axis is whether the operation writes {@code STORE_INFO_KEY} — that is what determines + * whether {@link DeleteStoreMode#ASYNC deleteStoreAsync}'s SERIALIZABLE header read + * catches a concurrent commit and forces the deleter to conflict. + */ + private enum ConcurrentOperation { + /** + * Flips the store's cacheability flag (toggles the current value). Writes + * {@code STORE_INFO_KEY} via {@code updateStoreHeaderAsync} regardless of direction. + */ + SET_CACHEABILITY(true) { + @Override + void apply(@Nonnull FDBRecordStore store) { + final boolean currentlyCacheable = store.getRecordStoreState().getStoreHeader().getCacheable(); + assertTrue(store.setStateCacheability(!currentlyCacheable), + "flipping cacheability should have changed the header"); + } + }, + /** + * Sets a header user field. Writes {@code STORE_INFO_KEY} via + * {@code updateStoreHeaderAsync}, but does not touch the cacheable flag — a + * representative "other part of the header" mutation. + */ + SET_HEADER_USER_FIELD(true) { + @Override + void apply(@Nonnull FDBRecordStore store) { + store.setHeaderUserField("concurrent-op", new byte[]{1, 2, 3}); + } + }, + /** + * Saves a record. Writes only inside the records subspace — {@code STORE_INFO_KEY} + * is untouched, so a concurrent {@code deleteStoreAsync} does NOT observe any + * write in its read set. + */ + SAVE_RECORD(false) { + @Override + void apply(@Nonnull FDBRecordStore store) { + store.saveRecord(TestRecords1Proto.MySimpleRecord.newBuilder() + .setRecNo(1L) + .setStrValueIndexed("racy") + .build()); + } + }; + + private final boolean writesStoreHeader; + + ConcurrentOperation(boolean writesStoreHeader) { + this.writesStoreHeader = writesStoreHeader; + } + + /** + * Whether or not this operation writes to the the store header. + * @return {@code true} iff the operation writes {@code STORE_INFO_KEY} on commit. + * The write-set of the concurrent operation is what {@code deleteStoreAsync}'s + * header read conflicts with; operations that never touch {@code STORE_INFO_KEY} + * cannot cause the deleter to abort. + */ + boolean writesStoreHeader() { + return writesStoreHeader; + } + + abstract void apply(@Nonnull FDBRecordStore store); + } + +} diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java index 4ae3f2c01a..8b70aff5f0 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java @@ -29,7 +29,6 @@ import com.apple.foundationdb.record.expressions.RecordKeyExpressionProto; import com.apple.foundationdb.record.metadata.Key; import com.apple.foundationdb.record.metadata.expressions.KeyExpression; -import com.apple.foundationdb.record.provider.foundationdb.DeleteStoreMode; import com.apple.foundationdb.record.provider.foundationdb.FDBDatabase; import com.apple.foundationdb.record.provider.foundationdb.FDBDatabaseFactory; import com.apple.foundationdb.record.provider.foundationdb.FDBExceptions; @@ -46,22 +45,16 @@ import com.apple.foundationdb.record.provider.foundationdb.RecordStoreStaleMetaDataVersionException; import com.apple.foundationdb.record.provider.foundationdb.keyspace.KeySpacePath; import com.apple.foundationdb.record.test.FakeClusterFileUtil; -import com.apple.foundationdb.subspace.Subspace; import com.apple.foundationdb.tuple.ByteArrayUtil; import com.apple.foundationdb.tuple.ByteArrayUtil2; -import com.apple.foundationdb.tuple.Tuple; -import com.apple.test.ParameterizedTestUtils; import com.apple.test.Tags; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.parallel.Isolated; import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.Arguments; -import org.junit.jupiter.params.provider.EnumSource; import org.junit.jupiter.params.provider.MethodSource; import javax.annotation.Nonnull; -import javax.annotation.Nullable; import java.util.Arrays; import java.util.Collections; import java.util.UUID; @@ -81,7 +74,6 @@ import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; /** * Tests to make sure that caching {@link FDBRecordStoreStateCacheEntry} objects work. @@ -89,200 +81,14 @@ @Tag(Tags.RequiresFDB) @Isolated // Needs to be run in isolation because key space path deletion updates the special meta-data versionstamp key, which is read within these tests public class FDBRecordStoreStateCacheTest extends FDBRecordStoreTestBase { - @Nonnull - private static final ReadVersionRecordStoreStateCacheFactory readVersionCacheFactory = ReadVersionRecordStoreStateCacheFactory.newInstance(); - @Nonnull - private static final MetaDataVersionStampStoreStateCacheFactory metaDataVersionStampCacheFactory = MetaDataVersionStampStoreStateCacheFactory.newInstance(); @Nonnull public static Stream factorySource() { - return Stream.of(readVersionCacheFactory, metaDataVersionStampCacheFactory); - } - - @Nonnull - public static Stream testContextSource() { - return Stream.of(new ReadVersionStateCacheTestContext(), new MetaDataVersionStampStateCacheTestContext()); - } - - @Nonnull - public static Stream testContextAndDeleteStoreModeSource() { - return testContextSource().flatMap(testContext -> - Stream.of(DeleteStoreMode.values()).map(mode -> Arguments.of(testContext, mode))); - } - - /** - * An operation applied on an open {@link FDBRecordStore} concurrently with a - * {@link FDBRecordStore#deleteStore}/{@link FDBRecordStore#deleteStoreAsync}. The interesting - * axis is whether the operation writes {@code STORE_INFO_KEY} — that is what determines - * whether {@link DeleteStoreMode#ASYNC deleteStoreAsync}'s SERIALIZABLE header read - * catches a concurrent commit and forces the deleter to conflict. - */ - private enum ConcurrentOperation { - /** - * Flips the store's cacheability flag (toggles the current value). Writes - * {@code STORE_INFO_KEY} via {@code updateStoreHeaderAsync} regardless of direction. - */ - SET_CACHEABILITY(true) { - @Override - void apply(@Nonnull FDBRecordStore store) { - final boolean currentlyCacheable = store.getRecordStoreState().getStoreHeader().getCacheable(); - assertTrue(store.setStateCacheability(!currentlyCacheable), - "flipping cacheability should have changed the header"); - } - }, - /** - * Sets a header user field. Writes {@code STORE_INFO_KEY} via - * {@code updateStoreHeaderAsync}, but does not touch the cacheable flag — a - * representative "other part of the header" mutation. - */ - SET_HEADER_USER_FIELD(true) { - @Override - void apply(@Nonnull FDBRecordStore store) { - store.setHeaderUserField("concurrent-op", new byte[]{1, 2, 3}); - } - }, - /** - * Saves a record. Writes only inside the records subspace — {@code STORE_INFO_KEY} - * is untouched, so a concurrent {@code deleteStoreAsync} does NOT observe any - * write in its read set. - */ - SAVE_RECORD(false) { - @Override - void apply(@Nonnull FDBRecordStore store) { - store.saveRecord(TestRecords1Proto.MySimpleRecord.newBuilder() - .setRecNo(1L) - .setStrValueIndexed("racy") - .build()); - } - }; - - private final boolean writesStoreHeader; - - ConcurrentOperation(boolean writesStoreHeader) { - this.writesStoreHeader = writesStoreHeader; - } - - /** - * Whether or not this operation writes to the the store header. - * @return {@code true} iff the operation writes {@code STORE_INFO_KEY} on commit. - * The write-set of the concurrent operation is what {@code deleteStoreAsync}'s - * header read conflicts with; operations that never touch {@code STORE_INFO_KEY} - * cannot cause the deleter to abort. - */ - boolean writesStoreHeader() { - return writesStoreHeader; - } - - abstract void apply(@Nonnull FDBRecordStore store); - } - - /** - * A wrapper interface for dealing with the differences between the different {@link FDBRecordStoreStateCache} - * implementations. - */ - public interface StateCacheTestContext { - @Nonnull - FDBRecordStoreStateCache getCache(@Nonnull FDBDatabase database); - - @Nonnull - default FDBRecordContext getCachedContext(@Nonnull FDBDatabase fdb, @Nonnull FDBRecordStore.Builder storeBuilder) { - return getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NO_INFO_AND_NOT_EMPTY); - } - - @Nonnull - FDBRecordContext getCachedContext(@Nonnull FDBDatabase fdb, @Nonnull FDBRecordStore.Builder storeBuilder, - @Nonnull FDBRecordStoreBase.StoreExistenceCheck existenceCheck); - - void invalidateCache(@Nonnull FDBDatabase fdb); - } - - /** - * An implementation of the {@link StateCacheTestContext} that handles caching by read version. - */ - public static class ReadVersionStateCacheTestContext implements StateCacheTestContext { - @Nonnull - @Override - public FDBRecordStoreStateCache getCache(@Nonnull FDBDatabase database) { - return readVersionCacheFactory.getCache(database); - } - - @Nonnull - @Override - public FDBRecordContext getCachedContext(@Nonnull FDBDatabase fdb, @Nonnull FDBRecordStore.Builder storeBuilder, - @Nonnull FDBRecordStoreBase.StoreExistenceCheck existenceCheck) { - long readVersion; - try (FDBRecordContext context = fdb.openContext()) { - storeBuilder.copyBuilder().setContext(context).createOrOpen(existenceCheck); - readVersion = context.getReadVersion(); - } - FDBRecordContext context = fdb.openContext(null, new FDBStoreTimer()); - context.setReadVersion(readVersion); - return context; - } - - @Override - public void invalidateCache(@Nonnull FDBDatabase fdb) { - // Ensure that the next read version includes at least one new commit. - try (FDBRecordContext context = fdb.openContext()) { - context.ensureActive().addWriteConflictKey(Tuple.from(UUID.randomUUID()).pack()); - context.commit(); - } - } - - @Override - public String toString() { - return "ReadVersionStateCacheTestContext"; - } + return Stream.of(FDBRecordStoreStateCacheTestUtils.readVersionCacheFactory, FDBRecordStoreStateCacheTestUtils.metaDataVersionStampCacheFactory); } - /** - * An implementation of the {@link StateCacheTestContext} that handles caching by the meta-data version-stamp. - */ - public static class MetaDataVersionStampStateCacheTestContext implements StateCacheTestContext { - - @Nonnull - @Override - public FDBRecordStoreStateCache getCache(@Nonnull FDBDatabase database) { - return metaDataVersionStampCacheFactory.getCache(database); - } - - @Nonnull - @Override - public FDBRecordContext getCachedContext(@Nonnull FDBDatabase fdb, @Nonnull FDBRecordStore.Builder storeBuilder, - @Nonnull FDBRecordStoreBase.StoreExistenceCheck existenceCheck) { - boolean cacheable = true; - try (FDBRecordContext context = fdb.openContext()) { - FDBRecordStore store = storeBuilder.copyBuilder().setContext(context).createOrOpen(existenceCheck); - if (!store.getRecordStoreState().getStoreHeader().getCacheable()) { - cacheable = false; - assertTrue(store.setStateCacheability(true)); - context.commit(); - } - } - if (!cacheable) { - try (FDBRecordContext context = fdb.openContext()) { - storeBuilder.copyBuilder().setContext(context).createOrOpen(existenceCheck); - context.commit(); - } - } - FDBRecordContext context = fdb.openContext(null, new FDBStoreTimer()); - context.getMetaDataVersionStampAsync(IsolationLevel.SNAPSHOT).join(); - return context; - } - - @Override - public void invalidateCache(@Nonnull FDBDatabase fdb) { - // Ensure that the next read version includes at least one new commit. - try (FDBRecordContext context = fdb.openContext()) { - context.setMetaDataVersionStamp(); - context.commit(); - } - } - - @Override - public String toString() { - return "MetaDataVersionStampStateCacheTestContext"; - } + public static Stream testContextSource() { + return FDBRecordStoreStateCacheTestUtils.testContextSource(); } /** @@ -290,14 +96,14 @@ public String toString() { */ @Test public void cacheByReadVersion() throws Exception { - fdb.setStoreStateCache(readVersionCacheFactory.getCache(fdb)); + fdb.setStoreStateCache(FDBRecordStoreStateCacheTestUtils.readVersionCacheFactory.getCache(fdb)); long readVersion; int metaDataVersion; // Open a record store but do not commit to make sure that the updated value is not cached try (FDBRecordContext context = openContext()) { openSimpleRecordStore(context); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); assertTrue(context.hasDirtyStoreState()); readVersion = context.getReadVersion(); metaDataVersion = recordStore.getRecordMetaData().getVersion(); @@ -311,7 +117,7 @@ public void cacheByReadVersion() throws Exception { openSimpleRecordStore(context); // For this specific case, we hit the cache, but then we need to validate that the store is empty // in order to match its null store header. - assertCacheMiss(context.getTimer(), 0); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 0); assertTrue(context.hasDirtyStoreState()); assertEquals(metaDataVersion, recordStore.getRecordMetaData().getVersion()); commit(context); // commit so a stable value is put into the database @@ -320,7 +126,7 @@ public void cacheByReadVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); assertFalse(context.hasDirtyStoreState()); assertEquals(metaDataVersion, recordStore.getRecordMetaData().getVersion()); readVersion = context.getReadVersion(); @@ -331,7 +137,7 @@ public void cacheByReadVersion() throws Exception { context.setReadVersion(readVersion); openSimpleRecordStore(context); assertFalse(context.hasDirtyStoreState()); - assertCacheHit(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(context.getTimer(), 1); assertEquals(metaDataVersion, recordStore.getRecordMetaData().getVersion()); } @@ -341,7 +147,7 @@ public void cacheByReadVersion() throws Exception { context.setReadVersion(readVersion); openSimpleRecordStore(context); assertFalse(context.hasDirtyStoreState()); - assertCacheHit(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(context.getTimer(), 1); recordStore.markIndexWriteOnly("MySimpleRecord$str_value_indexed").get(); assertTrue(context.hasDirtyStoreState()); assertFalse(recordStore.isIndexReadable("MySimpleRecord$str_value_indexed")); @@ -349,7 +155,7 @@ public void cacheByReadVersion() throws Exception { // Reopen the store with the same context and ensure the index is still not readable openSimpleRecordStore(context); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); assertNotSame(initialRecordStore, recordStore); assertNotSame(initialRecordStore.getRecordStoreState(), recordStore.getRecordStoreState()); assertFalse(recordStore.isIndexReadable("MySimpleRecord$str_value_indexed")); @@ -362,7 +168,7 @@ public void cacheByReadVersion() throws Exception { context.getTimer().reset(); context.setReadVersion(readVersion); openSimpleRecordStore(context); - assertCacheHit(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(context.getTimer(), 1); assertTrue(recordStore.isIndexReadable("MySimpleRecord$str_value_indexed")); // Add a random write-conflict range to ensure conflicts are actually checked @@ -376,7 +182,7 @@ public void cacheByReadVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); long newReadVersion = context.getReadVersion(); assertThat(newReadVersion, greaterThan(readVersion)); readVersion = newReadVersion; @@ -387,7 +193,7 @@ public void cacheByReadVersion() throws Exception { context.getTimer().reset(); context.setReadVersion(readVersion); openSimpleRecordStore(context); - assertCacheHit(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(context.getTimer(), 1); assertFalse(recordStore.isIndexReadable("MySimpleRecord$str_value_indexed")); } } @@ -397,7 +203,7 @@ public void cacheByReadVersion() throws Exception { */ @Test public void cacheByMetaDataVersion() throws Exception { - fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); + fdb.setStoreStateCache(FDBRecordStoreStateCacheTestUtils.metaDataVersionStampCacheFactory.getCache(fdb)); byte[] metaDataVersionStamp; // Open the store; save the meta-data @@ -411,7 +217,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); metaDataVersionStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); assertNotNull(metaDataVersionStamp); } @@ -419,7 +225,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); assertArrayEquals(metaDataVersionStamp, context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT)); recordStore.markIndexWriteOnly("MySimpleRecord$str_value_indexed").get(); assertTrue(context.hasDirtyStoreState()); @@ -431,7 +237,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); assertArrayEquals(metaDataVersionStamp, context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT)); assertTrue(recordStore.isIndexWriteOnly("MySimpleRecord$str_value_indexed")); commit(context); @@ -441,7 +247,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); assertTrue(recordStore.setStateCacheability(true)); assertTrue(context.hasDirtyStoreState()); assertArrayEquals(metaDataVersionStamp, context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT)); @@ -453,7 +259,7 @@ public void cacheByMetaDataVersion() throws Exception { context.getTimer().reset(); openSimpleRecordStore(context); assertArrayEquals(metaDataVersionStamp, context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT)); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); assertTrue(recordStore.isIndexWriteOnly("MySimpleRecord$str_value_indexed")); // don't need to commit } @@ -462,7 +268,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertCacheHit(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(context.getTimer(), 1); assertTrue(recordStore.isIndexWriteOnly("MySimpleRecord$str_value_indexed")); recordStore.markIndexReadable("MySimpleRecord$str_value_indexed").get(); assertTrue(context.hasDirtyStoreState()); @@ -474,7 +280,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); assertTrue(recordStore.isIndexReadable("MySimpleRecord$str_value_indexed")); byte[] trMetaDataVersionStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); assertNotNull(trMetaDataVersionStamp); @@ -486,7 +292,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertCacheHit(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(context.getTimer(), 1); assertTrue(recordStore.isIndexReadable("MySimpleRecord$str_value_indexed")); assertArrayEquals(metaDataVersionStamp, context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT)); } @@ -497,7 +303,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertCacheHit(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(context.getTimer(), 1); readVersion = context.getReadVersion(); assertTrue(recordStore.setStateCacheability(false)); assertTrue(context.hasDirtyStoreState()); @@ -512,7 +318,7 @@ public void cacheByMetaDataVersion() throws Exception { context.getTimer().reset(); context.setReadVersion(readVersion); openSimpleRecordStore(context); - assertCacheHit(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(context.getTimer(), 1); assertTrue(recordStore.getRecordStoreState().getStoreHeader().getCacheable()); } @@ -520,7 +326,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); byte[] trMetaDataVersionStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); assertNotNull(trMetaDataVersionStamp); assertThat(ByteArrayUtil.compareUnsigned(metaDataVersionStamp, trMetaDataVersionStamp), lessThan(0)); @@ -531,7 +337,7 @@ public void cacheByMetaDataVersion() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); byte[] trMetaDataVersionStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); assertNotNull(trMetaDataVersionStamp); assertArrayEquals(metaDataVersionStamp, trMetaDataVersionStamp); @@ -540,7 +346,7 @@ public void cacheByMetaDataVersion() throws Exception { @Test public void cacheByMetaDataVersionFirstTimeEver() throws Exception { - fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); + fdb.setStoreStateCache(FDBRecordStoreStateCacheTestUtils.metaDataVersionStampCacheFactory.getCache(fdb)); // Clear out the meta-data version-stamp key try (FDBRecordContext context = openContext()) { @@ -555,7 +361,7 @@ public void cacheByMetaDataVersionFirstTimeEver() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); assertNull(context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT)); recordStore.setStateCacheability(true); @@ -570,14 +376,14 @@ public void cacheByMetaDataVersionFirstTimeEver() throws Exception { try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); assertArrayEquals(commitVersionStamp, context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT)); } try (FDBRecordContext context = openContext()) { context.getTimer().reset(); openSimpleRecordStore(context); - assertCacheHit(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(context.getTimer(), 1); assertArrayEquals(commitVersionStamp, context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT)); } } @@ -588,7 +394,7 @@ public void cacheByMetaDataVersionFirstTimeEver() throws Exception { */ @ParameterizedTest(name = "conflictWhenCachedChanged (test context = {0})") @MethodSource("testContextSource") - public void conflictWhenCachedChanged(@Nonnull StateCacheTestContext testContext) { + public void conflictWhenCachedChanged(@Nonnull FDBRecordStoreStateCacheTestUtils.StateCacheTestContext testContext) { fdb.setStoreStateCache(testContext.getCache(fdb)); RecordMetaData metaData1 = RecordMetaData.build(TestRecords1Proto.getDescriptor()); @@ -608,7 +414,7 @@ public void conflictWhenCachedChanged(@Nonnull StateCacheTestContext testContext .setMetaDataProvider(metaData1) .setKeySpacePath(path) .create(); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); assertEquals(metaData1.getVersion(), recordStore.getRecordStoreState().getStoreHeader().getMetaDataversion()); commit(context); @@ -621,7 +427,7 @@ public void conflictWhenCachedChanged(@Nonnull StateCacheTestContext testContext .setContext(context1) .setMetaDataProvider(metaData1) .open(); - assertCacheHit(context1.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(context1.getTimer(), 1); assertEquals(metaData1.getVersion(), recordStore1.getRecordMetaData().getVersion()); assertEquals(metaData1.getVersion(), recordStore1.getRecordStoreState().getStoreHeader().getMetaDataversion()); @@ -630,7 +436,7 @@ public void conflictWhenCachedChanged(@Nonnull StateCacheTestContext testContext .setContext(context2) .setMetaDataProvider(metaData2) .open(); - assertCacheHit(context2.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(context2.getTimer(), 1); assertEquals(Collections.singletonList(recordStore2.getRecordMetaData().getRecordType("MySimpleRecord")), recordStore2.getRecordMetaData().recordTypesForIndex(recordStore2.getRecordMetaData().getIndex("MySimpleRecord$num_value_2"))); assertEquals(metaData2.getVersion(), recordStore2.getRecordMetaData().getVersion()); @@ -656,14 +462,14 @@ public void conflictWhenCachedChanged(@Nonnull StateCacheTestContext testContext .setContext(context) .setMetaDataProvider(metaData1) .open()); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); // Trying to load with the new meta-data should succeed FDBRecordStore recordStore = storeBuilder.copyBuilder() .setContext(context) .setMetaDataProvider(metaData2) .open(); - assertCacheHit(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(context.getTimer(), 1); assertEquals(metaData2.getVersion(), recordStore.getRecordStoreState().getStoreHeader().getMetaDataversion()); } } @@ -673,14 +479,14 @@ public void conflictWhenCachedChanged(@Nonnull StateCacheTestContext testContext */ @ParameterizedTest(name = "existenceCheckOnCachedStoreStates (test context = {0})") @MethodSource("testContextSource") - public void existenceCheckOnCachedStoreStates(@Nonnull StateCacheTestContext testContext) throws Exception { + public void existenceCheckOnCachedStoreStates(@Nonnull FDBRecordStoreStateCacheTestUtils.StateCacheTestContext testContext) throws Exception { fdb.setStoreStateCache(testContext.getCache(fdb)); // Create a record store FDBRecordStore.Builder storeBuilder; try (FDBRecordContext context = openContext()) { openSimpleRecordStore(context); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); assertTrue(context.hasDirtyStoreState()); // Save a record so that when the store header is deleted, it won't be an empty record store recordStore.saveRecord(TestRecords1Proto.MySimpleRecord.newBuilder() @@ -696,7 +502,7 @@ public void existenceCheckOnCachedStoreStates(@Nonnull StateCacheTestContext tes assertThrows(RecordStoreAlreadyExistsException.class, storeBuilder::create); context.getTimer().reset(); FDBRecordStore store = storeBuilder.open(); - assertCacheHit(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(context.getTimer(), 1); // Delete the store header storeInfoKey = store.getSubspace().pack(FDBRecordStoreKeyspace.STORE_INFO.key()); @@ -714,383 +520,12 @@ public void existenceCheckOnCachedStoreStates(@Nonnull StateCacheTestContext tes } } - /** - * Validate that deleting a record store causes the record store to go back to the database as it's possible the - * cached stuff is what was deleted. - */ - @ParameterizedTest(name = "storeDeletionInSameContext (test context = {0}, deleteMode = {1})") - @MethodSource("testContextAndDeleteStoreModeSource") - public void storeDeletionInSameContext(@Nonnull StateCacheTestContext testContext, - @Nonnull DeleteStoreMode deleteStoreMode) throws Exception { - fdb.setStoreStateCache(testContext.getCache(fdb)); - - FDBRecordStore.Builder storeBuilder; - try (FDBRecordContext context = openContext()) { - openSimpleRecordStore(context); - storeBuilder = recordStore.asBuilder(); - commit(context); - } - - try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder)) { - openSimpleRecordStore(context); - assertCacheHit(context.getTimer(), 1); - - context.getTimer().reset(); - deleteStoreMode.deleteStore(context, recordStore.getSubspace()); - recordStore.asBuilder().create(); - assertCacheMiss(context.getTimer(), 1); - - commit(context); - } - - try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder)) { - openSimpleRecordStore(context); - assertCacheHit(context.getTimer(), 1); - path.deleteAllData(context); - - context.getTimer().reset(); - recordStore.asBuilder().create(); - assertCacheMiss(context.getTimer(), 1); - } - - // Deleting all records should not disable the index, so the result should still be cacheable. - // See: https://github.com/FoundationDB/fdb-record-layer/issues/399 - final String disabledIndex = "MySimpleRecord$str_value_indexed"; - try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { - openSimpleRecordStore(context); - assertCacheHit(context.getTimer(), 1); - recordStore.markIndexDisabled(disabledIndex).get(); - commit(context); - } - - try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { - openSimpleRecordStore(context); - assertCacheHit(context.getTimer(), 1); - assertTrue(recordStore.isIndexDisabled(disabledIndex)); - recordStore.deleteAllRecords(); - - context.getTimer().reset(); - recordStore = recordStore.asBuilder().open(); - assertCacheHit(context.getTimer(), 1); - assertTrue(recordStore.isIndexDisabled(disabledIndex)); - commit(context); - } - } - - /** - * After a store is deleted, validate that future transactions need to reload it from cache. - */ - @ParameterizedTest(name = "storeDeletionAcrossContexts (test context = {0}, deleteMode = {1})") - @MethodSource("testContextAndDeleteStoreModeSource") - public void storeDeletionAcrossContexts(@Nonnull StateCacheTestContext testContext, - @Nonnull DeleteStoreMode deleteStoreMode) throws Exception { - fdb.setStoreStateCache(testContext.getCache(fdb)); - - FDBRecordStore.Builder storeBuilder; - try (FDBRecordContext context = openContext()) { - openSimpleRecordStore(context); - assertTrue(recordStore.setStateCacheability(true)); - storeBuilder = recordStore.asBuilder(); - commit(context); - } - - // Delete by calling deleteStore. - try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { - openSimpleRecordStore(context); - assertCacheHit(context.getTimer(), 1); - deleteStoreMode.deleteStore(context, recordStore.getSubspace()); - commit(context); - } - - // After deleting it, when opening the same store again, it shouldn't be cached. - try (FDBRecordContext context = fdb.openContext(null, new FDBStoreTimer())) { - FDBRecordStore store = storeBuilder.setContext(context).create(); - assertCacheMiss(context.getTimer(), 1); - assertTrue(store.setStateCacheability(true)); - commit(context); - } - - // Delete by calling path.deleteAllData - try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { - openSimpleRecordStore(context); - assertCacheHit(context.getTimer(), 1); - path.deleteAllData(context); - commit(context); - } - - try (FDBRecordContext context = fdb.openContext(null, new FDBStoreTimer())) { - FDBRecordStore store = storeBuilder.setContext(context).create(); - store.setStateCacheabilityAsync(true).get(); - assertCacheMiss(context.getTimer(), 1); - commit(context); - } - - // Deleting all records should not disable the index state. - final String disabledIndex = "MySimpleRecord$str_value_indexed"; - try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { - openSimpleRecordStore(context); - recordStore.markIndexDisabled(disabledIndex).get(); - commit(context); - } - - try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { - openSimpleRecordStore(context); - assertTrue(recordStore.isIndexDisabled(disabledIndex)); - recordStore.deleteAllRecords(); - commit(context); - } - - try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { - openSimpleRecordStore(context); - assertTrue(recordStore.isIndexDisabled(disabledIndex)); - commit(context); - } - } - - /** - * Behavior of {@link FDBRecordStore#deleteStore}/{@link FDBRecordStore#deleteStoreAsync} on a non-cacheable store, - * across both modes. - * - *
      - *
    • {@link DeleteStoreMode#ASYNC ASYNC} (new): reads the header, sees non-cacheable, and skips the meta-data - * version-stamp bump. No other client can hold a cached copy of a non-cacheable header, so bumping the - * cluster-wide bottleneck on the {@code \xff/metadataVersion} key would be pure waste. This is key to - * reducing cache invalidations and conflicts if a cluster is using the meta-data version-stamp, and is - * frequently deleting non-cacheable stores. For, example, if you have the relational catalog, and are - * frequently creating/deleting unrelated schemas, they would have conflicted with each other, because the - * catalog is a store with a cached header.
      • - *
    • {@link DeleteStoreMode#SYNC SYNC} (deprecated): bumps the meta-data version stamp unconditionally without - * reading the header — the pre-{@code deleteStoreAsync} behavior. Documented here so the deprecated method's - * cost is not silently forgotten.
      • - *
    - */ - @ParameterizedTest(name = "deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp [{0}]") - @EnumSource(DeleteStoreMode.class) - void deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { - ensureMetaDataVersionStampInitialized(); - assertMetaDataVersionStampBehaviorOnNonCacheableOrMissingStore(createStore(false).subspace(), deleteStoreMode); - } - - /** - * Behavior of {@link FDBRecordStore#deleteStore}/{@link FDBRecordStore#deleteStoreAsync} on an empty subspace with - * no store header, mirroring the {@link #deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp} - * split: {@link DeleteStoreMode#ASYNC ASYNC} sees the missing header and skips the bump; - * {@link DeleteStoreMode#SYNC SYNC} bumps unconditionally. - */ - @ParameterizedTest(name = "deleteMissingStoreDoesNotBumpMetaDataVersionStamp [{0}]") - @EnumSource(DeleteStoreMode.class) - void deleteMissingStoreDoesNotBumpMetaDataVersionStamp(@Nonnull DeleteStoreMode deleteStoreMode) { - ensureMetaDataVersionStampInitialized(); - - // Use the test's per-instance path, but never open a store there. - final Subspace subspace; - try (FDBRecordContext context = openContext()) { - subspace = path.toSubspace(context); - } - assertMetaDataVersionStampBehaviorOnNonCacheableOrMissingStore(subspace, deleteStoreMode); - } - - /** - * Snapshot the meta-data version stamp, run a delete of the given subspace via {@code mode}, - * and assert the stamp behaved as the mode's contract requires: {@link DeleteStoreMode#SYNC} - * bumps unconditionally, {@link DeleteStoreMode#ASYNC} leaves the stamp untouched when the - * header is absent or marks the store non-cacheable. - */ - private void assertMetaDataVersionStampBehaviorOnNonCacheableOrMissingStore(final Subspace subspace, - @Nonnull DeleteStoreMode deleteStoreMode) { - final byte[] beforeStamp = getMetaDataVersionStamp(); - assertNotNull(beforeStamp); - - deleteStore(subspace, deleteStoreMode); - - final byte[] afterStamp = getMetaDataVersionStamp(); - assertNotNull(afterStamp); - if (deleteStoreMode == DeleteStoreMode.SYNC) { - assertFalse(Arrays.equals(beforeStamp, afterStamp), - "deprecated sync deleteStore always bumps the meta-data version stamp, " + - "even when the store header is absent or non-cacheable"); - } else { - assertArrayEquals(beforeStamp, afterStamp, - "async deleteStoreAsync must not bump the meta-data version stamp when the " + - "store header is absent or non-cacheable — nothing to invalidate"); - } - } - - /** - * Complement of {@link #deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp(DeleteStoreMode)}: - * deleting a cacheable store MUST bump the stamp — otherwise sibling clients could keep serving - * reads out of a stale cached header long after the store is gone. Both modes bump: SYNC does - * so unconditionally, ASYNC does so because the header parses as cacheable. - */ - @ParameterizedTest(name = "deleteCacheableStoreBumpsMetaDataVersionStamp [{0}]") - @EnumSource(DeleteStoreMode.class) - void deleteCacheableStoreBumpsMetaDataVersionStamp(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { - ensureMetaDataVersionStampInitialized(); - - final Subspace subspace = createStore(true).subspace(); - // Commit above already bumped the stamp (transition to cacheable). Snapshot after that. - final byte[] beforeStamp = getMetaDataVersionStamp(); - assertNotNull(beforeStamp); - - deleteStore(subspace, deleteStoreMode); - - final byte[] afterStamp = getMetaDataVersionStamp(); - assertNotNull(afterStamp); - assertFalse(Arrays.equals(beforeStamp, afterStamp), - "deleting a cacheable store should have bumped the meta-data version stamp"); - } - - - @Nonnull - public static Stream concurrentOperationPreventsDeleteStore() { - return ParameterizedTestUtils.cartesianProduct( - Stream.of(ConcurrentOperation.values()), - Stream.of(DeleteStoreMode.values()), - ParameterizedTestUtils.booleans("startCacheable") - ); - } - - /** - * The concurrent operation commits BEFORE the delete. What matters is whether the - * delete's own commit machinery catches the concurrent modification and forces the - * deleter to abort. - * - *

    The key expectatian is that either:

    - *
      - *
    • The delete succeeds, and the range is empty, and if the store was even cacheable, the meta-data - * version-stamp was bumped.
      • - *
    • The delete conflicts.
      • - *
    - */ - @ParameterizedTest - @MethodSource - void concurrentOperationPreventsDeleteStore(@Nonnull ConcurrentOperation op, - @Nonnull DeleteStoreMode deleteStoreMode, - boolean startCacheable) throws Exception { - fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); - ensureMetaDataVersionStampInitialized(); - - final StoreSetup setup = createStore(startCacheable); - - final boolean deleterCommitted; - try (FDBRecordContext deleterContext = fdb.openContext(null, new FDBStoreTimer())) { - // Pin the deleter's read version BEFORE the concurrent op commits. - deleterContext.getReadVersion(); - - // Separate transaction: apply the concurrent operation and commit it. - try (FDBRecordContext opContext = fdb.openContext(null, new FDBStoreTimer())) { - FDBRecordStore opStore = setup.builder().copyBuilder().setContext(opContext).open(); - op.apply(opStore); - commit(opContext); - } - - // Deleter deletes & commits. - deleteStoreMode.deleteStore(deleterContext, setup.subspace()); - deleterCommitted = tryCommitOrDetectConflict(deleterContext); - } - - final boolean expectDeleterConflicts = - deleteStoreMode == DeleteStoreMode.ASYNC && op.writesStoreHeader(); - if (expectDeleterConflicts) { - if (deleterCommitted) { - fail("expected deleteStoreAsync to conflict with a concurrent " + op - + " that wrote STORE_INFO_KEY " - + ", but it committed. The read-conflict range added by the header " - + "read did not fire."); - } - // Op's committed state should still be on disk. - try (FDBRecordContext peek = fdb.openContext(null, new FDBStoreTimer())) { - assertFalse(peek.ensureActive().getRange(setup.subspace().range()).asList().join().isEmpty(), - "op's committed state should still be on disk after the deleter conflicted"); - } - } else { - assertTrue(deleterCommitted, - "expected the deleter to commit for op=" + op + " mode=" + deleteStoreMode - + " startCacheable=" + startCacheable); - // Deleter's clear(subspace.range()) supersedes whatever the op wrote. - try (FDBRecordContext peek = fdb.openContext(null, new FDBStoreTimer())) { - assertTrue(peek.ensureActive().getRange(setup.subspace().range()).asList().join().isEmpty(), - "deleter committed, so the subspace should have been cleared"); - } - } - } - - @Nonnull - public static Stream concurrentOperationConflictsCases() { - return ParameterizedTestUtils.cartesianProduct( - Stream.of(ConcurrentOperation.values()), - Stream.of(DeleteStoreMode.values()), - ParameterizedTestUtils.booleans("startCacheable"), - // for a store that is not cacheable, pre-warming the cache should be a no-op - ParameterizedTestUtils.booleans("cachePreWarmed") - ); - } - - /** - * The delete commits BEFORE the concurrent operation. In all combinations, the operation's own {@code open()} adds - * {@code STORE_INFO_KEY} to its read set. That read-conflict range catches the prior - * delete's write via {@code clear(subspace.range())}, so the operation always aborts - * and the delete always sticks. - */ - @ParameterizedTest - @MethodSource("concurrentOperationConflictsCases") - void concurrentOperationConflictsWithDeleteStore(@Nonnull ConcurrentOperation op, - @Nonnull DeleteStoreMode deleteStoreMode, - boolean startCacheable, - boolean cachePreWarmed) throws Exception { - fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); - ensureMetaDataVersionStampInitialized(); - - final StoreSetup setup = createStore(startCacheable); - - if (cachePreWarmed) { - // Open the store once in a separate transaction so the state cache has an entry - // for this subspace before opContext.open() runs. For a cacheable store this - // makes the subsequent open() take the handleCachedState path; for a - // non-cacheable store the cache doesn't populate and this is effectively a no-op. - try (FDBRecordContext warmup = fdb.openContext(null, new FDBStoreTimer())) { - setup.builder().copyBuilder().setContext(warmup).open(); - } - } - - final boolean opCommitted; - try (FDBRecordContext opContext = fdb.openContext(null, new FDBStoreTimer())) { - // Open the store first — this adds STORE_INFO_KEY to opContext's read set at - // opContext's pinned read version, either via a SERIALIZABLE header load - // (cache miss) or via handleCachedState (cache hit). - FDBRecordStore opStore = setup.builder().copyBuilder().setContext(opContext).open(); - - // Delete and commit in a nested transaction, writing STORE_INFO_KEY (via clear). - try (FDBRecordContext deleterContext = fdb.openContext(null, new FDBStoreTimer())) { - deleteStoreMode.deleteStore(deleterContext, setup.subspace()); - commit(deleterContext); - } - - // Apply the concurrent operation on the already-open opStore and try to commit. - // The read-conflict range on STORE_INFO_KEY from the initial open() overlaps - // the deleter's committed write, so opContext must abort. - op.apply(opStore); - opCommitted = tryCommitOrDetectConflict(opContext); - } - - assertFalse(opCommitted, "op should have conflicted with the preceding deleteStore"); - - // Delete correctly landed. Subspace should be empty; a subsequent create() succeeds. - try (FDBRecordContext contextUsingCache = fdb.openContext(null, new FDBStoreTimer())) { - assertTrue(contextUsingCache.ensureActive() - .getRange(setup.subspace().range()).asList().join().isEmpty(), - "The store should have been deleted"); - recordStore = setup.builder().copyBuilder().setContext(contextUsingCache).create(); - assertNotCacheable(); - } - } - /** * Verify that updating a header user field will be updated if the store state is cached. */ @ParameterizedTest(name = "cacheUserFields (test context = {0})") @MethodSource("testContextSource") - public void cacheUserFields(@Nonnull StateCacheTestContext testContext) throws Exception { + public void cacheUserFields(@Nonnull FDBRecordStoreStateCacheTestUtils.StateCacheTestContext testContext) throws Exception { fdb.setStoreStateCache(testContext.getCache(fdb)); FDBRecordStore.Builder storeBuilder; @@ -1129,7 +564,7 @@ public void cacheUserFields(@Nonnull StateCacheTestContext testContext) throws E */ @ParameterizedTest(name = "cacheTwoSubspaces (test context = {0})") @MethodSource("testContextSource") - public void cacheTwoSubspaces(@Nonnull StateCacheTestContext testContext) throws Exception { + public void cacheTwoSubspaces(@Nonnull FDBRecordStoreStateCacheTestUtils.StateCacheTestContext testContext) throws Exception { fdb.setStoreStateCache(testContext.getCache(fdb)); final KeySpacePath path1 = pathManager.createPath(); final KeySpacePath path2 = pathManager.createPath(); @@ -1160,11 +595,11 @@ public void cacheTwoSubspaces(@Nonnull StateCacheTestContext testContext) throws long readVersion; try (FDBRecordContext context = testContext.getCachedContext(fdb, storeBuilder1, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NOT_EXISTS)) { FDBRecordStore store1 = storeBuilder1.setContext(context).open(); - assertCacheHit(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(context.getTimer(), 1); assertTrue(store1.isIndexWriteOnly("MySimpleRecord$str_value_indexed")); assertTrue(store1.isIndexReadable("MySimpleRecord$num_value_3_indexed")); FDBRecordStore store2 = storeBuilder2.setContext(context).open(); - assertCacheMiss(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(context.getTimer(), 1); assertTrue(store2.isIndexReadable("MySimpleRecord$str_value_indexed")); assertTrue(store2.isIndexDisabled("MySimpleRecord$num_value_3_indexed")); @@ -1176,11 +611,11 @@ public void cacheTwoSubspaces(@Nonnull StateCacheTestContext testContext) throws context.getTimer().reset(); context.setReadVersion(readVersion); FDBRecordStore store1 = storeBuilder1.setContext(context).open(); - assertCacheHit(context.getTimer(), 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(context.getTimer(), 1); assertTrue(store1.isIndexWriteOnly("MySimpleRecord$str_value_indexed")); assertTrue(store1.isIndexReadable("MySimpleRecord$num_value_3_indexed")); FDBRecordStore store2 = storeBuilder2.setContext(context).open(); - assertCacheHit(context.getTimer(), 2); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(context.getTimer(), 2); assertTrue(store2.isIndexReadable("MySimpleRecord$str_value_indexed")); assertTrue(store2.isIndexDisabled("MySimpleRecord$num_value_3_indexed")); } @@ -1191,7 +626,7 @@ public void cacheTwoSubspaces(@Nonnull StateCacheTestContext testContext) throws */ @ParameterizedTest(name = "cacheWithVersionTracking (test context = {0})") @MethodSource("testContextSource") - public void cacheWithVersionTracking(@Nonnull StateCacheTestContext testContext) throws Exception { + public void cacheWithVersionTracking(@Nonnull FDBRecordStoreStateCacheTestUtils.StateCacheTestContext testContext) throws Exception { fdb.setStoreStateCache(testContext.getCache(fdb)); fdb.setTrackLastSeenVersion(true); FDBStoreTimer timer = new FDBStoreTimer(); @@ -1209,7 +644,7 @@ public void cacheWithVersionTracking(@Nonnull StateCacheTestContext testContext) try (FDBRecordContext context = fdb.openContext(null, timer, readSemantics)) { openSimpleRecordStore(context); recordStore.setStateCacheabilityAsync(true).get(); - assertCacheMiss(timer, 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(timer, 1); recordStore.markIndexDisabled("MySimpleRecord$str_value_indexed").get(); commit(context); commitVersion = context.getCommittedVersion(); @@ -1220,7 +655,7 @@ public void cacheWithVersionTracking(@Nonnull StateCacheTestContext testContext) try (FDBRecordContext context = fdb.openContext(null, timer, readSemantics)) { assertEquals(commitVersion, context.getReadVersion()); openSimpleRecordStore(context); - assertCacheMiss(timer, 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(timer, 1); assertTrue(recordStore.isIndexDisabled("MySimpleRecord$str_value_indexed")); commit(context); // should be read only-so won't change commit version } @@ -1230,7 +665,7 @@ public void cacheWithVersionTracking(@Nonnull StateCacheTestContext testContext) try (FDBRecordContext context = fdb.openContext(null, timer, readSemantics)) { assertEquals(commitVersion, context.getReadVersion()); openSimpleRecordStore(context); - assertCacheHit(timer, 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(timer, 1); assertTrue(recordStore.isIndexDisabled("MySimpleRecord$str_value_indexed")); // Add a dummy write to increase the DB version @@ -1246,10 +681,10 @@ public void cacheWithVersionTracking(@Nonnull StateCacheTestContext testContext) try (FDBRecordContext context = fdb.openContext(null, timer, readSemantics)) { assertEquals(commitVersion, context.getReadVersion()); openSimpleRecordStore(context); - if (testContext instanceof ReadVersionStateCacheTestContext) { - assertCacheMiss(timer, 1); + if (testContext instanceof FDBRecordStoreStateCacheTestUtils.ReadVersionStateCacheTestContext) { + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(timer, 1); } else { - assertCacheHit(timer, 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(timer, 1); } assertTrue(recordStore.isIndexDisabled("MySimpleRecord$str_value_indexed")); @@ -1267,10 +702,10 @@ public void cacheWithVersionTracking(@Nonnull StateCacheTestContext testContext) readVersion = context.getReadVersion(); assertThat(readVersion, greaterThanOrEqualTo(commitVersion)); openSimpleRecordStore(context); - if (testContext instanceof ReadVersionStateCacheTestContext) { - assertCacheMiss(timer, 1); + if (testContext instanceof FDBRecordStoreStateCacheTestUtils.ReadVersionStateCacheTestContext) { + FDBRecordStoreStateCacheTestUtils.assertCacheMiss(timer, 1); } else { - assertCacheHit(timer, 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(timer, 1); } assertTrue(recordStore.isIndexDisabled("MySimpleRecord$str_value_indexed")); } @@ -1280,7 +715,7 @@ public void cacheWithVersionTracking(@Nonnull StateCacheTestContext testContext) try (FDBRecordContext context = fdb.openContext(null, timer, readSemantics)) { assertEquals(readVersion, context.getReadVersion()); openSimpleRecordStore(context); - assertCacheHit(timer, 1); + FDBRecordStoreStateCacheTestUtils.assertCacheHit(timer, 1); assertTrue(recordStore.isIndexDisabled("MySimpleRecord$str_value_indexed")); } } @@ -1312,7 +747,7 @@ void setCacheabilityDuringStoreOpening() { metaDataVersionStamp1 = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); commit(context); } - assertCacheMissAndHitAndReset(timer, 1, 0); + FDBRecordStoreStateCacheTestUtils.assertCacheMissAndHitAndReset(timer, 1, 0); // Open the store, this time changing to make cacheable try (FDBRecordContext context = fdb.openContext(null, timer)) { @@ -1321,7 +756,7 @@ void setCacheabilityDuringStoreOpening() { assertNotCacheable(); commit(context); } - assertCacheMissAndHitAndReset(timer, 1, 0); + FDBRecordStoreStateCacheTestUtils.assertCacheMissAndHitAndReset(timer, 1, 0); // Open the store, this time changing to make cacheable try (FDBRecordContext context = fdb.openContext(null, timer)) { @@ -1330,7 +765,7 @@ void setCacheabilityDuringStoreOpening() { assertCacheable(); commit(context); } - assertCacheMissAndHitAndReset(timer, 1, 0); + FDBRecordStoreStateCacheTestUtils.assertCacheMissAndHitAndReset(timer, 1, 0); // Open the store, again with DEFAULT behavior. It should now be marked cacheable try (FDBRecordContext context = fdb.openContext(null, timer)) { @@ -1339,7 +774,7 @@ void setCacheabilityDuringStoreOpening() { assertCacheable(); commit(context); } - assertCacheMissAndHitAndReset(timer, 1, 0); + FDBRecordStoreStateCacheTestUtils.assertCacheMissAndHitAndReset(timer, 1, 0); // Turn off caching during check version. The actual opening should be a hit, but the next one // should miss as it turns off state cacheability @@ -1349,7 +784,7 @@ void setCacheabilityDuringStoreOpening() { assertNotCacheable(); commit(context); } - assertCacheMissAndHitAndReset(timer, 0, 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMissAndHitAndReset(timer, 0, 1); // Opening the store again should be a cache miss byte[] metaDataVersionStamp2; @@ -1361,7 +796,7 @@ void setCacheabilityDuringStoreOpening() { assertNotCacheable(); commit(context); } - assertCacheMissAndHitAndReset(timer, 1, 0); + FDBRecordStoreStateCacheTestUtils.assertCacheMissAndHitAndReset(timer, 1, 0); // Open again. This time, using NOT_CACHEABLE should not induce any changes (including to the meta-data versionstamp) try (FDBRecordContext context = fdb.openContext(null, timer)) { @@ -1370,7 +805,7 @@ void setCacheabilityDuringStoreOpening() { assertNotCacheable(); commit(context); } - assertCacheMissAndHitAndReset(timer, 1, 0); + FDBRecordStoreStateCacheTestUtils.assertCacheMissAndHitAndReset(timer, 1, 0); try (FDBRecordContext context = fdb.openContext(null, timer)) { assertArrayEquals(context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT), metaDataVersionStamp2, "Meta-data version stamp should not be changed if the store state was originally not cacheable"); @@ -1403,14 +838,14 @@ void setCacheabilityOnStoreCreation() { openSimpleStoreWithCacheabilityOnOpen(context, FDBRecordStore.StateCacheabilityOnOpen.DEFAULT); assertCacheable(); } - assertCacheMissAndHitAndReset(timer, 1, 0); + FDBRecordStoreStateCacheTestUtils.assertCacheMissAndHitAndReset(timer, 1, 0); // Open the store a third time, this time using the cache try (FDBRecordContext context = fdb.openContext(null, timer)) { openSimpleStoreWithCacheabilityOnOpen(context, FDBRecordStore.StateCacheabilityOnOpen.DEFAULT); assertCacheable(); } - assertCacheMissAndHitAndReset(timer, 0, 1); + FDBRecordStoreStateCacheTestUtils.assertCacheMissAndHitAndReset(timer, 0, 1); } @@ -1475,7 +910,7 @@ void doNotSetCacheabilityDuringCheckVersionOnOldFormatVersion() throws Exception public void useWithDifferentDatabase(FDBRecordStoreStateCacheFactory storeStateCacheFactory) throws Exception { final FDBDatabaseFactory factory = dbExtension.getDatabaseFactory(); String clusterFile = FakeClusterFileUtil.createFakeClusterFile("record_store_cache_"); - FDBDatabaseFactory.instance().setStoreStateCacheFactory(readVersionCacheFactory); + FDBDatabaseFactory.instance().setStoreStateCacheFactory(FDBRecordStoreStateCacheTestUtils.readVersionCacheFactory); FDBDatabase secondDatabase = FDBDatabaseFactory.instance().getDatabase(clusterFile); // Using the cache with a context from the wrong database shouldn't work @@ -1496,7 +931,7 @@ public void useWithDifferentDatabase(FDBRecordStoreStateCacheFactory storeStateC @Test public void setCacheableAtWrongFormatVersion() throws Exception { - fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); + fdb.setStoreStateCache(FDBRecordStoreStateCacheTestUtils.metaDataVersionStampCacheFactory.getCache(fdb)); // Initialize the store at the format version prior to the cacheable state version FDBRecordStore.Builder storeBuilder = FDBRecordStore.newBuilder() @@ -1564,82 +999,4 @@ private void ensureMetaDataVersionStampInitialized() { } } - private static void assertCacheMissAndHitAndReset(final FDBStoreTimer timer, final int expectedMissCount, - final int expectedHitCount) { - assertCacheMiss(timer, expectedMissCount); - assertCacheHit(timer, expectedHitCount); - timer.reset(); - } - - private static void assertCacheHit(final FDBStoreTimer timer, int expected) { - assertEquals(expected, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); - } - - private static void assertCacheMiss(final FDBStoreTimer timer, int expected) { - assertEquals(expected, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); - } - - @Nullable - private byte[] getMetaDataVersionStamp() { - try (FDBRecordContext context = fdb.openContext()) { - return context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); - } - } - - private void deleteStore(final Subspace subspace, @Nonnull DeleteStoreMode deleteStoreMode) { - try (FDBRecordContext context = openContext()) { - deleteStoreMode.deleteStore(context, subspace); - commit(context); - } - } - - /** - * Small tuple returned by {@link #createStore(boolean)} for tests that need both a - * pre-configured {@link FDBRecordStore.Builder builder} (to rebind to fresh contexts) and - * the {@link Subspace subspace} the store lives in. Both are captured while the setup - * transaction is still open, so callers don't have to open another context just to - * resolve the subspace. - */ - private record StoreSetup(@Nonnull FDBRecordStore.Builder builder, @Nonnull Subspace subspace) { - } - - /** - * Create the standard simple record store, optionally flipped to cacheable, and return - * a re-usable builder plus the store's subspace. Used by the {@code deleteStore} tests - * to share the "open, configure cacheability, commit" boilerplate. - */ - private StoreSetup createStore(boolean cacheable) throws Exception { - try (FDBRecordContext context = openContext()) { - openSimpleRecordStore(context); - if (cacheable) { - assertTrue(recordStore.setStateCacheability(true), - "flipping to cacheable should have changed the header"); - } else { - assertNotCacheable(); - } - final FDBRecordStore.Builder builder = recordStore.asBuilder(); - final Subspace subspace = recordStore.getSubspace(); - commit(context); - return new StoreSetup(builder, subspace); - } - } - - /** - * Attempt to commit the given context. Returns {@code true} on success, {@code false} if - * the commit failed with a conflict (any nested cause). Any other exception is re-raised - * so an unexpected failure mode doesn't silently masquerade as "conflict". - */ - private boolean tryCommitOrDetectConflict(@Nonnull FDBRecordContext context) throws Exception { - try { - commit(context); - return true; - } catch (Exception ex) { - for (Throwable cause = ex; cause != null; cause = cause.getCause()) { - if (cause instanceof FDBExceptions.FDBStoreTransactionConflictException) { - return false; - } - } - throw new AssertionError("unexpected exception from commit: " + ex, ex); - } - } } diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTestUtils.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTestUtils.java new file mode 100644 index 0000000000..96e166a594 --- /dev/null +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTestUtils.java @@ -0,0 +1,175 @@ +/* + * RecordStoreStateCacheTestUtils.java + * + * This source file is part of the FoundationDB open source project + * + * Copyright 2015-2026 Apple Inc. and the FoundationDB project authors + * + * Licensed 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 com.apple.foundationdb.record.provider.foundationdb.storestate; + +import com.apple.foundationdb.record.IsolationLevel; +import com.apple.foundationdb.record.provider.foundationdb.FDBDatabase; +import com.apple.foundationdb.record.provider.foundationdb.FDBRecordContext; +import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStore; +import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStoreBase; +import com.apple.foundationdb.record.provider.foundationdb.FDBStoreTimer; +import com.apple.foundationdb.tuple.Tuple; + +import javax.annotation.Nonnull; +import java.util.UUID; +import java.util.stream.Stream; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Utilities for testing interactions with {@link FDBRecordStoreStateCache}. + */ +public class FDBRecordStoreStateCacheTestUtils { + @Nonnull + static final ReadVersionRecordStoreStateCacheFactory readVersionCacheFactory = ReadVersionRecordStoreStateCacheFactory.newInstance(); + @Nonnull + static final MetaDataVersionStampStoreStateCacheFactory metaDataVersionStampCacheFactory = MetaDataVersionStampStoreStateCacheFactory.newInstance(); + + @Nonnull + public static Stream testContextSource() { + return Stream.of(new ReadVersionStateCacheTestContext(), new MetaDataVersionStampStateCacheTestContext()); + } + + static void assertCacheMissAndHitAndReset(final FDBStoreTimer timer, final int expectedMissCount, + final int expectedHitCount) { + assertCacheMiss(timer, expectedMissCount); + assertCacheHit(timer, expectedHitCount); + timer.reset(); + } + + static void assertCacheHit(final FDBStoreTimer timer, int expected) { + assertEquals(expected, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_HIT)); + } + + static void assertCacheMiss(final FDBStoreTimer timer, int expected) { + assertEquals(expected, timer.getCount(FDBStoreTimer.Counts.STORE_STATE_CACHE_MISS)); + } + + /** + * A wrapper interface for dealing with the differences between the different {@link FDBRecordStoreStateCache} + * implementations. + */ + public interface StateCacheTestContext { + @Nonnull + FDBRecordStoreStateCache getCache(@Nonnull FDBDatabase database); + + @Nonnull + default FDBRecordContext getCachedContext(@Nonnull FDBDatabase fdb, @Nonnull FDBRecordStore.Builder storeBuilder) { + return getCachedContext(fdb, storeBuilder, FDBRecordStoreBase.StoreExistenceCheck.ERROR_IF_NO_INFO_AND_NOT_EMPTY); + } + + @Nonnull + FDBRecordContext getCachedContext(@Nonnull FDBDatabase fdb, @Nonnull FDBRecordStore.Builder storeBuilder, + @Nonnull FDBRecordStoreBase.StoreExistenceCheck existenceCheck); + + void invalidateCache(@Nonnull FDBDatabase fdb); + } + + /** + * An implementation of the {@link StateCacheTestContext} that handles caching by read version. + */ + public static class ReadVersionStateCacheTestContext implements StateCacheTestContext { + @Nonnull + @Override + public FDBRecordStoreStateCache getCache(@Nonnull FDBDatabase database) { + return readVersionCacheFactory.getCache(database); + } + + @Nonnull + @Override + public FDBRecordContext getCachedContext(@Nonnull FDBDatabase fdb, @Nonnull FDBRecordStore.Builder storeBuilder, + @Nonnull FDBRecordStoreBase.StoreExistenceCheck existenceCheck) { + long readVersion; + try (FDBRecordContext context = fdb.openContext()) { + storeBuilder.copyBuilder().setContext(context).createOrOpen(existenceCheck); + readVersion = context.getReadVersion(); + } + FDBRecordContext context = fdb.openContext(null, new FDBStoreTimer()); + context.setReadVersion(readVersion); + return context; + } + + @Override + public void invalidateCache(@Nonnull FDBDatabase fdb) { + // Ensure that the next read version includes at least one new commit. + try (FDBRecordContext context = fdb.openContext()) { + context.ensureActive().addWriteConflictKey(Tuple.from(UUID.randomUUID()).pack()); + context.commit(); + } + } + + @Override + public String toString() { + return "ReadVersionStateCacheTestContext"; + } + } + + /** + * An implementation of the {@link StateCacheTestContext} that handles caching by the meta-data version-stamp. + */ + public static class MetaDataVersionStampStateCacheTestContext implements StateCacheTestContext { + + @Nonnull + @Override + public FDBRecordStoreStateCache getCache(@Nonnull FDBDatabase database) { + return metaDataVersionStampCacheFactory.getCache(database); + } + + @Nonnull + @Override + public FDBRecordContext getCachedContext(@Nonnull FDBDatabase fdb, @Nonnull FDBRecordStore.Builder storeBuilder, + @Nonnull FDBRecordStoreBase.StoreExistenceCheck existenceCheck) { + boolean cacheable = true; + try (FDBRecordContext context = fdb.openContext()) { + FDBRecordStore store = storeBuilder.copyBuilder().setContext(context).createOrOpen(existenceCheck); + if (!store.getRecordStoreState().getStoreHeader().getCacheable()) { + cacheable = false; + assertTrue(store.setStateCacheability(true)); + context.commit(); + } + } + if (!cacheable) { + try (FDBRecordContext context = fdb.openContext()) { + storeBuilder.copyBuilder().setContext(context).createOrOpen(existenceCheck); + context.commit(); + } + } + FDBRecordContext context = fdb.openContext(null, new FDBStoreTimer()); + context.getMetaDataVersionStampAsync(IsolationLevel.SNAPSHOT).join(); + return context; + } + + @Override + public void invalidateCache(@Nonnull FDBDatabase fdb) { + // Ensure that the next read version includes at least one new commit. + try (FDBRecordContext context = fdb.openContext()) { + context.setMetaDataVersionStamp(); + context.commit(); + } + } + + @Override + public String toString() { + return "MetaDataVersionStampStateCacheTestContext"; + } + } +} From b32c95badf33222dc6040540e9df84bb22aee743 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Fri, 24 Jul 2026 13:27:40 -0400 Subject: [PATCH 11/14] Add basic tests of deleteStore --- .../storestate/DeleteStoreTest.java | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/DeleteStoreTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/DeleteStoreTest.java index 0ab967a640..a397c2bcdd 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/DeleteStoreTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/DeleteStoreTest.java @@ -29,6 +29,7 @@ import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStoreBase; import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStoreTestBase; import com.apple.foundationdb.record.provider.foundationdb.FDBStoreTimer; +import com.apple.foundationdb.record.provider.foundationdb.RecordStoreDoesNotExistException; import com.apple.foundationdb.subspace.Subspace; import com.apple.test.ParameterizedTestUtils; import org.junit.jupiter.params.ParameterizedTest; @@ -46,6 +47,7 @@ import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -277,6 +279,67 @@ void deleteCacheableStoreBumpsMetaDataVersionStamp(@Nonnull DeleteStoreMode dele "deleting a cacheable store should have bumped the meta-data version stamp"); } + /** + * Sanity check that {@link FDBRecordStore#deleteStore}/{@link FDBRecordStore#deleteStoreAsync} + * actually clears the store's subspace: both the store header and every previously-saved + * record disappear on commit. + */ + @ParameterizedTest + @EnumSource(DeleteStoreMode.class) + void deleteStoreClearsSubspace(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { + final StoreSetup setup = createStoreWithRecords(1L, 2L); + assertFalse(isSubspaceEmpty(setup.subspace()), + "store should have data on disk before deletion"); + + deleteStore(setup.subspace(), deleteStoreMode); + + // After the delete, the subspace should be entirely empty — no header, no records, + // no index entries. deleteStore clears subspace.range() unconditionally. + assertTrue(isSubspaceEmpty(setup.subspace()), + "store subspace should be empty after deletion"); + } + + /** + * After a delete, opening the same store with the default + * {@link FDBRecordStoreBase.StoreExistenceCheck#ERROR_IF_NOT_EXISTS} check (i.e. via + * {@link FDBRecordStore.Builder#open() Builder.open()}) must throw + * {@link RecordStoreDoesNotExistException} — the store header is gone, so as far as + * the record layer is concerned the store no longer exists. + */ + @ParameterizedTest + @EnumSource(DeleteStoreMode.class) + void openAfterDeleteThrows(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { + final StoreSetup setup = createStoreWithRecords(1L); + deleteStore(setup.subspace(), deleteStoreMode); + + try (FDBRecordContext context = openContext()) { + assertThrows(RecordStoreDoesNotExistException.class, + () -> setup.builder().copyBuilder().setContext(context).open(), + "opening a deleted store should throw RecordStoreDoesNotExistException"); + } + } + + /** + * After deleting a store, {@link FDBRecordStore.Builder#create()} must succeed on the + * same path — a fresh, empty store — proving the delete really wiped every trace of the + * old store's header rather than merely marking it inactive. + */ + @ParameterizedTest + @EnumSource(DeleteStoreMode.class) + void deletedStoreCanBeRecreated(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { + final StoreSetup setup = createStoreWithRecords(1L); + deleteStore(setup.subspace(), deleteStoreMode); + + // Recreate the store; expect no prior records and be able to save fresh ones. + try (FDBRecordContext context = openContext()) { + final FDBRecordStore recreated = setup.builder().copyBuilder().setContext(context).create(); + assertNotNull(recreated); + recreated.saveRecord(TestRecords1Proto.MySimpleRecord.newBuilder() + .setRecNo(1L).setStrValueIndexed("new").build()); + commit(context); + } + } + @Nonnull public static Stream concurrentOperationPreventsDeleteStore() { @@ -504,6 +567,33 @@ private StoreSetup createStore(boolean cacheable) throws Exception { } } + /** + * Same as {@link #createStore(boolean) createStore(false)} but additionally saves one + * {@link TestRecords1Proto.MySimpleRecord MySimpleRecord} per given {@code recNo} in a + * follow-up transaction, so tests get a store with some concrete on-disk data to + * exercise deletion against without having to hand-roll the save-then-commit boilerplate. + */ + private StoreSetup createStoreWithRecords(long... recNos) throws Exception { + final StoreSetup setup = createStore(false); + try (FDBRecordContext context = openContext()) { + final FDBRecordStore store = setup.builder().copyBuilder().setContext(context).open(); + for (long recNo : recNos) { + store.saveRecord(TestRecords1Proto.MySimpleRecord.newBuilder() + .setRecNo(recNo) + .setStrValueIndexed("rec-" + recNo) + .build()); + } + commit(context); + } + return setup; + } + + private boolean isSubspaceEmpty(@Nonnull Subspace subspace) { + try (FDBRecordContext context = fdb.openContext()) { + return context.ensureActive().getRange(subspace.range()).asList().join().isEmpty(); + } + } + /** * Attempt to commit the given context. Returns {@code true} on success, {@code false} if * the commit failed with a conflict (any nested cause). Any other exception is re-raised From e8e89f50b2dd6c6e75cff79ee39d3b816389cc9a Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Fri, 24 Jul 2026 13:28:37 -0400 Subject: [PATCH 12/14] Remove static imports --- .../provider/foundationdb/storestate/DeleteStoreTest.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/DeleteStoreTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/DeleteStoreTest.java index a397c2bcdd..fa77c666bb 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/DeleteStoreTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/DeleteStoreTest.java @@ -42,8 +42,6 @@ import java.util.Arrays; import java.util.stream.Stream; -import static com.apple.foundationdb.record.provider.foundationdb.storestate.FDBRecordStoreStateCacheTestUtils.metaDataVersionStampCacheFactory; -import static com.apple.foundationdb.record.provider.foundationdb.storestate.FDBRecordStoreStateCacheTestUtils.testContextSource; import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -367,7 +365,7 @@ public static Stream concurrentOperationPreventsDeleteStore() { void concurrentOperationPreventsDeleteStore(@Nonnull ConcurrentOperation op, @Nonnull DeleteStoreMode deleteStoreMode, boolean startCacheable) throws Exception { - fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); + fdb.setStoreStateCache(FDBRecordStoreStateCacheTestUtils.metaDataVersionStampCacheFactory.getCache(fdb)); ensureMetaDataVersionStampInitialized(); final StoreSetup setup = createStore(startCacheable); @@ -438,7 +436,7 @@ void concurrentOperationConflictsWithDeleteStore(@Nonnull ConcurrentOperation op @Nonnull DeleteStoreMode deleteStoreMode, boolean startCacheable, boolean cachePreWarmed) throws Exception { - fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); + fdb.setStoreStateCache(FDBRecordStoreStateCacheTestUtils.metaDataVersionStampCacheFactory.getCache(fdb)); ensureMetaDataVersionStampInitialized(); final StoreSetup setup = createStore(startCacheable); @@ -488,7 +486,7 @@ void concurrentOperationConflictsWithDeleteStore(@Nonnull ConcurrentOperation op @Nonnull public static Stream testContextAndDeleteStoreModeSource() { - return testContextSource().flatMap(testContext -> + return FDBRecordStoreStateCacheTestUtils.testContextSource().flatMap(testContext -> Stream.of(DeleteStoreMode.values()).map(mode -> Arguments.of(testContext, mode))); } From 34e7e31da15a63e939ef5ee76c22bad3493f5844 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Fri, 24 Jul 2026 15:10:06 -0400 Subject: [PATCH 13/14] Elaborate on some of the metadata-version key javadocs --- .../foundationdb/system/SystemKeyspace.java | 10 ++++++++++ .../foundationdb/FDBRecordContext.java | 18 +++++++++--------- .../MetaDataVersionStampStoreStateCache.java | 10 ++++++++++ 3 files changed, 29 insertions(+), 9 deletions(-) diff --git a/fdb-extensions/src/main/java/com/apple/foundationdb/system/SystemKeyspace.java b/fdb-extensions/src/main/java/com/apple/foundationdb/system/SystemKeyspace.java index dc755d8af3..c17de65478 100644 --- a/fdb-extensions/src/main/java/com/apple/foundationdb/system/SystemKeyspace.java +++ b/fdb-extensions/src/main/java/com/apple/foundationdb/system/SystemKeyspace.java @@ -74,6 +74,16 @@ public class SystemKeyspace { // System keys + /** + * The database's meta-data version-stamp key. This is a single key whose value is a versionstamp, bumped + * atomically by {@code SET_VERSIONSTAMPED_VALUE} mutations and returned to every client alongside its + * transaction's read-version — so reading it costs zero storage-server round-trips. Designed to be + * used as a coordinated cross-process cache-invalidation broadcast: any transaction (from + * any client) that bumps the stamp causes every other client's next read to see a new value, which + * can be used to invalidate stale local cache entries. + * + * @see com.apple.foundationdb.MutationType#SET_VERSIONSTAMPED_VALUE + */ public static final byte[] METADATA_VERSION_KEY = systemPrefixedKey("/metadataVersion"); public static final byte[] PRIMARY_DATACENTER_KEY = systemPrefixedKey("/primaryDatacenter"); diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordContext.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordContext.java index 52cca04a0f..82014cd7b7 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordContext.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordContext.java @@ -65,9 +65,9 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.Optional; import java.util.Queue; +import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentNavigableMap; import java.util.concurrent.ConcurrentSkipListMap; @@ -1159,12 +1159,12 @@ public byte[] getVersionStamp() { } /** - * Get the database's meta-data version-stamp. This key is somewhat different from other keys in the - * database in that its value is returned to the client at the same time that the client receives its - * {@linkplain Transaction#getReadVersion() read version}. This means that reading this key does not - * require querying any storage server, so the client can use this key as a kind of "cache invalidation" key - * without needing to worry about the extra reads to this key overloading the backing storage servers (which - * would be the case for other keys). + * Get the database's meta-data version-stamp. This key is somewhat different from other keys in the database, + * in that its value is returned to the client at the same time that the client receives its + * {@linkplain Transaction#getReadVersion() read version}, which means that reading it does not require querying + * any storage server. The intended use is as a coordinated cross-process cache-invalidation signal: any transaction + * that calls {@link #setMetaDataVersionStamp()} advances the stamp, and every other client's next read sees the new + * value at zero storage-server cost. * *

    * This key can only be updated by calling {@link #setMetaDataVersionStamp()}, which will set the key @@ -1215,8 +1215,8 @@ public byte[] getMetaDataVersionStamp(@Nonnull IsolationLevel isolationLevel) { } /** - * Update the meta-data version-stamp. At commit time, the database will write to this key - * the commit version-stamp of this transaction. After this has been committed, any subsequent + * Update the meta-data version-stamp. At commit time, the database will write the commit version-stamp of this + * transaction to a system key. After this has been committed, any subsequent * transaction will see an updated value when calling {@link #getMetaDataVersionStamp(IsolationLevel)}, * and those transactions may use that value to invalidate any stale cache entries using the * meta-data version-stamp key. After this method has been called, any calls to {@code getMetaDataVersionStamp()} diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/storestate/MetaDataVersionStampStoreStateCache.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/storestate/MetaDataVersionStampStoreStateCache.java index 4808fbfe88..17f317a18a 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/storestate/MetaDataVersionStampStoreStateCache.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/storestate/MetaDataVersionStampStoreStateCache.java @@ -42,6 +42,16 @@ * on any record store whose state one would want cached by calling {@link FDBRecordStore#setStateCacheability(boolean)}. * Then only those stores will be cached by this store state cache. * + *

    + * Note that the cache instance itself is process-local and bound to a single {@link FDBDatabase} object, so each JVM + * has its own copy of cached entries. What makes the cache safe across processes is the usage of the + * meta-data version-stamp key: entries are keyed on the value of that stamp at load time, and any transaction + * (in any process) that bumps the stamp via {@link FDBRecordContext#setMetaDataVersionStamp()} causes every process's + * next cache lookup to see a different stamp value and consequently invalidate the older entry. The per-JVM cache and + * the cluster-wide invalidation key are complementary: the former makes the cache cheap to consult, the latter makes it + * safe to trust. + *

    + * * @see FDBRecordStore#setStateCacheabilityAsync(boolean) * @see FDBRecordContext#getMetaDataVersionStamp(IsolationLevel) */ From f6dc002056e2a3c7fbbb40a8e51a684fd1d91898 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Fri, 24 Jul 2026 18:49:10 -0400 Subject: [PATCH 14/14] Various minor cleanup items --- .../provider/foundationdb/FDBStoreTimer.java | 2 +- .../indexes/VersionIndexTest.java | 14 +- .../storestate/DeleteStoreTest.java | 183 +++++++++--------- 3 files changed, 94 insertions(+), 105 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBStoreTimer.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBStoreTimer.java index 42a34eb9a1..86cca7be7f 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBStoreTimer.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBStoreTimer.java @@ -477,7 +477,7 @@ public enum Waits implements Wait { /** Wait to run all the postClose hooks. */ WAIT_RUN_CLOSE_HOOKS("Wait to run all postClose hooks"), /** Wait to delete a store. */ - WAIT_DELETE_STORE("wait to delete store"); + WAIT_DELETE_STORE("wait for delete store"); private final String title; private final String logKey; diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/VersionIndexTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/VersionIndexTest.java index 7c2ef24b04..06c3ea5bc7 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/VersionIndexTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/VersionIndexTest.java @@ -85,6 +85,7 @@ import com.apple.foundationdb.tuple.Tuple; import com.apple.foundationdb.tuple.TupleHelpers; import com.apple.foundationdb.tuple.Versionstamp; +import com.apple.test.ParameterizedTestUtils; import com.apple.test.Tags; import com.google.auto.service.AutoService; import com.google.common.collect.Lists; @@ -3030,14 +3031,11 @@ void deleteAllRecordsWithUncommittedData(FormatVersion testFormatVersion, boolea } static Stream deleteStoreWithUncommittedVersionData() { - return formatVersionsOfInterest().flatMap(testFormatVersion -> - Stream.of(false, true).flatMap(testSplitLongRecords -> - Stream.of(false, true).flatMap(clearPath -> - Stream.of(DeleteStoreMode.values()).map(deleteStoreMode -> - Arguments.of(testFormatVersion, testSplitLongRecords, clearPath, deleteStoreMode) - ) - ) - ) + return ParameterizedTestUtils.cartesianProduct( + formatVersionsOfInterest(), + ParameterizedTestUtils.booleans("splitLongRecords"), + ParameterizedTestUtils.booleans("clearPath"), + Arrays.stream(DeleteStoreMode.values()) ); } diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/DeleteStoreTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/DeleteStoreTest.java index fa77c666bb..93986d0d03 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/DeleteStoreTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/DeleteStoreTest.java @@ -52,10 +52,76 @@ public class DeleteStoreTest extends FDBRecordStoreTestBase { /** - * Validate that deleting a record store causes the record store to go back to the database as it's possible the - * cached stuff is what was deleted. + * Sanity check that {@link FDBRecordStore#deleteStore}/{@link FDBRecordStore#deleteStoreAsync} + * actually clears the store's subspace: both the store header and every previously-saved + * record disappear on commit. + */ + @ParameterizedTest + @EnumSource(DeleteStoreMode.class) + void deleteStoreClearsSubspace(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { + final StoreSetup setup = createStoreWithRecords(1L, 2L); + assertFalse(isSubspaceEmpty(setup.subspace()), + "store should have data on disk before deletion"); + + deleteStore(setup.subspace(), deleteStoreMode); + + // After the delete, the subspace should be entirely empty — no header, no records, + // no index entries. deleteStore clears subspace.range() unconditionally. + assertTrue(isSubspaceEmpty(setup.subspace()), + "store subspace should be empty after deletion"); + } + + /** + * After a delete, opening the same store with the default + * {@link FDBRecordStoreBase.StoreExistenceCheck#ERROR_IF_NOT_EXISTS} check (i.e. via + * {@link FDBRecordStore.Builder#open() Builder.open()}) must throw + * {@link RecordStoreDoesNotExistException} — the store header is gone, so as far as + * the record layer is concerned the store no longer exists. */ - @ParameterizedTest(name = "storeDeletionInSameContext (test context = {0}, deleteMode = {1})") + @ParameterizedTest + @EnumSource(DeleteStoreMode.class) + void openAfterDeleteThrows(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { + final StoreSetup setup = createStoreWithRecords(1L); + deleteStore(setup.subspace(), deleteStoreMode); + + try (FDBRecordContext context = openContext()) { + assertThrows(RecordStoreDoesNotExistException.class, + () -> setup.builder().copyBuilder().setContext(context).open(), + "opening a deleted store should throw RecordStoreDoesNotExistException"); + } + } + + /** + * After deleting a store, {@link FDBRecordStore.Builder#create()} must succeed on the + * same path — a fresh, empty store — proving the delete really wiped every trace of the + * old store's header rather than merely marking it inactive. + */ + @ParameterizedTest + @EnumSource(DeleteStoreMode.class) + void deletedStoreCanBeRecreated(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { + final StoreSetup setup = createStoreWithRecords(1L); + deleteStore(setup.subspace(), deleteStoreMode); + + // Recreate the store; expect no prior records and be able to save fresh ones. + try (FDBRecordContext context = openContext()) { + final FDBRecordStore recreated = setup.builder().copyBuilder().setContext(context).create(); + assertNotNull(recreated); + recreated.saveRecord(TestRecords1Proto.MySimpleRecord.newBuilder() + .setRecNo(1L).setStrValueIndexed("new").build()); + commit(context); + } + } + + @Nonnull + public static Stream testContextAndDeleteStoreModeSource() { + return FDBRecordStoreStateCacheTestUtils.testContextSource().flatMap(testContext -> + Stream.of(DeleteStoreMode.values()).map(mode -> Arguments.of(testContext, mode))); + } + + /** + * Validate that deleting a record store invalidates the {@link FDBRecordStoreStateCache}. + */ + @ParameterizedTest @MethodSource("testContextAndDeleteStoreModeSource") public void storeDeletionInSameContext(@Nonnull FDBRecordStoreStateCacheTestUtils.StateCacheTestContext testContext, @Nonnull DeleteStoreMode deleteStoreMode) throws Exception { @@ -115,9 +181,9 @@ public void storeDeletionInSameContext(@Nonnull FDBRecordStoreStateCacheTestUtil } /** - * After a store is deleted, validate that future transactions need to reload it from cache. + * After a store is deleted, validate that future transactions need to reload the store header/state from cache. */ - @ParameterizedTest(name = "storeDeletionAcrossContexts (test context = {0}, deleteMode = {1})") + @ParameterizedTest @MethodSource("testContextAndDeleteStoreModeSource") public void storeDeletionAcrossContexts(@Nonnull FDBRecordStoreStateCacheTestUtils.StateCacheTestContext testContext, @Nonnull DeleteStoreMode deleteStoreMode) throws Exception { @@ -191,17 +257,16 @@ public void storeDeletionAcrossContexts(@Nonnull FDBRecordStoreStateCacheTestUti *
      *
    • {@link DeleteStoreMode#ASYNC ASYNC} (new): reads the header, sees non-cacheable, and skips the meta-data * version-stamp bump. No other client can hold a cached copy of a non-cacheable header, so bumping the - * cluster-wide bottleneck on the {@code \xff/metadataVersion} key would be pure waste. This is key to - * reducing cache invalidations and conflicts if a cluster is using the meta-data version-stamp, and is - * frequently deleting non-cacheable stores. For, example, if you have the relational catalog, and are - * frequently creating/deleting unrelated schemas, they would have conflicted with each other, because the - * catalog is a store with a cached header.
      • + * cluster-wide bottleneck on the {@link com.apple.foundationdb.system.SystemKeyspace#METADATA_VERSION_KEY} + * key would be pure waste. This is key to reducing cache invalidations and conflicts if a cluster is using + * the meta-data version-stamp, and is frequently deleting non-cacheable stores. For, example, if you have the + * relational catalog, and are frequently creating/deleting unrelated schemas, they would have conflicted with + * each other, because the catalog is a store with a cached header. *
    • {@link DeleteStoreMode#SYNC SYNC} (deprecated): bumps the meta-data version stamp unconditionally without - * reading the header — the pre-{@code deleteStoreAsync} behavior. Documented here so the deprecated method's - * cost is not silently forgotten.
      • + * reading the header. Included here to make sure we don't lose coverage before deleting the sync method. *
    */ - @ParameterizedTest(name = "deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp [{0}]") + @ParameterizedTest @EnumSource(DeleteStoreMode.class) void deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { ensureMetaDataVersionStampInitialized(); @@ -214,7 +279,7 @@ void deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp(@Nonnull DeleteStore * split: {@link DeleteStoreMode#ASYNC ASYNC} sees the missing header and skips the bump; * {@link DeleteStoreMode#SYNC SYNC} bumps unconditionally. */ - @ParameterizedTest(name = "deleteMissingStoreDoesNotBumpMetaDataVersionStamp [{0}]") + @ParameterizedTest @EnumSource(DeleteStoreMode.class) void deleteMissingStoreDoesNotBumpMetaDataVersionStamp(@Nonnull DeleteStoreMode deleteStoreMode) { ensureMetaDataVersionStampInitialized(); @@ -228,7 +293,7 @@ void deleteMissingStoreDoesNotBumpMetaDataVersionStamp(@Nonnull DeleteStoreMode } /** - * Snapshot the meta-data version stamp, run a delete of the given subspace via {@code mode}, + * Snapshot the meta-data version stamp, run a delete of the given subspace via {@code deleteStoreMode}, * and assert the stamp behaved as the mode's contract requires: {@link DeleteStoreMode#SYNC} * bumps unconditionally, {@link DeleteStoreMode#ASYNC} leaves the stamp untouched when the * header is absent or marks the store non-cacheable. @@ -259,7 +324,7 @@ private void assertMetaDataVersionStampBehaviorOnNonCacheableOrMissingStore(fina * reads out of a stale cached header long after the store is gone. Both modes bump: SYNC does * so unconditionally, ASYNC does so because the header parses as cacheable. */ - @ParameterizedTest(name = "deleteCacheableStoreBumpsMetaDataVersionStamp [{0}]") + @ParameterizedTest @EnumSource(DeleteStoreMode.class) void deleteCacheableStoreBumpsMetaDataVersionStamp(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { ensureMetaDataVersionStampInitialized(); @@ -277,68 +342,6 @@ void deleteCacheableStoreBumpsMetaDataVersionStamp(@Nonnull DeleteStoreMode dele "deleting a cacheable store should have bumped the meta-data version stamp"); } - /** - * Sanity check that {@link FDBRecordStore#deleteStore}/{@link FDBRecordStore#deleteStoreAsync} - * actually clears the store's subspace: both the store header and every previously-saved - * record disappear on commit. - */ - @ParameterizedTest - @EnumSource(DeleteStoreMode.class) - void deleteStoreClearsSubspace(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { - final StoreSetup setup = createStoreWithRecords(1L, 2L); - assertFalse(isSubspaceEmpty(setup.subspace()), - "store should have data on disk before deletion"); - - deleteStore(setup.subspace(), deleteStoreMode); - - // After the delete, the subspace should be entirely empty — no header, no records, - // no index entries. deleteStore clears subspace.range() unconditionally. - assertTrue(isSubspaceEmpty(setup.subspace()), - "store subspace should be empty after deletion"); - } - - /** - * After a delete, opening the same store with the default - * {@link FDBRecordStoreBase.StoreExistenceCheck#ERROR_IF_NOT_EXISTS} check (i.e. via - * {@link FDBRecordStore.Builder#open() Builder.open()}) must throw - * {@link RecordStoreDoesNotExistException} — the store header is gone, so as far as - * the record layer is concerned the store no longer exists. - */ - @ParameterizedTest - @EnumSource(DeleteStoreMode.class) - void openAfterDeleteThrows(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { - final StoreSetup setup = createStoreWithRecords(1L); - deleteStore(setup.subspace(), deleteStoreMode); - - try (FDBRecordContext context = openContext()) { - assertThrows(RecordStoreDoesNotExistException.class, - () -> setup.builder().copyBuilder().setContext(context).open(), - "opening a deleted store should throw RecordStoreDoesNotExistException"); - } - } - - /** - * After deleting a store, {@link FDBRecordStore.Builder#create()} must succeed on the - * same path — a fresh, empty store — proving the delete really wiped every trace of the - * old store's header rather than merely marking it inactive. - */ - @ParameterizedTest - @EnumSource(DeleteStoreMode.class) - void deletedStoreCanBeRecreated(@Nonnull DeleteStoreMode deleteStoreMode) throws Exception { - final StoreSetup setup = createStoreWithRecords(1L); - deleteStore(setup.subspace(), deleteStoreMode); - - // Recreate the store; expect no prior records and be able to save fresh ones. - try (FDBRecordContext context = openContext()) { - final FDBRecordStore recreated = setup.builder().copyBuilder().setContext(context).create(); - assertNotNull(recreated); - recreated.saveRecord(TestRecords1Proto.MySimpleRecord.newBuilder() - .setRecNo(1L).setStrValueIndexed("new").build()); - commit(context); - } - } - - @Nonnull public static Stream concurrentOperationPreventsDeleteStore() { return ParameterizedTestUtils.cartesianProduct( @@ -355,7 +358,7 @@ public static Stream concurrentOperationPreventsDeleteStore() { * *

    The key expectatian is that either:

    *
      - *
    • The delete succeeds, and the range is empty, and if the store was even cacheable, the meta-data + *
    • The delete succeeds, and the range is empty, and if the store was ever cacheable, the meta-data * version-stamp was bumped.
      • *
    • The delete conflicts.
      • *
    @@ -387,8 +390,7 @@ void concurrentOperationPreventsDeleteStore(@Nonnull ConcurrentOperation op, deleterCommitted = tryCommitOrDetectConflict(deleterContext); } - final boolean expectDeleterConflicts = - deleteStoreMode == DeleteStoreMode.ASYNC && op.writesStoreHeader(); + final boolean expectDeleterConflicts = deleteStoreMode == DeleteStoreMode.ASYNC && op.writesStoreHeader(); if (expectDeleterConflicts) { if (deleterCommitted) { fail("expected deleteStoreAsync to conflict with a concurrent " + op @@ -402,9 +404,7 @@ void concurrentOperationPreventsDeleteStore(@Nonnull ConcurrentOperation op, "op's committed state should still be on disk after the deleter conflicted"); } } else { - assertTrue(deleterCommitted, - "expected the deleter to commit for op=" + op + " mode=" + deleteStoreMode - + " startCacheable=" + startCacheable); + assertTrue(deleterCommitted, "expected the deleter to commit"); // Deleter's clear(subspace.range()) supersedes whatever the op wrote. try (FDBRecordContext peek = fdb.openContext(null, new FDBStoreTimer())) { assertTrue(peek.ensureActive().getRange(setup.subspace().range()).asList().join().isEmpty(), @@ -483,14 +483,6 @@ void concurrentOperationConflictsWithDeleteStore(@Nonnull ConcurrentOperation op } } - - @Nonnull - public static Stream testContextAndDeleteStoreModeSource() { - return FDBRecordStoreStateCacheTestUtils.testContextSource().flatMap(testContext -> - Stream.of(DeleteStoreMode.values()).map(mode -> Arguments.of(testContext, mode))); - } - - @Nullable private byte[] getMetaDataVersionStamp() { try (FDBRecordContext context = fdb.openContext()) { @@ -567,9 +559,8 @@ private StoreSetup createStore(boolean cacheable) throws Exception { /** * Same as {@link #createStore(boolean) createStore(false)} but additionally saves one - * {@link TestRecords1Proto.MySimpleRecord MySimpleRecord} per given {@code recNo} in a - * follow-up transaction, so tests get a store with some concrete on-disk data to - * exercise deletion against without having to hand-roll the save-then-commit boilerplate. + * {@link TestRecords1Proto.MySimpleRecord MySimpleRecord} per given {@code recNo} in a follow-up transaction, + * so tests get a store with some concrete on-disk data to exercise deletion against. */ private StoreSetup createStoreWithRecords(long... recNos) throws Exception { final StoreSetup setup = createStore(false); @@ -664,7 +655,7 @@ void apply(@Nonnull FDBRecordStore store) { } /** - * Whether or not this operation writes to the the store header. + * Whether this operation writes to the store header. * @return {@code true} iff the operation writes {@code STORE_INFO_KEY} on commit. * The write-set of the concurrent operation is what {@code deleteStoreAsync}'s * header read conflicts with; operations that never touch {@code STORE_INFO_KEY}