Fix CompactFiles bug
#4665
Closed
Fix CompactFiles bug
#4665
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
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
ajkr
reviewed
Nov 12, 2018
| mutex_.Unlock(); | ||
| delete sv; | ||
|
|
||
| current->Unref(); |
There was a problem hiding this comment.
can this release the last reference? In that case we'd need to delete it.
There was a problem hiding this comment.
oh never mind, Version::Unref does it for us. I'm not sure why it returns a bool then..
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
ajkr
approved these changes
Nov 12, 2018
Closed
facebook-github-bot
pushed a commit
that referenced
this pull request
Nov 13, 2018
Summary: Our internal CI test caught RocksDB Lite build failures. The failures are due to a new test introduced in #4665 using `SSTFileWriter` and `IngestExternalFile`, but these is not exposed under lite mode. Fixed by #ifdef'ing out the test. ``` db/db_test2.cc: In member function ‘virtual void rocksdb::DBTest2_TestCompactFiles_Test::TestBody()’: db/db_test2.cc:2907:3: error: ‘SstFileWriter’ is not a member of ‘rocksdb’ rocksdb::SstFileWriter sst_file_writer{rocksdb::EnvOptions(), options}; ^ In file included from ./util/testharness.h:15:0, from ./table/mock_table.h:23, from ./db/db_test_util.h:44, from db/db_test2.cc:13: db/db_test2.cc:2912:13: error: ‘sst_file_writer’ was not declared in this scope ASSERT_OK(sst_file_writer.Open(external_file1)); ``` Pull Request resolved: #4675 Differential Revision: D13035984 Pulled By: sagar0 fbshipit-source-id: c1ceac550dfac1a85eeea436693dc7dd467519a6
DorianZheng
added a commit
to DorianZheng/rocksdb
that referenced
this pull request
Nov 13, 2018
Summary:
`CompactFiles` gets `SuperVersion` before `WaitForIngestFile`, while `IngestExternalFile` may add files that overlap with `input_file_names`
The timeline of execution flow is as follow:
Let's say that level N has two file [1,2] and [5,6]
```
timeline user_thread1 user_thread2
t0 | CompactFiles([1, 2], [5, 6]) begin
t1 | GetReferencedSuperVersion()
t2 | IngestExternalFile([3,4]) to level N begin
t3 | CompactFiles resume
V
```
Pull Request resolved: facebook#4665
Differential Revision: D13030674
Pulled By: ajkr
fbshipit-source-id: 8be19477fd6e505032267a979d32f3097cc3be51
(cherry picked from commit 0f88160)
DorianZheng
pushed a commit
to DorianZheng/rocksdb
that referenced
this pull request
Nov 13, 2018
Summary: Our internal CI test caught RocksDB Lite build failures. The failures are due to a new test introduced in facebook#4665 using `SSTFileWriter` and `IngestExternalFile`, but these is not exposed under lite mode. Fixed by #ifdef'ing out the test. ``` db/db_test2.cc: In member function ‘virtual void rocksdb::DBTest2_TestCompactFiles_Test::TestBody()’: db/db_test2.cc:2907:3: error: ‘SstFileWriter’ is not a member of ‘rocksdb’ rocksdb::SstFileWriter sst_file_writer{rocksdb::EnvOptions(), options}; ^ In file included from ./util/testharness.h:15:0, from ./table/mock_table.h:23, from ./db/db_test_util.h:44, from db/db_test2.cc:13: db/db_test2.cc:2912:13: error: ‘sst_file_writer’ was not declared in this scope ASSERT_OK(sst_file_writer.Open(external_file1)); ``` Pull Request resolved: facebook#4675 Differential Revision: D13035984 Pulled By: sagar0 fbshipit-source-id: c1ceac550dfac1a85eeea436693dc7dd467519a6 (cherry picked from commit 2993cd2)
huachaohuang
pushed a commit
to tikv/rocksdb
that referenced
this pull request
Nov 13, 2018
* [cherry-pick] Fix DBImpl::GetColumnFamilyHandleUnlocked race condition (facebook#4391) Summary: - Fix DBImpl API race condition The timeline of execution flow is as follow: ``` timeline user_thread1 user_thread2 t1 | cfh = GetColumnFamilyHandleUnlocked(0) t2 | id1 = cfh->GetID() t3 | GetColumnFamilyHandleUnlocked(1) t4 | id2 = cfh->GetID() V ``` The original implementation return a pointer to a stateful variable, so that the return `ColumnFamilyHandle` will be changed when another thread calls `GetColumnFamilyHandleUnlocked` with different `column family id` - Expose ColumnFamily ID to compaction event listener - Fix the return status of `DBImpl::GetLatestSequenceForKey` Pull Request resolved: facebook#4391 Differential Revision: D10221243 Pulled By: yiwu-arbug fbshipit-source-id: dec60ee9ff0c8261a2f2413a8506ec1063991993 # Conflicts: # db/db_test2.cc * Fix `DBImpl::GetColumnFamilyHandleUnlocked` data race (facebook#4666) Summary: Hi, yiwu-arbug, I found that `DBImpl::GetColumnFamilyHandleUnlocked` still have data race condition, because `column_family_memtables_` has a stateful cache `current_` and `column_family_memtables_::Seek` maybe call without the protection of `mutex_` by a write thread check https://github.com/facebook/rocksdb/blob/859dbda6e3cac17416aff48f1760d01707867351/db/write_batch.cc#L1188 and https://github.com/facebook/rocksdb/blob/859dbda6e3cac17416aff48f1760d01707867351/db/write_batch.cc#L1756 and https://github.com/facebook/rocksdb/blob/859dbda6e3cac17416aff48f1760d01707867351/db/db_impl_write.cc#L318 So it's better to use `versions_->GetColumnFamilySet()->GetColumnFamily` instead. Pull Request resolved: facebook#4666 Differential Revision: D13027117 Pulled By: yiwu-arbug fbshipit-source-id: 4e3778eaf8e7f7c8577bbd78129b6a5fd7ce79fb (cherry picked from commit 09426ae) * Fix `CompactFiles` bug (facebook#4665) Summary: `CompactFiles` gets `SuperVersion` before `WaitForIngestFile`, while `IngestExternalFile` may add files that overlap with `input_file_names` The timeline of execution flow is as follow: Let's say that level N has two file [1,2] and [5,6] ``` timeline user_thread1 user_thread2 t0 | CompactFiles([1, 2], [5, 6]) begin t1 | GetReferencedSuperVersion() t2 | IngestExternalFile([3,4]) to level N begin t3 | CompactFiles resume V ``` Pull Request resolved: facebook#4665 Differential Revision: D13030674 Pulled By: ajkr fbshipit-source-id: 8be19477fd6e505032267a979d32f3097cc3be51 (cherry picked from commit 0f88160) * Fix RocksDB Lite build (facebook#4675) Summary: Our internal CI test caught RocksDB Lite build failures. The failures are due to a new test introduced in facebook#4665 using `SSTFileWriter` and `IngestExternalFile`, but these is not exposed under lite mode. Fixed by #ifdef'ing out the test. ``` db/db_test2.cc: In member function ‘virtual void rocksdb::DBTest2_TestCompactFiles_Test::TestBody()’: db/db_test2.cc:2907:3: error: ‘SstFileWriter’ is not a member of ‘rocksdb’ rocksdb::SstFileWriter sst_file_writer{rocksdb::EnvOptions(), options}; ^ In file included from ./util/testharness.h:15:0, from ./table/mock_table.h:23, from ./db/db_test_util.h:44, from db/db_test2.cc:13: db/db_test2.cc:2912:13: error: ‘sst_file_writer’ was not declared in this scope ASSERT_OK(sst_file_writer.Open(external_file1)); ``` Pull Request resolved: facebook#4675 Differential Revision: D13035984 Pulled By: sagar0 fbshipit-source-id: c1ceac550dfac1a85eeea436693dc7dd467519a6 (cherry picked from commit 2993cd2)
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.
CompactFilesgetsSuperVersionbeforeWaitForIngestFile, whileIngestExternalFilemay add files that overlap withinput_file_namesThe timeline of execution flow is as follow:
Let's say that level N has two file [1,2] and [5,6]