Make secondary instance use ManifestTailer #7998
Conversation
|
cc @jay-zhuang |
| uint64_t ReactiveVersionSet::TEST_read_edits_in_atomic_group() const { | ||
| assert(manifest_tailer_); | ||
| return manifest_tailer_->GetReadBuffer().TEST_read_edits_in_atomic_group(); | ||
| } |
There was a problem hiding this comment.
Should this be defined within:
#ifndef NDEBUG
...
#endif
db/version_edit_handler.cc
Outdated
|
|
||
| Version* dummy_version = tmp_cfd->dummy_versions(); | ||
| assert(dummy_version); | ||
| Version* base_version = dummy_version->TEST_Next(); |
There was a problem hiding this comment.
Should TEST_Next() only be used for debug code?
There was a problem hiding this comment.
It should. I should probably replace TEST_Next() with Next() and see if it will cause additional irrelevant changes.
There was a problem hiding this comment.
will it expose too much internal information of version?
There was a problem hiding this comment.
It will, but I am not too worried here because Version itself is not public. It's defined in db and not exposed to application. Do you have a case in mind that will be risky?
| // For now, ignore new column families created after Recover() succeeds. | ||
| return Status::OK(); | ||
| } | ||
| auto builder_iter = builders_.find(edit.GetColumnFamily()); |
There was a problem hiding this comment.
In which condition, will it reach here? Should not new CF only be handled in Recovery mode (which is handled by VersionEditHanlder::OnColumnFamilyAdd())?
There was a problem hiding this comment.
If the primary later creates a new column family, it will write a VersionEdit to the MANIFEST. The secondary, whether in Recovery mode or Catchup mode, will be able to see this VersionEdit.
If the column family is added by the primary after the secondary is created and the secondary does not specify it in its Open call, then we ignore it. Should the secondary be interested in this column family, it should be opened with this column family in the first place. This is aligned with how we call DB::Open() with column families.
For column families that are specified when creating the secondary, we will see VersionEdit.is_column_family_add == true for them multiple times, whether secondary is in Recovery or Catchup mode. In fact, the primary will write a VersionEdit to add these column families each time it switches to a new MANIFEST.
Maybe an example can help.
MANIFEST-1 : | 'add_cf_1' | 'cf1': 'add [2.sst]' | 'cf1': 'add [3.sst]' |
MANIFEST-4 : |'add_cf_1' | 'cf1': 'add [2.sst,3.sst]' |
The code snippet here handles the case when the add_cf_1 in MANIFEST-4 is seen. Suppose at this time, the secondary instance sees the second version edit of MANIFEST-1 but for some reason switches to MANIFEST-4. We need to build the version storage for cf1 from the empty base version of cf1, but from the version with 2.sst, otherwise the consistency check will fail due to two occurences of 2.sst.
There was a problem hiding this comment.
Thanks for the explanation.
|
Thanks @jay-zhuang for the review |
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
riversand963
force-pushed
the
manifest-tailer
branch
from
ec3557e
to
7c0405f
Compare
|
@riversand963 has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@riversand963 merged this pull request in 64517d1. |
This PR
ManifestTailerthat inherits fromVersionEditHandlerPointInTime.ManifestTailer::Iterate()can be called multiple times to tail the primary instance's MANIFEST and apply the changes to the secondary,ReactiveVersionSet::ReadAndApplyto use this class,Test plan:
make check
Existing and newly-added tests in version_set_test.cc and db_secondary_test.cc