Fix MANIFEST name assignment #6426
Conversation
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.
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
fix-new-manifest-name
branch
from
c14ee98
to
586d87a
Compare
|
@riversand963 has updated the pull request. Re-import the pull request |
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.
There was a problem hiding this comment.
The change itself is OK. Can you explain more about whether it is a user facing bug or not? If it is a user facing bug, what will be the impact?
Is it a regression bug related to the out-of-space recovery fix? If that is the case, do we need to backport to 6.7 branch?
HISTORY.md
Outdated
| @@ -12,6 +12,7 @@ | |||
| * BlobDB now ignores trivially moved files when updating the mapping between blob files and SSTs. This should mitigate issue #6338 where out of order flush/compaction notifications could trigger an assertion with the earlier code. | |||
| * Batched MultiGet() ignores IO errors while reading data blocks, causing it to potentially continue looking for a key and returning stale results. | |||
| * `WriteBatchWithIndex::DeleteRange` returns `Status::NotSupported`. Previously it returned success even though reads on the batch did not account for range tombstones. The corresponding language bindings now cannot be used. In C, that includes `rocksdb_writebatch_wi_delete_range`, `rocksdb_writebatch_wi_delete_range_cf`, `rocksdb_writebatch_wi_delete_rangev`, and `rocksdb_writebatch_wi_delete_rangev_cf`. In Java, that includes `WriteBatchWithIndex::deleteRange`. | |||
| * Assign new MANIFEST file number when caller tries to create a new MANIFEST by calling LogAndApply(..., new_descriptor_log=true). | |||
There was a problem hiding this comment.
I don't think LogAndApply() is a public API. Can you explain from RocksDB users' point of the view, how the bug can be triggered?
|
CC @anand1976 |
|
The change looks fine to me. I agree with @siying that the description needs to be updated to say when this can happen from the user perspective. I don't think its related to the out of space recovery though, as background ops call |
riversand963
force-pushed
the
fix-new-manifest-name
branch
from
586d87a
to
3cd6eb0
Compare
|
Thanks @siying and @anand1976 for the review. I have updated the PR description and HISTORY.md. |
|
A backport may be necessary. |
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.
Summary: Currently, a new MANIFEST file is assigned a new file number when 1) no MANIFEST is open, or 2) current MANIFEST file size exceeds a threshold. This is not sufficient. There are cases when the caller explicitly specifies that a new MANIFEST be created. Test Plan: make check
riversand963
force-pushed
the
fix-new-manifest-name
branch
from
3cd6eb0
to
e600fba
Compare
|
@riversand963 has updated the pull request. Re-import the pull request |
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.
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.
|
The appveyor failure is irrelevant. |
|
@riversand963 merged this pull request in 362b8d4. |
Summary:
Currently, a new MANIFEST file is assigned a new file number when 1) no
MANIFEST is open, or 2) current MANIFEST file size exceeds a threshold. This is
not sufficient. There are cases when the caller explicitly specifies that a new
MANIFEST be created. For example, if user sets options.write_dbid_to_manifest = true,
and there are WAL files, then RocksDB will run into an issue during recovery.
DBImpl::Recover()will callLogAndApply()to write dbid. At this point, the db beingrecovered creates a new MANIFEST, say, MANIFEST-000003. Since there are WALs,
DBImpl::RecoverLogFileswill be called. Towards the end of this function, we callLogAndApply(new_descriptor_log=true), which explicitly creates a new MANIFEST.However, the manifest_file_number is wrong before this fix. Consequently, RocksDB
opens an existing, non-empty file for append, effectively truncating the file to zero.
If a crash occurs, then there will be data loss.
Test Plan (devserver):
make check