Add file operation callbacks to SequentialFileReader #8982
Conversation
Summary: This change adds File IO Notifications to the SequentialFileReader and ReadAheadSequentialFileReader. The SequentialFileReader is extended with a listener parameter. Test Plan: A new test EventListenerTest::OnWALOperationTest has been added. The test verifies that during restore the sequential file reader is called and the notifications are fired. Reviewers: yanqin, rmanji Subscribers: Tasks: T75121251 Tags:
|
@shrfb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: . Test Plan: A new test EventListenerTest::OnWALOperationTest has been added. The test verifies that during restore the sequential file reader is called and the notifications are fired. Reviewers: yanqin, rmanji Subscribers: Tasks: T75121251 Tags:
|
@shrfb has updated the pull request. You must reimport the pull request before landing. |
|
@shrfb 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.
Thanks @shrfb for the PR.
In the PR description, there is a mention of "ReadAheadSequentialFileReader", but I cannot find it in the PR. Are you referring to ReadaheadSequentialFile? For this PR, adding the change just for SequentialFileReader is totally fine.
db/listener_test.cc
Outdated
| ASSERT_OK(dbfull()->Flush(FlushOptions())); | ||
| ASSERT_OK(dbfull()->TEST_WaitForFlushMemTable()); |
There was a problem hiding this comment.
By removing this Flush, we can test reading from WAL during recovery.
ReadManifestAndWALOnRecovery. Summary: This changes the name of the OnWALOperationTest to ReadManifestAndWALOnRecovery. It also removes the flush operation to also test WAL reading. Test Plan: A new test EventListenerTest::OnWALOperationTest has been added. The test verifies that during restore the sequential file reader is called and the notifications are fired. Reviewers: yanqin, rmanji Subscribers: Tasks: T75121251 Tags:
|
@shrfb has updated the pull request. You must reimport the pull request before landing. |
|
@shrfb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
db/listener_test.cc
Outdated
| } | ||
| DestroyAndReopen(options); | ||
| ASSERT_OK(Put("foo", "aaa")); | ||
| ASSERT_OK(dbfull()->TEST_WaitForFlushMemTable()); |
There was a problem hiding this comment.
Nit: this can also be removed.
file/sequence_file_reader.cc
Outdated
| #ifndef ROCKSDB_LITE | ||
| if (ShouldNotifyListeners()) { | ||
| auto finish_ts = FileOperationInfo::FinishNow(); | ||
| size_t offset = offset_.fetch_add(n); |
There was a problem hiding this comment.
The above Read can return fewer bytes than n, thus I think we should increment offset_ by result->size().
Summary: - Change ReadManifestAndWALOnRecovery to not use WaitForFlushMemTable. - Use result size in updating the offset in the sequential reader. Test Plan: iClose popup and return A new test EventListenerTest::ReadManifestAndWALOnRecovery has been added. The test verifies that during restore the sequential file reader is called and the notifications are fired. Reviewers: yanqin, rmanji Subscribers: Tasks: T75121251 Tags:
|
@shrfb has updated the pull request. You must reimport the pull request before landing. |
|
@shrfb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: This change adds File IO Notifications to the SequentialFileReader The SequentialFileReader is extended with a listener parameter. Pull Request resolved: facebook/rocksdb#8982 Test Plan: A new test EventListenerTest::OnWALOperationTest has been added. The test verifies that during restore the sequential file reader is called and the notifications are fired. Reviewed By: riversand963 Differential Revision: D31320844 Pulled By: shrfb fbshipit-source-id: 040b24da7c010d7c14ebb5c6460fae9a19b8c168
Summary:
This change adds File IO Notifications to the SequentialFileReader The SequentialFileReader is extended
with a listener parameter.
Test Plan:
A new test EventListenerTest::OnWALOperationTest has been added. The
test verifies that during restore the sequential file reader is called
and the notifications are fired.
Reviewers:
yanqin, rmanji
Subscribers:
Tasks:
T75121251
Tags: