Make default value of options.ttl to be 30 days when it is supported. #6073
Conversation
There was a problem hiding this comment.
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
HISTORY.md
Outdated
| @@ -10,6 +10,7 @@ | |||
| * With FIFO compaction style, options.periodic_compaction_seconds will have the same meaning as options.ttl. Whichever stricter will be used. With the default options.periodic_compaction_seconds value with options.ttl's default of 0, RocksDB will give a default of 30 days. | |||
| * Added an API GetCreationTimeOfOldestFile(uint64_t* creation_time) to get the file_creation_time of the oldest SST file in the DB. | |||
| * An unlikely usage of FilterPolicy is no longer supported. Calling GetFilterBitsBuilder() on the FilterPolicy returned by NewBloomFilterPolicy will now cause an assertion violation in debug builds, because RocksDB has internally migrated to a more elaborate interface that is expected to evolve further. Custom implementations of FilterPolicy should work as before, except those wrapping the return of NewBloomFilterPolicy, which will require a new override of a protected function in FilterPolicy. | |||
| * In cases where TTL is supported, make the default value 30 days. To revert the old behavior, you can explictly set it to 0. | |||
There was a problem hiding this comment.
I think you should mention the default value as UINT64_MAX-1 (or 0xfffffffffffffffe) instead of 30 days here. And that RocksDB will internally convert it to 30 days wherever TTL is supported.
| @@ -655,11 +655,15 @@ struct AdvancedColumnFamilyOptions { | |||
| // unit: seconds. Ex: 1 day = 1 * 24 * 60 * 60 | |||
| // In FIFO, this option will have the same meaning as | |||
| // periodic_compaction_seconds. Whichever stricter will be used. | |||
| // 0 means disabling. | |||
| // UINT64_MAX - 1 (0xfffffffffffffffe) is special flag to allow RocksDB to | |||
There was a problem hiding this comment.
why not just use UINT64_MAX?
Are you concerned that it might collide somewhere with the default for periodic_compaction_seconds?
There was a problem hiding this comment.
I'm concerned that people might already use UINT64_MAX to indicate infinite. It's less likely to use UINT64_MAX - 1.
There was a problem hiding this comment.
I don't think so. But I don't have a strong preference for either of these values (i.e. UINT64_MAX vs UINT64_MAX - 1), so I'll let you decide the best value.
If that is indeed the case may be it would be good to set the default value of periodic_compaction_seconds also to be this value, just to match.
There was a problem hiding this comment.
Actually I tried and oddly most tests failed. No idea why so I gave it up for now. Will give it another try when I have time.
There was a problem hiding this comment.
Should we change the default for periodic_compaction_seconds or leave it as is?
There was a problem hiding this comment.
I think I might know the issue, as I ran into something similar. You might need to change some code in version_set.cc similar to PR #5865 . Just a guess without looking at the logs ... might or might not work.
There was a problem hiding this comment.
Hmm.. This is odd. It means in somewhere of the code, the option came to there without being sanitized?
There was a problem hiding this comment.
I think I might have figured out the reason. Will send out a separate PR.
There was a problem hiding this comment.
Let me do it in the same PR as there will be conflicts.
There was a problem hiding this comment.
Still some test faiures...
|
@siying has updated the pull request. Re-import the pull request |
4 similar comments
|
@siying has updated the pull request. Re-import the pull request |
|
@siying has updated the pull request. Re-import the pull request |
|
@siying has updated the pull request. Re-import the pull request |
|
@siying has updated the pull request. Re-import the pull request |
Summary: By default options.ttl is disabled. We believe a better default will be 30 days, which means deleted data the database will be removed from SST files slightly after 30 days, for most of the cases. Test Plan: Add unit tests for it.
|
@siying has updated the pull request. Re-import the pull request |
|
@siying has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@siying has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
This pull request has been merged in 77eab5c. |
* upstream/master: (572 commits) Work around weird unused errors with Mingw (facebook#6075) Support options.max_open_files = -1 with periodic_compaction_seconds (facebook#6090) Fix HISTORY.md for 6.6.0 (facebook#6096) Expose and elaborate FilterBuildingContext (facebook#6088) Fix compilation under MSVC VS2015 (facebook#6081) Add shared library for musl-libc (facebook#3143) Refactor and clean up the code that reads a blob from a file (facebook#6093) Allow fractional bits/key in BloomFilterPolicy (facebook#6092) Refactor blob file creation logic (facebook#6066) Use lowercase for shlwapi.lib rpcrt4.lib (facebook#6076) Fix naming of library on PPC64LE (facebook#6080) Small improvements to Docker build for RocksJava (facebook#6079) Remove unused/undefined ImmutableCFOptions() (facebook#6086) Update 3rd-party libraries used by RocksJava (facebook#6084) Make default value of options.ttl to be 30 days when it is supported. (facebook#6073) Ignore value of BackupableDBOptions::max_valid_backups_to_open when B… (facebook#6072) Update HISTORY.md for forward compatibility (facebook#6085) Support ttl in Universal Compaction (facebook#6071) Fix the constness issues around autovector::iterator_impl's dereference operators (facebook#6057) Support options.ttl with options.max_open_files = -1 (facebook#6060) ...
…facebook#6073) Summary: By default options.ttl is disabled. We believe a better default will be 30 days, which means deleted data the database will be removed from SST files slightly after 30 days, for most of the cases. Make the default UINT64_MAX - 1 to indicate that it is not overridden by users. Change periodic_compaction_seconds to be UINT64_MAX - 1 to UINT64_MAX too to be consistent. Also fix a small bug in the previous periodic_compaction_seconds default code. Pull Request resolved: facebook#6073 Test Plan: Add unit tests for it. Differential Revision: D18669626 fbshipit-source-id: 957cd4374cafc1557d45a0ba002010552a378cc8
Summary: By default options.ttl is disabled. We believe a better default will be 30 days, which means deleted data the database will be removed from SST files slightly after 30 days, for most of the cases.
Make the default UINT64_MAX - 1 to indicate that it is not overridden by users.
Change periodic_compaction_seconds to be UINT64_MAX - 1 to UINT64_MAX too to be consistent. Also fix a small bug in the previous periodic_compaction_seconds default code.
Test Plan: Add unit tests for it.