Change deleteStore to check StoreHeader before bumping MetaDataVersionStamp

fdb-extensions/src/main/java/com/apple/foundationdb/system/SystemKeyspace.java

@@ -73,7 +73,17 @@
     private static final byte[] SPECIAL_PREFIX = {(byte)0xff, (byte)0xff};
 
     // 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");
 

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 @@
     }
 
     /**
-     * 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.
      *
      * <p>
      * This key can only be updated by calling {@link #setMetaDataVersionStamp()}, which will set the key
@@ -1215,8 +1215,8 @@
     }
 
     /**
-     * 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()}

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java

@@ -1779,22 +1779,26 @@
         SplitHelper.deleteSplit(getRecordContext(), recordsSubspace(), primaryKey, metaData.isSplitLongRecords(), omitUnsplitRecordSuffix, clearBasedOnPreviousSizeInfo, oldRecord);
     }
 
     /**
-     * Delete the record store at the given {@link KeySpacePath}. This behaves like
+     * Delete the record store at the given {@link KeySpacePath}; <em>deprecated</em>, 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}. In addition to the store's
+     * Delete the record store at the given {@link Subspace}; <em>deprecated</em>, 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.
      *
@@ -1810,15 +1814,80 @@
      * @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, but we can't know that from here.
+        // 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<Void> 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
+     * the store existed.
+     *
+     * <p>
+     * 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.
+     * </p>
+     *
+     * @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 CompletableFuture<Void> 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);
+        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());
+        });
+    }
+
     @Override
     @SuppressWarnings("PMD.CloseResource")
     public void deleteAllRecords() {

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStoreBase.java

@@ -1660,13 +1660,13 @@
     /**
      * Delete all the data in the record store.
      * <p>
      * 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.
      *
      * <p>
@@ -1676,8 +1676,8 @@
      * See: <a href="https://github.com/FoundationDB/fdb-record-layer/issues/398">Issue #398</a>.
      * </p>
      *
-     * @see FDBRecordStore#deleteStore(FDBRecordContext, KeySpacePath)
-     * @see FDBRecordStore#deleteStore(FDBRecordContext, Subspace)
+     * @see FDBRecordStore#deleteStoreAsync(FDBRecordContext, KeySpacePath)
+     * @see FDBRecordStore#deleteStoreAsync(FDBRecordContext, Subspace)
      */
     void deleteAllRecords();
 

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBStoreTimer.java

@@ -475,8 +475,9 @@
         /** wait to load partition metadata for one or more grouping key. */
         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 for delete store");
 
         private final String title;
         private final String logKey;

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/storestate/MetaDataVersionStampStoreStateCache.java

@@ -41,7 +41,17 @@
  * has marked its state as cacheable. This means that one should first set the state cacheability to {@code true}
  * 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.
  *
+ * <p>
+ * 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.
+ * </p>
+ *
  * @see FDBRecordStore#setStateCacheabilityAsync(boolean)
  * @see FDBRecordContext#getMetaDataVersionStamp(IsolationLevel)
  */

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);

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();

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();

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();

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.

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;
@@ -84,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;
@@ -3029,18 +3031,18 @@ void deleteAllRecordsWithUncommittedData(FormatVersion testFormatVersion, boolea
     }
 
     static Stream<Arguments> deleteStoreWithUncommittedVersionData() {
-        return formatVersionsOfInterest().flatMap(testFormatVersion ->
-                Stream.of(false, true).flatMap(testSplitLongRecords ->
-                        Stream.of(false, true).map(clearPath ->
-                            Arguments.of(testFormatVersion, testSplitLongRecords, clearPath)
-                        )
-                )
+        return ParameterizedTestUtils.cartesianProduct(
+                formatVersionsOfInterest(),
+                ParameterizedTestUtils.booleans("splitLongRecords"),
+                ParameterizedTestUtils.booleans("clearPath"),
+                Arrays.stream(DeleteStoreMode.values())
         );
     }
 
-    @ParameterizedTest(name = "deleteStoreWithUncommittedVersionData [" + ARGUMENTS_PLACEHOLDER + "]")
+    @ParameterizedTest
     @MethodSource
-    void deleteStoreWithUncommittedVersionData(FormatVersion testFormatVersion, boolean testSplitLongRecords, boolean clearPath) throws Exception {
+    void deleteStoreWithUncommittedVersionData(FormatVersion testFormatVersion, boolean testSplitLongRecords, boolean clearPath,
+                                               DeleteStoreMode deleteStoreMode) throws Exception {
         formatVersion = testFormatVersion;
         splitLongRecords = testSplitLongRecords;
 
@@ -3098,7 +3100,7 @@ void deleteStoreWithUncommittedVersionData(FormatVersion testFormatVersion, bool
             if (clearPath) {
                 path1.deleteAllData(context);
             } else {
-                FDBRecordStore.deleteStore(context, path1);
+                deleteStoreMode.deleteStore(context, path1);
             }
 
             i = 0;
@@ -3115,7 +3117,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()