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.

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

Support switching on / off UDT together with in-Memtable-only feature #11623

Closed
wants to merge 2 commits into from

Conversation

jowlyzhang
Copy link
Contributor

Add support to allow enabling / disabling user-defined timestamps feature for an existing column family in combination with the in-Memtable only feature.

To do this, this PR includes:

  1. Log the persist_user_defined_timestamps option per column family in Manifest to facilitate detecting an attempt to enable / disable UDT. This entry is enforced to be logged in the same VersionEdit as the user comparator name entry.

  2. User-defined timestamps related options are validated when re-opening a column family, including user comparator name and the persist_user_defined_timestamps flag. These type of settings and settings change are considered valid:
    a) no user comparator change and no effective persist_user_defined_timestamp flag change.
    b) switch user comparator to enable UDT provided the immediately effective persist_user_defined_timestamps flag
    is false.
    c) switch user comparator to disable UDT provided that the before-change persist_user_defined_timestamps is
    already false.

  3. when an attempt to enable UDT is detected, we mark all its existing SST files as "having no UDT" by marking its FileMetaData.user_defined_timestamps_persisted flag to false and handle their file boundaries FileMetaData.smallest, FileMetaData.largest by padding a min timestamp.

  4. while enabling / disabling UDT feature, timestamp size inconsistency in existing WAL logs are handled to make it compatible with the running user comparator.

Test plan:

make all check
./db_with_timestamp_basic_test --gtest-filter="*EnableDisableUDT*"
./db_wal_test --gtest_filter="*EnableDisableUDT*"

@facebook-github-bot
Copy link
Contributor

@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@ltamasi ltamasi left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @jowlyzhang !

db/db_impl/db_impl_open.cc Outdated Show resolved Hide resolved
db/db_wal_test.cc Show resolved Hide resolved
util/udt_util.cc Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@jowlyzhang has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@jowlyzhang has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@jowlyzhang has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@jowlyzhang merged this pull request in c24ef26.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants