Skip to content

Commit

Permalink
Merge pull request #1937 from alecgrieser/01936-old-version-clear
Browse files Browse the repository at this point in the history 
Resolves #1936: Reduce unnecessary clears of legacy version space
  • Loading branch information
@MMcM
MMcM committed Dec 5, 2022
2 parents a5f98c4 + 72f3391 commit fde2de3
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 11 deletions.
2 changes: 1 addition & 1 deletion docs/ReleaseNotes.md
View file
Expand Up @@ -21,7 +21,7 @@ The Guava dependency version has been updated to 31.1. Projects may need to chec
* **Bug fix** Fix 4 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
* **Bug fix** Fix 5 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
* **Performance** Improvement 1 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
* **Performance** Improvement 2 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
* **Performance** Reduce the number of extraneous clear ranges issued during `checkVersion` [(Issue #1936)](https://github.com/FoundationDB/fdb-record-layer/issues/1936)
* **Performance** Improvement 3 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
* **Performance** Improvement 4 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
* **Performance** Improvement 5 [(Issue #NNN)](https://github.com/FoundationDB/fdb-record-layer/issues/NNN)
Expand Down
15 changes: 11 additions & 4 deletions ...ore/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java
View file
Expand Up @@ -331,10 +331,14 @@ public int getUserVersion() {
}

private boolean useOldVersionFormat() {
return useOldVersionFormat(getFormatVersion(), omitUnsplitRecordSuffix);
}

private static boolean useOldVersionFormat(int formatVersion, boolean omitUnsplitRecordSuffix) {
// If the store is either explicitly using the older format version or if
// it is using a newer one, but because of how the data were originally stored
// in this record store, then use the older location for record versions.
return getFormatVersion() < SAVE_VERSION_WITH_RECORD_FORMAT_VERSION || omitUnsplitRecordSuffix;
return formatVersion < SAVE_VERSION_WITH_RECORD_FORMAT_VERSION || omitUnsplitRecordSuffix;
}

/**
Expand Down Expand Up @@ -2042,11 +2046,11 @@ private CompletableFuture<Boolean> checkVersion(@Nonnull RecordMetaDataProto.Dat
private CompletableFuture<Void> checkUserVersion(@Nullable UserVersionChecker userVersionChecker,
@Nonnull final RecordMetaDataProto.DataStoreInfo storeHeader,
@Nonnull RecordMetaDataProto.DataStoreInfo.Builder info, @Nonnull boolean[] dirty) {
final boolean newStore = info.getFormatVersion() == 0;
final int oldUserVersion = newStore ? -1 : info.getUserVersion();
if (userVersionChecker == null) {
return AsyncUtil.DONE;
}
final boolean newStore = info.getFormatVersion() == 0;
final int oldUserVersion = newStore ? -1 : info.getUserVersion();
return userVersionChecker.checkUserVersion(storeHeader, metaDataProvider)
.thenApply(newUserVersion -> {
userVersion = newUserVersion;
Expand Down Expand Up @@ -3920,7 +3924,10 @@ private CompletableFuture<Void> checkRebuild(@Nullable UserVersionChecker userVe
final boolean metaDataVersionChanged = oldMetaDataVersion != newMetaDataVersion;
if (metaDataVersionChanged) {
// Clear the version table if we are no longer storing record versions.
if (!metaData.isStoreRecordVersions()) {
// This can be skipped if the store is new (in which case there is no data), or if the old
// store did not use the old version format to store record versions
if (!metaData.isStoreRecordVersions() && !newStore
&& useOldVersionFormat(oldFormatVersion, omitUnsplitRecordSuffix)) {
final Transaction tr = ensureContextActive();
tr.clear(getSubspace().subspace(Tuple.from(RECORD_VERSION_KEY)).range());
}
Expand Down
93 changes: 87 additions & 6 deletions ...st/java/com/apple/foundationdb/record/provider/foundationdb/indexes/VersionIndexTest.java
View file
Expand Up @@ -22,6 +22,7 @@

import com.apple.foundationdb.record.ExecuteProperties;
import com.apple.foundationdb.record.IndexEntry;
import com.apple.foundationdb.record.IndexFetchMethod;
import com.apple.foundationdb.record.IndexScanType;
import com.apple.foundationdb.record.ObjectPlanHash;
import com.apple.foundationdb.record.PlanHashable;
Expand Down Expand Up @@ -55,10 +56,12 @@
import com.apple.foundationdb.record.provider.foundationdb.FDBQueriedRecord;
import com.apple.foundationdb.record.provider.foundationdb.FDBRecord;
import com.apple.foundationdb.record.provider.foundationdb.FDBRecordContext;
import com.apple.foundationdb.record.provider.foundationdb.FDBRecordContextConfig;
import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStore;
import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStoreBase;
import com.apple.foundationdb.record.provider.foundationdb.FDBRecordStoreTestBase.RecordMetaDataHook;
import com.apple.foundationdb.record.provider.foundationdb.FDBRecordVersion;
import com.apple.foundationdb.record.provider.foundationdb.FDBStoreTimer;
import com.apple.foundationdb.record.provider.foundationdb.FDBStoredRecord;
import com.apple.foundationdb.record.provider.foundationdb.FDBTestBase;
import com.apple.foundationdb.record.provider.foundationdb.IndexOrphanBehavior;
Expand All @@ -68,7 +71,6 @@
import com.apple.foundationdb.record.provider.foundationdb.TestKeySpace;
import com.apple.foundationdb.record.query.RecordQuery;
import com.apple.foundationdb.record.query.expressions.Query;
import com.apple.foundationdb.record.IndexFetchMethod;
import com.apple.foundationdb.record.query.plan.RecordQueryPlanner;
import com.apple.foundationdb.record.query.plan.plans.RecordQueryPlan;
import com.apple.foundationdb.subspace.Subspace;
Expand Down Expand Up @@ -230,15 +232,24 @@ public void setUp() {
metaDataBuilder.addIndex("MySimpleRecord", maxEverVersionIndex);
};

// Provide a combination of format versions relevant to versionstamps along with
// information as to whether large records are split
private static Stream<Arguments> formatVersionArguments() {
private static Stream<Integer> formatVersionsOfInterest() {
return Stream.of(
FDBRecordStore.SAVE_UNSPLIT_WITH_SUFFIX_FORMAT_VERSION - 1,
FDBRecordStore.SAVE_UNSPLIT_WITH_SUFFIX_FORMAT_VERSION,
FDBRecordStore.SAVE_VERSION_WITH_RECORD_FORMAT_VERSION,
FDBRecordStore.MAX_SUPPORTED_FORMAT_VERSION
).flatMap(formatVersion -> Stream.of(Arguments.of(formatVersion, true), Arguments.of(formatVersion, false)));
);
}

private static Stream<Integer> formatVersionsOfInterest(int minVersion) {
return formatVersionsOfInterest().filter(version -> version >= minVersion);
}

// Provide a combination of format versions relevant to versionstamps along with
// information as to whether large records are split
private static Stream<Arguments> formatVersionArguments() {
return formatVersionsOfInterest()
.flatMap(formatVersion -> Stream.of(Arguments.of(formatVersion, true), Arguments.of(formatVersion, false)));
}

// Provide a combination of format versions, split and a remote fetch option
Expand Down Expand Up @@ -423,7 +434,10 @@ private FDBRecordContext openContext(@Nullable RecordMetaDataHook hook) {
RecordMetaDataBuilder metaDataBuilder = RecordMetaData.newBuilder().setRecords(TestRecords1Proto.getDescriptor());
hook.apply(metaDataBuilder);

FDBRecordContext context = fdb.openContext();
FDBRecordContextConfig config = FDBRecordContextConfig.newBuilder()
.setTimer(new FDBStoreTimer())
.build();
FDBRecordContext context = fdb.openContext(config);
recordStore = FDBRecordStore.newBuilder()
.setMetaDataProvider(metaDataBuilder)
.setContext(context)
Expand Down Expand Up @@ -1085,6 +1099,73 @@ public void updateWithinContext(int testFormatVersion, boolean testSplitLongReco
}
}

@SuppressWarnings("unused") // used as parameter source for parameterized test
static Stream<Arguments> updateFormatVersionAndVersionStorage() {
return formatVersionsOfInterest().flatMap(firstFormatVersion ->
formatVersionsOfInterest(firstFormatVersion).flatMap(secondFormatVersion ->
formatVersionsOfInterest(secondFormatVersion).flatMap(thirdFormatVersion ->
Stream.of(false, true).map(testSplitLongRecords -> Arguments.of(firstFormatVersion, secondFormatVersion, thirdFormatVersion, testSplitLongRecords)))));
}

@ParameterizedTest(name = "updateFormatVersionAndVersionStorage[firstFormatVersion={0}, secondFormatVersion={1}, thirdFormatVersion={2}, splitLongRecords={2}]")
@MethodSource
public void updateFormatVersionAndVersionStorage(int firstFormatVersion, int secondFormatVersion, int thirdFormatVersion, boolean testSplitLongRecords) {
formatVersion = firstFormatVersion;
splitLongRecords = testSplitLongRecords;

final RecordMetaDataHook noVersionHook = metaDataBuilder -> {
simpleVersionHook.apply(metaDataBuilder);
metaDataBuilder.removeIndex("globalVersion");
metaDataBuilder.removeIndex("MySimpleRecord$num2-version");
metaDataBuilder.setStoreRecordVersions(false);
};

// Save records with versions
final MySimpleRecord record1 = MySimpleRecord.newBuilder().setRecNo(1066).setNumValue2(42).build();
try (FDBRecordContext context = openContext(simpleVersionHook)) {
recordStore.saveRecord(record1);
context.commit();
}

formatVersion = secondFormatVersion;

FDBRecordVersion version;
boolean inLegacyVersionSpace;
try (FDBRecordContext context = openContext(simpleVersionHook)) {
version = recordStore.loadRecord(Tuple.from(1066L)).getVersion();
assertNotNull(version);
inLegacyVersionSpace = context.ensureActive().getRange(recordStore.getLegacyVersionSubspace().range()).iterator().hasNext();
boolean shouldHaveVersionInOldSpace = (secondFormatVersion < FDBRecordStore.SAVE_VERSION_WITH_RECORD_FORMAT_VERSION)
|| (!testSplitLongRecords && firstFormatVersion < FDBRecordStore.SAVE_UNSPLIT_WITH_SUFFIX_FORMAT_VERSION);
assertEquals(shouldHaveVersionInOldSpace, inLegacyVersionSpace);
context.commit();
}

formatVersion = thirdFormatVersion;

try (FDBRecordContext context = openContext(noVersionHook)) {
FDBRecordVersion newVersion = recordStore.loadRecord(Tuple.from(1066L)).getVersion();
if (inLegacyVersionSpace) {
// We clear out the legacy version space if versions are removed, so this should be null
assertNull(newVersion);
} else {
// We leave the version if it's stored with the record
assertEquals(version, newVersion);
}
// There should be 4 range deletes per former index, plus 1 for the version space, if required.
// This assert may need to change if additional indexes subspaces are created
long rangeDeletes = recordStore.getTimer().getCount(FDBStoreTimer.Counts.RANGE_DELETES);
if (inLegacyVersionSpace) {
assertEquals(9L, rangeDeletes);
} else {
assertEquals(8L, rangeDeletes);
}
// The legacy version space should be empty now, either because it was deleted or because the version was
// never there
assertFalse(context.ensureActive().getRange(recordStore.getLegacyVersionSubspace().range()).iterator().hasNext());
}
}

/**
* Store two records with the same primary key in two record stores. Each one should get its own version.
* This validates that the local version cache is per-record-store.
Expand Down

0 comments on commit fde2de3

Please sign in to comment.