Skip to content

Transition FormatVersion from constants to an enum #3304

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
Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Transition FormatVersion from constants to an enum #3304

wants to merge 20 commits into from
+828 −251

Conversation

ScottDugas
Copy link
Collaborator

@ScottDugas ScottDugas commented Apr 15, 2025 •
edited
Loading

The FormatVersion was just an integers, with an associated set of constants, of the form: FDBRecordStore.*_FORMAT_VERSION. This PR starts the migration of the code to a new enum: FormatVersion, and introduces setters and getters that align with this.
This leaves around the existing constants, and methods to avoid being a breaking change, but they are all now flagged with @API(API.Status.DEPRECATED) and will be removed in the near future.

I did not replace the integer usages in the internals of the record store, because I felt that code was complicated enough that it would be worth focusing on independently. I did change everything else to use the new code.

This also adds a bunch of documentation, and thus resolves: #710.
I also added a bunch of documentation to the DatastoreInfo, because a lot of the details of that are tightly coupled with the FormatVersion.

Note: This is somewhat related to #3309 as some of the new paths will also throw an exception on invalid states, whereas the integer versions would not.

@ScottDugas ScottDugas added enhancement New feature or request documentation Documentation change labels Apr 15, 2025
@ScottDugas
Introduce a new FormatVersion enum, and document it
f2c257c 
The intent is to replace the constants in FDBRecordStore with this,
but I wanted to introduce it on it's own, primarily to show that
FormatVersionTest.testOrderingAlignment works with the constants.
This is part of FoundationDB#710, but in order for that be complete, the
constants will have to be updated too.
@ScottDugas ScottDugas force-pushed the document-format-version branch from 335fad3 to f2c257c Compare April 15, 2025 16:39
@ScottDugas
Update a large portion of the code to use FormatVersion
3e37225 
This changes the constants in FDBRecordStore to call through to the
enum. It also adds setters on the builders, and deprecates the old
ones. With this change, I also went through and changed all the
tests to use the enum rather than the constants.

This leaves a couple places untouched:
1. The code within FDBRecordStore still uses the constants
2. getFormatVersion() still returns the integer
3. Code interacting with the result of getFormatVersion() also
references the comments
@ScottDugas
Replace most remaining usages of the integer FormatVersion
18137fe 
This deprecates the remaining places that treat the FormatVersion
as just an integer, and adds a new `FDBRecordStore.getFormatVersionEnum`.

I did not change the code around rebuilding, or that interacts with
a StoreHeader, because that felt like it was still bringing in
a fair amount of risk.
@ScottDugas ScottDugas changed the title Document format version Transition FormatVersion from constants to an enum Apr 16, 2025
ScottDugas
ScottDugas commented Apr 16, 2025
View reviewed changes
...ava/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStoreFormatVersionTest.java Outdated
Comment on lines 47 to 51
final List<FormatVersion> sortedVersions = Arrays.stream(FormatVersion.values()).sorted().collect(Collectors.toList());
FormatVersion penultimateVersion = sortedVersions.get(sortedVersions.size() - 1);
try (FDBRecordContext context = openContext()) {
recordStore = getStoreBuilder(context, simpleMetaData(NO_HOOK))
.setFormatVersion(FDBRecordStore.MAX_SUPPORTED_FORMAT_VERSION - 1)
.setFormatVersion(penultimateVersion)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about introducing a FormatVersion.previous(), but it would only be used for internal tests, and you can't change the numbers associated with the enums, so it seemed best to keep it simple.

alecgrieser
alecgrieser approved these changes Apr 16, 2025
View reviewed changes
Copy link
Collaborator

@alecgrieser alecgrieser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few smaller comments about some things that could be improved or were a bit erroneous, but overall, this LGTM, and I like the way this refactoring evolves the API

@ScottDugas
Add a FormatVersionTestUtils.previous
2cb848e 
This retrieves the format version previous to the given one, which
is very useful for upgrade tests.
Since this is only used for upgrade tests, I felt it was better
to add as a test utils, rather than a method on FormatVersion itself
@ScottDugas ScottDugas force-pushed the document-format-version branch from 67cded1 to 2cb848e Compare April 16, 2025 18:55
@ScottDugas
Update comments as per PR-comments
c7669f5
@ScottDugas
Change FDBRecordStore.formatVersion to FormatVersion
6950019 
Note: This changes a `protected` method of FDBRecordStore, and field,
but the class is flagged `UNSTABLE`. It _feels_ to me like users
should assume that extending classes is INTERNAL, and really anything
protected.
@ScottDugas
Add FormatVersion.isAtLeast
46aec46
@ScottDugas ScottDugas force-pushed the document-format-version branch from a5a3da9 to 46aec46 Compare April 17, 2025 18:31
ScottDugas
ScottDugas commented Apr 18, 2025
View reviewed changes
.../test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/VersionIndexTest.java Outdated
FDBRecordStore.SAVE_UNSPLIT_WITH_SUFFIX_FORMAT_VERSION,
FDBRecordStore.SAVE_VERSION_WITH_RECORD_FORMAT_VERSION,
FDBRecordStore.MAX_SUPPORTED_FORMAT_VERSION
FormatVersion.SAVE_UNSPLIT_WITH_SUFFIX,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was wrong, it should be the previous one.

.../test/java/com/apple/foundationdb/record/provider/foundationdb/indexes/VersionIndexTest.java
@@ -2254,23 +2257,23 @@ public void withMetaDataRebuilds(int testFormatVersion, boolean testSplitLongRec
List<FDBQueriedRecord<Message>> records = recordStore.executeQuery(plan).asList().join();
assertEquals(3, records.size());

if (testFormatVersion < FDBRecordStore.SAVE_VERSION_WITH_RECORD_FORMAT_VERSION) {
if (testFormatVersion.isAtLeast(FormatVersion.SAVE_VERSION_WITH_RECORD)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if/else block was inverted to avoid the negation

@ScottDugas ScottDugas marked this pull request as ready for review April 18, 2025 16:53
alecgrieser
alecgrieser reviewed Apr 23, 2025
View reviewed changes
Copy link
Collaborator

@alecgrieser alecgrieser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was one relatively minor typo, but otherwise, LGTM pending resolving the merge conflicts with #3309

@ScottDugas
Merge branch 'main' into document-format-version
645692e 
This conflicted on FDBRecordStore.Builder.setFormatVersion
The checks are now handled within the enum, so not needed
explicitly in the setter.

This still fails to compile because new tests on main use
the deprecated constants.
@ScottDugas
Adjust FDBRecordStoreFormatVersionTest to fix deprecation warnings
b7843fb
@ScottDugas ScottDugas requested a review from alecgrieser April 25, 2025 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alecgrieser alecgrieser Awaiting requested review from alecgrieser

Assignees
No one assigned
Labels
documentation Documentation change enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Document the different store format versions
2 participants
@ScottDugas @alecgrieser