Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Resolves #1936: Reduce unnecessary clears of legacy version space #1937

Merged
merged 1 commit into from Dec 5, 2022
Merged

Resolves #1936: Reduce unnecessary clears of legacy version space #1937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/ReleaseNotes.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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