Add DBOptions. avoid_unnecessary_blocking_io to defer file deletions #5043
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
@al13n321 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
anand1976
reviewed
Mar 8, 2019
There was a problem hiding this comment.
I'm a little hesitant to remove the existing ReadOptions::background_purge_on_iterator_cleanup. I do like the idea of make background_purge_on_cf_cleanup more generic. Maybe like you suggested we can change that to mean all file purges should be done in the background, and modify iterator behavior to do background purge if either of the options is set.
|
Yes, existing options cannot be easily deleted. |
|
Ok, I'll make it defer deletions if either of the settings is set. Just need a name for the setting. How about "avoid_unnecessary_blocking_io"? This way in future we can get rid of other unexpected IO, e.g. creating WAL files on write path. |
|
@al13n321 has updated the pull request. Re-import the pull request |
|
Renamed to avoid_unnecessary_blocking_io and ORed it with ReadOptions::background_purge_on_iterator_cleanup. |
al13n321
changed the title
|
Hey, I just met you and this is crazy |
|
@al13n321 has updated the pull request. Re-import the pull request |
|
@al13n321 has updated the pull request. Re-import the pull request |
anand1976
approved these changes
Apr 1, 2019
include/rocksdb/options.h
Outdated
| // If true, ColumnFamilyHandle's destructor won't delete obsolete files | ||
| // directly and will instead schedule a background job to do it. | ||
| // Use it if you're destroying ColumnFamilyHandle-s for dropped column | ||
| // families from latency-sensitive threads. |
There was a problem hiding this comment.
Nit: Can we mention the iterator behavior also in the comment?
…s from ColumnFamilyHandle's destructor to background thread
|
@al13n321 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@al13n321 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
siying
reviewed
Apr 2, 2019
| FlushOptions fo; | ||
| ColumnFamilyHandle* cfh = nullptr; | ||
|
|
||
| ASSERT_OK(db_->CreateColumnFamily(co, "dropme", &cfh)); |
There was a problem hiding this comment.
"Clang analyze" fails with this line:
db/deletefile_test.cc:289:15: warning: Called C++ object pointer is null
ASSERT_OK(db_->CreateColumnFamily(co, "dropme", &cfh));
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./util/testharness.h:39:71: note: expanded from macro 'ASSERT_OK'
#define ASSERT_OK(s) ASSERT_PRED_FORMAT1(rocksdb::test::AssertStatus, s)
^
./third-party/gtest-1.7.0/fused-src/gtest/gtest.h:20119:36: note: expanded from macro 'ASSERT_PRED_FORMAT1'
GTEST_PRED_FORMAT1_(pred_format, v1, GTEST_FATAL_FAILURE_)
^~
./third-party/gtest-1.7.0/fused-src/gtest/gtest.h:20102:34: note: expanded from macro 'GTEST_PRED_FORMAT1_'
GTEST_ASSERT_(pred_format(#v1, v1), \
^~
./third-party/gtest-1.7.0/fused-src/gtest/gtest.h:20078:52: note: expanded from macro 'GTEST_ASSERT_'
if (const ::testing::AssertionResult gtest_ar = (expression)) \
^~~~~~~~~~
1 warning generated.
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 3, 2019
Summary: This PR address two open issues: 1. clang analyzer is paranoid about db_ being nullptr after DB::Open calls in the test. See #5043 (comment) Add an assert to keep clang happy 2. PR #5049 introduced a variable shadowing: ``` db/db_iterator_test.cc: In constructor ‘rocksdb::DBIteratorWithReadCallbackTest_ReadCallback_Test::TestBody()::TestReadCallback::TestReadCallback(rocksdb::SequenceNumber)’: db/db_iterator_test.cc:2484:9: error: declaration of ‘max_visible_seq’ shadows a member of 'this' [-Werror=shadow] : ReadCallback(max_visible_seq) {} ^ ``` Pull Request resolved: #5140 Differential Revision: D14735497 Pulled By: miasantreble fbshipit-source-id: 3219ea75cf4ae04f64d889323f6779e84be98144
facebook-github-bot
pushed a commit
to facebookarchive/LogDevice
that referenced
this pull request
Apr 30, 2019
Summary: Make use of facebook/rocksdb#5043 . Need to wait for that PR to get released to fbcode to make sure version ifdef in this diff is correct. Reviewed By: gdrane Differential Revision: D14352417 fbshipit-source-id: 755fb0d3a3b897e0d0016cb8ccf7a2fd89e4450c
vagogte
pushed a commit
to vagogte/rocksdb
that referenced
this pull request
Jun 18, 2019
…acebook#5043) Summary: Just like ReadOptions::background_purge_on_iterator_cleanup but for ColumnFamilyHandle instead of Iterator. In our use case we sometimes call ColumnFamilyHandle's destructor from low-latency threads, and sometimes it blocks the thread for a few seconds deleting the files. To avoid that, we can either offload ColumnFamilyHandle's destruction to a background thread on our side, or add this option on rocksdb side. This PR does the latter, to be consistent with how we solve exactly the same problem for iterators using background_purge_on_iterator_cleanup option. (EDIT: It's avoid_unnecessary_blocking_io now, and affects both CF drops and iterator destructors.) I'm not quite comfortable with having two separate options (background_purge_on_iterator_cleanup and background_purge_on_cf_cleanup) for such a rarely used thing. Maybe we should merge them? Rename background_purge_on_cf_cleanup to something like delete_files_on_background_threads_only or avoid_blocking_io_in_unexpected_places, and make iterators use it instead of the one in ReadOptions? I can do that here if you guys think it's better. Pull Request resolved: facebook#5043 Differential Revision: D14339233 Pulled By: al13n321 fbshipit-source-id: ccf7efa11c85c9a5b91d969bb55627d0fb01e7b8
vagogte
pushed a commit
to vagogte/rocksdb
that referenced
this pull request
Jun 18, 2019
…book#5140) Summary: This PR address two open issues: 1. clang analyzer is paranoid about db_ being nullptr after DB::Open calls in the test. See facebook#5043 (comment) Add an assert to keep clang happy 2. PR facebook#5049 introduced a variable shadowing: ``` db/db_iterator_test.cc: In constructor ‘rocksdb::DBIteratorWithReadCallbackTest_ReadCallback_Test::TestBody()::TestReadCallback::TestReadCallback(rocksdb::SequenceNumber)’: db/db_iterator_test.cc:2484:9: error: declaration of ‘max_visible_seq’ shadows a member of 'this' [-Werror=shadow] : ReadCallback(max_visible_seq) {} ^ ``` Pull Request resolved: facebook#5140 Differential Revision: D14735497 Pulled By: miasantreble fbshipit-source-id: 3219ea75cf4ae04f64d889323f6779e84be98144
facebook-github-bot
pushed a commit
that referenced
this pull request
Sep 11, 2020
…7369) Summary: #3341 guaranteed that upon return of `GetSortedWalFiles` after `DisableFileDeletions`, all pending purges of previously obsolete WAL files will have finished. However, the addition of avoid_unnecessary_blocking_io in #5043 opened a hole in the code making that assurance, which can lead to files to be copied for checkpoint or backup going missing before being copied, with that option enabled. This change patches the hole. Pull Request resolved: #7369 Test Plan: apparent fix to backups in crash test observed. Will work on a unit test for another commit Reviewed By: ajkr Differential Revision: D23620258 Pulled By: pdillinger fbshipit-source-id: bea36b461a5b719c3e3ef802f967bc3e8ae71614
codingrhythm
pushed a commit
to SafetyCulture/rocksdb
that referenced
this pull request
Mar 5, 2021
…acebook#7369) Summary: facebook#3341 guaranteed that upon return of `GetSortedWalFiles` after `DisableFileDeletions`, all pending purges of previously obsolete WAL files will have finished. However, the addition of avoid_unnecessary_blocking_io in facebook#5043 opened a hole in the code making that assurance, which can lead to files to be copied for checkpoint or backup going missing before being copied, with that option enabled. This change patches the hole. Pull Request resolved: facebook#7369 Test Plan: apparent fix to backups in crash test observed. Will work on a unit test for another commit Reviewed By: ajkr Differential Revision: D23620258 Pulled By: pdillinger fbshipit-source-id: bea36b461a5b719c3e3ef802f967bc3e8ae71614
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just like ReadOptions::background_purge_on_iterator_cleanup but for ColumnFamilyHandle instead of Iterator.
In our use case we sometimes call ColumnFamilyHandle's destructor from low-latency threads, and sometimes it blocks the thread for a few seconds deleting the files. To avoid that, we can either offload ColumnFamilyHandle's destruction to a background thread on our side, or add this option on rocksdb side. This PR does the latter, to be consistent with how we solve exactly the same problem for iterators using background_purge_on_iterator_cleanup option.
(EDIT: It's avoid_unnecessary_blocking_io now, and affects both CF drops and iterator destructors.)
I'm not quite comfortable with having two separate options (background_purge_on_iterator_cleanup and background_purge_on_cf_cleanup) for such a rarely used thing. Maybe we should merge them? Rename background_purge_on_cf_cleanup to something like delete_files_on_background_threads_only or avoid_blocking_io_in_unexpected_places, and make iterators use it instead of the one in ReadOptions? I can do that here if you guys think it's better.