Add support for MANIFEST tailing #4710
Closed
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
riversand963
force-pushed
the
ro_db_manifest
branch
2 times, most recently
from
d620c76 to
376391c
Compare
riversand963
force-pushed
the
ro_db_manifest
branch
from
376391c to
f234600
Compare
riversand963
force-pushed
the
ro_db_manifest
branch
8 times, most recently
from
7c909c7 to
97c83a3
Compare
riversand963
changed the title
siying
reviewed
Dec 26, 2018
db/db_impl_secondary.cc
Outdated
| tracer_->Get(column_family, key); | ||
| } | ||
| } | ||
| SuperVersion* super_version = cfd->GetSuperVersion(); |
There was a problem hiding this comment.
Can this SuperVersion go away later? Do we need to do the thread-local super version trick?
There was a problem hiding this comment.
You are right. If the secondary calls InstallSuperVersion (in another thread), this superversion can go away.
db/db_impl_secondary.cc
Outdated
| ColumnFamilyHandle* column_family) { | ||
| auto cfh = reinterpret_cast<ColumnFamilyHandleImpl*>(column_family); | ||
| auto cfd = cfh->cfd(); | ||
| SuperVersion* super_version = cfd->GetSuperVersion()->Ref(); |
There was a problem hiding this comment.
Is there a data race here? Do we need to do the thread-local super version trick?
There was a problem hiding this comment.
You are right. Let me take a look.
| return Status::OK(); | ||
| } | ||
|
|
||
| Status DBImplSecondary::TryCatchUpWithPrimary() { |
There was a problem hiding this comment.
Who is supposed to call this function? I didn't find it called, except in a unit test.
There was a problem hiding this comment.
Currently it's called only in unit test (#4820). I'm thinking about adding a separate thread calling this function in a future PR.
| // It's possible that we have finished reading the current MANIFEST, and | ||
| // the primary has created a new MANIFEST. | ||
| log::Reader::Reporter* reporter = reader->GetReporter(); | ||
| s = MaybeSwitchManifest(reporter, manifest_reader); |
There was a problem hiding this comment.
Wow, this is complicated... If you asked me before, I would say we don't support manifest switch for now. Anyway since it is already implemented...
There was a problem hiding this comment.
I see. Let's keep it for now.
| // the primary has created a new MANIFEST. | ||
| log::Reader::Reporter* reporter = reader->GetReporter(); | ||
| s = MaybeSwitchManifest(reporter, manifest_reader); | ||
| reader = manifest_reader->get(); |
There was a problem hiding this comment.
Hmm, so far I didn't see how this safely handles manfiest switching. Handling a new manifest, we need to start with empty Version. Maybe I miss something but I don't see it is the way handled.
There was a problem hiding this comment.
We do not keep Versions across different MANIFESTs, I assume you are referring to VersionBuilders which actually save (buffer) the changes of one or multiple VersionEdits?
There was a problem hiding this comment.
Discussed with @siying offline. For this PR, we are not going to worry about MANIFEST switching.
riversand963
force-pushed
the
ro_db_manifest
branch
3 times, most recently
from
78fa5b2 to
c750120
Compare
riversand963
force-pushed
the
ro_db_manifest
branch
from
c750120 to
98818a5
Compare
Test plan (to be updated) ``` $make clean && make -j32 all check $./db_secondary_test ``` All tests must pass.
riversand963
force-pushed
the
ro_db_manifest
branch
from
98818a5 to
2060fed
Compare
|
Moved to #4899 |
facebook-github-bot
pushed a commit
that referenced
this pull request
Mar 26, 2019
Summary: This PR allows RocksDB to run in single-primary, multi-secondary process mode. The writer is a regular RocksDB (e.g. an `DBImpl`) instance playing the role of a primary. Multiple `DBImplSecondary` processes (secondaries) share the same set of SST files, MANIFEST, WAL files with the primary. Secondaries tail the MANIFEST of the primary and apply updates to their own in-memory state of the file system, e.g. `VersionStorageInfo`. This PR has several components: 1. (Originally in #4745). Add a `PathNotFound` subcode to `IOError` to denote the failure when a secondary tries to open a file which has been deleted by the primary. 2. (Similar to #4602). Add `FragmentBufferedReader` to handle partially-read, trailing record at the end of a log from where future read can continue. 3. (Originally in #4710 and #4820). Add implementation of the secondary, i.e. `DBImplSecondary`. 3.1 Tail the primary's MANIFEST during recovery. 3.2 Tail the primary's MANIFEST during normal processing by calling `ReadAndApply`. 3.3 Tailing WAL will be in a future PR. 4. Add an example in 'examples/multi_processes_example.cc' to demonstrate the usage of secondary RocksDB instance in a multi-process setting. Instructions to run the example can be found at the beginning of the source code. Pull Request resolved: #4899 Differential Revision: D14510945 Pulled By: riversand963 fbshipit-source-id: 4ac1c5693e6012ad23f7b4b42d3c374fecbe8886
jwasinger
pushed a commit
to jwasinger/rocksdb
that referenced
this pull request
Apr 1, 2019
Summary: This PR allows RocksDB to run in single-primary, multi-secondary process mode. The writer is a regular RocksDB (e.g. an `DBImpl`) instance playing the role of a primary. Multiple `DBImplSecondary` processes (secondaries) share the same set of SST files, MANIFEST, WAL files with the primary. Secondaries tail the MANIFEST of the primary and apply updates to their own in-memory state of the file system, e.g. `VersionStorageInfo`. This PR has several components: 1. (Originally in facebook#4745). Add a `PathNotFound` subcode to `IOError` to denote the failure when a secondary tries to open a file which has been deleted by the primary. 2. (Similar to facebook#4602). Add `FragmentBufferedReader` to handle partially-read, trailing record at the end of a log from where future read can continue. 3. (Originally in facebook#4710 and facebook#4820). Add implementation of the secondary, i.e. `DBImplSecondary`. 3.1 Tail the primary's MANIFEST during recovery. 3.2 Tail the primary's MANIFEST during normal processing by calling `ReadAndApply`. 3.3 Tailing WAL will be in a future PR. 4. Add an example in 'examples/multi_processes_example.cc' to demonstrate the usage of secondary RocksDB instance in a multi-process setting. Instructions to run the example can be found at the beginning of the source code. Pull Request resolved: facebook#4899 Differential Revision: D14510945 Pulled By: riversand963 fbshipit-source-id: 4ac1c5693e6012ad23f7b4b42d3c374fecbe8886
vagogte
pushed a commit
to vagogte/rocksdb
that referenced
this pull request
Jun 18, 2019
Summary: This PR allows RocksDB to run in single-primary, multi-secondary process mode. The writer is a regular RocksDB (e.g. an `DBImpl`) instance playing the role of a primary. Multiple `DBImplSecondary` processes (secondaries) share the same set of SST files, MANIFEST, WAL files with the primary. Secondaries tail the MANIFEST of the primary and apply updates to their own in-memory state of the file system, e.g. `VersionStorageInfo`. This PR has several components: 1. (Originally in facebook#4745). Add a `PathNotFound` subcode to `IOError` to denote the failure when a secondary tries to open a file which has been deleted by the primary. 2. (Similar to facebook#4602). Add `FragmentBufferedReader` to handle partially-read, trailing record at the end of a log from where future read can continue. 3. (Originally in facebook#4710 and facebook#4820). Add implementation of the secondary, i.e. `DBImplSecondary`. 3.1 Tail the primary's MANIFEST during recovery. 3.2 Tail the primary's MANIFEST during normal processing by calling `ReadAndApply`. 3.3 Tailing WAL will be in a future PR. 4. Add an example in 'examples/multi_processes_example.cc' to demonstrate the usage of secondary RocksDB instance in a multi-process setting. Instructions to run the example can be found at the beginning of the source code. Pull Request resolved: facebook#4899 Differential Revision: D14510945 Pulled By: riversand963 fbshipit-source-id: 4ac1c5693e6012ad23f7b4b42d3c374fecbe8886
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.
This depends on #4602 and #4745
Test plan (to be updated)
All tests much pass.