Pass a timeout to FileSystem for random reads #6751
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
There was a problem hiding this comment.
Thanks @anand1976 for the PR.
I wonder whether the file-level API should take a ReadOptions object as argument. My feeling is that ReadOptions is for db-level read operations, since all of its current members are db-centric concepts, e.g. snapshot, key, etc.
db/db_basic_test.cc
Outdated
| std::chrono::microseconds now = | ||
| std::chrono::microseconds(Env::Default()->NowMicros()); | ||
| std::chrono::microseconds delta = std::chrono::microseconds(10); | ||
| EXPECT_GE(deadline + delta - now, opts.timeout); |
There was a problem hiding this comment.
Will this be flaky since it relies on wall time?
There was a problem hiding this comment.
I added a buffer to hopefully eliminate flakiness. Let me know if you have any better suggestions.
There was a problem hiding this comment.
iiuc, opts.timeout is computed before reaching this line according to opts.timeout = read_opts.deadline - now(). in which now() is the time calling PrepareIOFromReadOptions().
When control flow reaches here, now in this function should be larger, right? Without considering buffer (delta), should the following be true?
deadline - now < opts.timeout?
There was a problem hiding this comment.
Yes, or rather <= to be more precise. I wasn't 100% sure if now would be exactly the same if the thread gets scheduled on another cpu in between for some reason. Maybe the delta is unnecessary, but as long as opts.timeout is in the ballpark I think we're fine.
There was a problem hiding this comment.
So I guess line 2458 should be EXPECT_LE?
There was a problem hiding this comment.
nvm. I realized that I confused read_opts.deadline with fs.deadline.
file/random_access_file_reader.cc
Outdated
| NotifyOnFileReadFinish(offset + pos, tmp_result.size(), start_ts, | ||
| finish_ts, s); | ||
| } | ||
| if (ShouldNotifyListeners()) { |
anand1976
force-pushed
the
read_io_timeout
branch
from
4bebb46
to
881127e
Compare
|
@riversand963 I don't have a strong opinion about passing |
There was a problem hiding this comment.
@anand1976 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
added 3 commits
April 29, 2020 23:22
FileSystem::Read/FileSystem::MultiRead. For now, TableReader creation, whichmight do file opens and reads, are not covered. It will be implemented in another PR. Pass ReadOptions to RandomAccessFileReader::Read and MultiRead Pass timeout down to FileSystem Update tests Add some comments in tests
Summary: 1. Pass IOOptions to RandomAccessFileReader::Read and MultiRead instead of ReadOptions 2. Use SpecialEnv in unit tests to fake time
anand1976
force-pushed
the
read_io_timeout
branch
from
881127e
to
a45f154
Compare
|
@anand1976 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@anand1976 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
riversand963
approved these changes
Apr 30, 2020
| } | ||
| return FSRandomAccessFileWrapper::MultiRead(reqs, num_reqs, options, dbg); | ||
| } | ||
|
|
||
| private: | ||
| void AssertDeadline(const std::chrono::microseconds deadline, | ||
| const IOOptions& opts) const { | ||
| // Give a leeway of +- 10us as it can take some time for the Get/ |
There was a problem hiding this comment.
The comment needs to be updated.
|
@anand1976 merged this pull request in ab13d43. |
levichen94
pushed a commit
to bytedance/terarkdb
that referenced
this pull request
Jun 18, 2021
Summary: Calculate ```IOOptions::timeout``` using ```ReadOptions::deadline``` and pass it to ```FileSystem::Read/FileSystem::MultiRead```. This allows us to impose a tighter bound on the time taken by Get/MultiGet on FileSystem/Envs that support IO timeouts. Even on those that don't support, check in ```RandomAccessFileReader::Read``` and ```MultiRead``` and return ```Status::TimedOut()``` if the deadline is exceeded. For now, TableReader creation, which might do file opens and reads, are not covered. It will be implemented in another PR. Tests: Update existing unit tests to verify the correct timeout value is being passed Pull Request resolved: facebook/rocksdb#6751 Reviewed By: riversand963 Differential Revision: D21285631 Pulled By: anand1976 fbshipit-source-id: d89af843e5a91ece866e87aa29438b52a65a8567 Signed-off-by: Changlong Chen <levisonchen@live.cn>
levichen94
pushed a commit
to bytedance/terarkdb
that referenced
this pull request
Jun 18, 2021
Summary: Calculate ```IOOptions::timeout``` using ```ReadOptions::deadline``` and pass it to ```FileSystem::Read/FileSystem::MultiRead```. This allows us to impose a tighter bound on the time taken by Get/MultiGet on FileSystem/Envs that support IO timeouts. Even on those that don't support, check in ```RandomAccessFileReader::Read``` and ```MultiRead``` and return ```Status::TimedOut()``` if the deadline is exceeded. For now, TableReader creation, which might do file opens and reads, are not covered. It will be implemented in another PR. Tests: Update existing unit tests to verify the correct timeout value is being passed Pull Request resolved: facebook/rocksdb#6751 Reviewed By: riversand963 Differential Revision: D21285631 Pulled By: anand1976 fbshipit-source-id: d89af843e5a91ece866e87aa29438b52a65a8567 Signed-off-by: Changlong Chen <levisonchen@live.cn>
levichen94
pushed a commit
to bytedance/terarkdb
that referenced
this pull request
Jun 18, 2021
Summary: Calculate ```IOOptions::timeout``` using ```ReadOptions::deadline``` and pass it to ```FileSystem::Read/FileSystem::MultiRead```. This allows us to impose a tighter bound on the time taken by Get/MultiGet on FileSystem/Envs that support IO timeouts. Even on those that don't support, check in ```RandomAccessFileReader::Read``` and ```MultiRead``` and return ```Status::TimedOut()``` if the deadline is exceeded. For now, TableReader creation, which might do file opens and reads, are not covered. It will be implemented in another PR. Tests: Update existing unit tests to verify the correct timeout value is being passed Pull Request resolved: facebook/rocksdb#6751 Reviewed By: riversand963 Differential Revision: D21285631 Pulled By: anand1976 fbshipit-source-id: d89af843e5a91ece866e87aa29438b52a65a8567 Signed-off-by: Changlong Chen <levisonchen@live.cn>
mm304321141
pushed a commit
to bytedance/terarkdb
that referenced
this pull request
Jun 23, 2021
Summary: Calculate ```IOOptions::timeout``` using ```ReadOptions::deadline``` and pass it to ```FileSystem::Read/FileSystem::MultiRead```. This allows us to impose a tighter bound on the time taken by Get/MultiGet on FileSystem/Envs that support IO timeouts. Even on those that don't support, check in ```RandomAccessFileReader::Read``` and ```MultiRead``` and return ```Status::TimedOut()``` if the deadline is exceeded. For now, TableReader creation, which might do file opens and reads, are not covered. It will be implemented in another PR. Tests: Update existing unit tests to verify the correct timeout value is being passed Pull Request resolved: facebook/rocksdb#6751 Reviewed By: riversand963 Differential Revision: D21285631 Pulled By: anand1976 fbshipit-source-id: d89af843e5a91ece866e87aa29438b52a65a8567 Signed-off-by: Changlong Chen <levisonchen@live.cn>
mm304321141
pushed a commit
to bytedance/terarkdb
that referenced
this pull request
Jun 23, 2021
Summary: Calculate ```IOOptions::timeout``` using ```ReadOptions::deadline``` and pass it to ```FileSystem::Read/FileSystem::MultiRead```. This allows us to impose a tighter bound on the time taken by Get/MultiGet on FileSystem/Envs that support IO timeouts. Even on those that don't support, check in ```RandomAccessFileReader::Read``` and ```MultiRead``` and return ```Status::TimedOut()``` if the deadline is exceeded. For now, TableReader creation, which might do file opens and reads, are not covered. It will be implemented in another PR. Tests: Update existing unit tests to verify the correct timeout value is being passed Pull Request resolved: facebook/rocksdb#6751 Reviewed By: riversand963 Differential Revision: D21285631 Pulled By: anand1976 fbshipit-source-id: d89af843e5a91ece866e87aa29438b52a65a8567 Signed-off-by: Changlong Chen <levisonchen@live.cn>
levichen94
pushed a commit
to bytedance/terarkdb
that referenced
this pull request
Jun 24, 2021
Summary: Calculate ```IOOptions::timeout``` using ```ReadOptions::deadline``` and pass it to ```FileSystem::Read/FileSystem::MultiRead```. This allows us to impose a tighter bound on the time taken by Get/MultiGet on FileSystem/Envs that support IO timeouts. Even on those that don't support, check in ```RandomAccessFileReader::Read``` and ```MultiRead``` and return ```Status::TimedOut()``` if the deadline is exceeded. For now, TableReader creation, which might do file opens and reads, are not covered. It will be implemented in another PR. Tests: Update existing unit tests to verify the correct timeout value is being passed Pull Request resolved: facebook/rocksdb#6751 Reviewed By: riversand963 Differential Revision: D21285631 Pulled By: anand1976 fbshipit-source-id: d89af843e5a91ece866e87aa29438b52a65a8567 Signed-off-by: Changlong Chen <levisonchen@live.cn>
levichen94
pushed a commit
to bytedance/terarkdb
that referenced
this pull request
Sep 14, 2021
Summary: Calculate ```IOOptions::timeout``` using ```ReadOptions::deadline``` and pass it to ```FileSystem::Read/FileSystem::MultiRead```. This allows us to impose a tighter bound on the time taken by Get/MultiGet on FileSystem/Envs that support IO timeouts. Even on those that don't support, check in ```RandomAccessFileReader::Read``` and ```MultiRead``` and return ```Status::TimedOut()``` if the deadline is exceeded. For now, TableReader creation, which might do file opens and reads, are not covered. It will be implemented in another PR. Tests: Update existing unit tests to verify the correct timeout value is being passed Pull Request resolved: facebook/rocksdb#6751 Reviewed By: riversand963 Differential Revision: D21285631 Pulled By: anand1976 fbshipit-source-id: d89af843e5a91ece866e87aa29438b52a65a8567 Signed-off-by: Changlong Chen <levisonchen@live.cn>
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.
Calculate
IOOptions::timeoutusingReadOptions::deadlineand pass it toFileSystem::Read/FileSystem::MultiRead. This allows us to impose a tighter bound on the time taken by Get/MultiGet on FileSystem/Envs that support IO timeouts. Even on those that don't support, check inRandomAccessFileReader::ReadandMultiReadand returnStatus::TimedOut()if the deadline is exceeded.For now, TableReader creation, which might do file opens and reads, are not covered. It will be implemented in another PR.
Tests:
Update existing unit tests to verify the correct timeout value is being passed