Do readahead in VerifyChecksum() #5713
Conversation
| @@ -2585,10 +2585,10 @@ class ModelDB : public DB { | |||
| return Status::NotSupported("Not implemented."); | |||
| } | |||
|
|
|||
| Status VerifyChecksum() override { | |||
There was a problem hiding this comment.
should keep this version as well?
There was a problem hiding this comment.
The default implementation of DB::VerifyChecksum() will call the other implementation. Also, this is unit test code. If the test code doesn't use the function, I don't think we should worry about it.
There was a problem hiding this comment.
Interestingly, Windows complains about it. Let me add it back.
There was a problem hiding this comment.
Not Windows, Travis.
There was a problem hiding this comment.
@siying 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.
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: Right now VerifyChecksum() doesn't do read-ahead. In some use cases, users won't be able to achieve good performance. With this change, by default, RocksDB will do a default readahead, and users will be able to overwrite the readahead size by passing in a ReadOptions. Test Plan: Add a new unit test. Fix db_test build issue in GCC. Fix the bug.
siying
force-pushed
the
checksum_readahead
branch
from
5e40c70
to
e92c293
Compare
|
@siying has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@siying has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
This pull request has been merged in e1c468d. |
Summary: Right now VerifyChecksum() doesn't do read-ahead. In some use cases, users won't be able to achieve good performance. With this change, by default, RocksDB will do a default readahead, and users will be able to overwrite the readahead size by passing in a ReadOptions. Pull Request resolved: facebook#5713 Test Plan: Add a new unit test. Differential Revision: D16860874 fbshipit-source-id: 0cff0fe79ac855d3d068e6ccd770770854a68413
Summary:
Right now VerifyChecksum() doesn't do read-ahead. In some use cases, users won't be able to achieve good performance. With this change, by default, RocksDB will do a default readahead, and users will be able to overwrite the readahead size by passing in a ReadOptions.
Test Plan: Add a new unit test.