Add logs and stats in DeleteScheduler #6927
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.
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
akankshamahajan15
force-pushed
the
tech-debt_task
branch
from
46fe435
to
af4a91a
Compare
There was a problem hiding this comment.
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
riversand963
reviewed
Jun 3, 2020
There was a problem hiding this comment.
LGTM. Thanks @akankshamahajan15 for working on this. Left a few minor comments.
file/delete_scheduler_test.cc
Outdated
| @@ -318,8 +330,9 @@ TEST_F(DeleteSchedulerTest, DisableRateLimiting) { | |||
|
|
|||
| rate_bytes_per_sec_ = 0; | |||
| NewDeleteScheduler(); | |||
| int num_files = 10; | |||
There was a problem hiding this comment.
Nit:
constexpr int num_files = 10;
file/delete_scheduler.h
Outdated
| @@ -83,6 +83,8 @@ class DeleteScheduler { | |||
| static Status CleanupDirectory(Env* env, SstFileManagerImpl* sfm, | |||
| const std::string& path); | |||
|
|
|||
| void SetStatisticsPtr(std::shared_ptr<Statistics> stats) { stats_ = stats; } | |||
There was a problem hiding this comment.
I believe the type of the argument can be const std::shared_ptr<Statistics>&.
file/sst_file_manager_impl.h
Outdated
| @@ -135,6 +135,11 @@ class SstFileManagerImpl : public SstFileManager { | |||
| // once in the object's lifetime, and before the destructor | |||
| void Close(); | |||
|
|
|||
| void SetStatisticsPtr(std::shared_ptr<Statistics> stats) { | |||
There was a problem hiding this comment.
const std::shared_ptr<Statistics>&.
| @@ -65,24 +65,36 @@ Status DeleteScheduler::DeleteFile(const std::string& file_path, | |||
| s = fs_->DeleteFile(file_path, IOOptions(), nullptr); | |||
| if (s.ok()) { | |||
| sst_file_manager_->OnDeleteFile(file_path); | |||
| ROCKS_LOG_INFO(info_log_, | |||
There was a problem hiding this comment.
Maybe we can print a message during db start, similar to the following:
SstFileManager instance: <addr in hex>
file/delete_scheduler.cc
Outdated
| while (!closing_ && !cv_.TimedWait(start_time + total_penlty)) {} | ||
| ROCKS_LOG_INFO( | ||
| info_log_, | ||
| "Rate limiting is enabled with penalty %lu after deleting file %s", |
There was a problem hiding this comment.
We use PRIi64, PRIu64 and ROCKSDB_PRIszt to print 64-bit signed integers, 64-bit unsigned, size_t-reps.
akankshamahajan15
force-pushed
the
tech-debt_task
branch
from
af4a91a
to
54e7c61
Compare
|
@akankshamahajan15 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
Addressed comments |
riversand963
reviewed
Jun 4, 2020
file/delete_scheduler.cc
Outdated
| @@ -39,6 +40,7 @@ DeleteScheduler::DeleteScheduler(Env* env, FileSystem* fs, | |||
| assert(sst_file_manager != nullptr); | |||
| assert(max_trash_db_ratio >= 0); | |||
| MaybeCreateBackgroundThread(); | |||
| ROCKS_LOG_INFO(info_log_, "SstFileManager instance: %p", sst_file_manager); | |||
There was a problem hiding this comment.
Sorry for the previous confusion. Maybe move this to DBImpl::Open()?
| @@ -135,6 +135,11 @@ class SstFileManagerImpl : public SstFileManager { | |||
| // once in the object's lifetime, and before the destructor | |||
| void Close(); | |||
|
|
|||
| void SetStatisticsPtr(const std::shared_ptr<Statistics>& stats) override { | |||
There was a problem hiding this comment.
We should call this function in when we open the db.
db/db_sst_test.cc
Outdated
Comment on lines
431
to
434
| ASSERT_EQ( | ||
| 4, options.statistics.get()->getAndResetTickerCount(FILES_MARKED_TRASH)); | ||
| ASSERT_EQ(0, options.statistics.get()->getAndResetTickerCount( | ||
| FILES_DELETED_IMMEDIATELY)); |
There was a problem hiding this comment.
Nit: options.statistics-> suffices.
Summary: Add logs and stats for files marked as trash and files deleted immediately in DeleteScheduler Test Plan: make check -j64 Reviewers: Subscribers: Tasks: T67391953 Tags:
akankshamahajan15
force-pushed
the
tech-debt_task
branch
from
54e7c61
to
9bf4fe5
Compare
|
@akankshamahajan15 has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
riversand963
approved these changes
Jun 4, 2020
|
@akankshamahajan15 merged this pull request in 2677bd5. |
|
This seems to cause this TSAN failure: @akankshamahajan15 can you fix it? |
|
Yes. I will fix this. Thanks. |
levichen94
pushed a commit
to bytedance/terarkdb
that referenced
this pull request
Jun 18, 2021
Summary: Add logs and stats for files marked as trash and files deleted immediately in DeleteScheduler Pull Request resolved: facebook/rocksdb#6927 Test Plan: make check -j64 Reviewed By: riversand963 Differential Revision: D21869068 Pulled By: akankshamahajan15 fbshipit-source-id: e9f673c4fa8049ce648b23c75d742f2f9c6c57a1 Signed-off-by: Changlong Chen <levisonchen@live.cn>
mm304321141
pushed a commit
to bytedance/terarkdb
that referenced
this pull request
Jun 23, 2021
Summary: Add logs and stats for files marked as trash and files deleted immediately in DeleteScheduler Pull Request resolved: facebook/rocksdb#6927 Test Plan: make check -j64 Reviewed By: riversand963 Differential Revision: D21869068 Pulled By: akankshamahajan15 fbshipit-source-id: e9f673c4fa8049ce648b23c75d742f2f9c6c57a1 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.
Summary: Add logs and stats for files marked as trash and files deleted immediately in DeleteScheduler
Test Plan: make check -j64
Reviewers:
Subscribers:
Tasks: T67391953
Tags: