Carry over min_log_number_to_keep_2pc in new MANIFEST #7747
Conversation
ghost
requested a review
from 
There was a problem hiding this comment.
@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| @@ -5278,6 +5278,14 @@ Status VersionSet::WriteCurrentStateToManifest( | |||
| assert(iter != curr_state.end()); | |||
| uint64_t log_number = iter->second.log_number; | |||
| edit.SetLogNumber(log_number); | |||
|
|
|||
| if (cfd->GetID() == 0) { | |||
There was a problem hiding this comment.
Why does it only apply to default CF?
There was a problem hiding this comment.
min_log_number_to_keep is for the whole db, not for specific column family.
So it does not need to be set for every column family, just need to be set once.
Since default CF can never be dropped, we set the min_log to the default CF here.
There was a problem hiding this comment.
maybe add a comment for that?
| @@ -5278,6 +5278,14 @@ Status VersionSet::WriteCurrentStateToManifest( | |||
| assert(iter != curr_state.end()); | |||
| uint64_t log_number = iter->second.log_number; | |||
| edit.SetLogNumber(log_number); | |||
|
|
|||
| if (cfd->GetID() == 0) { | |||
There was a problem hiding this comment.
maybe add a comment for that?
|
@cheng-chang has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
|
@cheng-chang has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
@Cheng-Chang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@cheng-chang has updated the pull request. You must reimport the pull request before landing. |
|
@Cheng-Chang merged this pull request in 80159f6. |
Summary: When two phase commit is enabled, `VersionSet::min_log_number_to_keep_2pc` is set during flush. But when a new MANIFEST is created, the `min_log_number_to_keep_2pc` is not carried over to the new MANIFEST. So if a new MANIFEST is created and then DB is reopened, the `min_log_number_to_keep_2pc` will be lost. This may cause DB recovery errors. The bug is reproduced in a new unit test in `version_set_test.cc`. Pull Request resolved: facebook#7747 Test Plan: The new unit test in `version_set_test.cc` should pass. Reviewed By: jay-zhuang Differential Revision: D25350661 Pulled By: cheng-chang fbshipit-source-id: eee890d5b19f15769069670692e270ae31044ece
When two phase commit is enabled,
VersionSet::min_log_number_to_keep_2pcis set during flush.But when a new MANIFEST is created, the
min_log_number_to_keep_2pcis not carried over to the new MANIFEST. So if a new MANIFEST is created and then DB is reopened, themin_log_number_to_keep_2pcwill be lost. This may cause DB recovery errors.The bug is reproduced in a new unit test in
version_set_test.cc.Test Plan:
The new unit test in
version_set_test.ccshould pass.