Add condition on NotifyOnFlushComplete that FlushJob was not mempurge. Add event listeners to mempurge tests. #8672
Conversation
|
I use |
|
@bjlemaire has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@bjlemaire has updated the pull request. You must reimport the pull request before landing. |
|
@bjlemaire has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@bjlemaire has updated the pull request. You must reimport the pull request before landing. |
|
@bjlemaire has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| @@ -411,6 +414,7 @@ Status DBImpl::AtomicFlushMemTablesToOutputFiles( | |||
| } | |||
|
|
|||
| std::vector<FileMetaData> file_meta(num_cfs); | |||
| std::vector<uint8_t> switched_to_mempurge(num_cfs, false); | |||
There was a problem hiding this comment.
Shouldn't it be 0 instead of false? I don't think it will make any difference apart from readability.
There was a problem hiding this comment.
My bad, thanks for catching that, I was trying to use bools the whole time so that's just a remnant of what I used before switching to uint8_t.
db/flush_job.cc
Outdated
| @@ -228,6 +228,9 @@ Status FlushJob::Run(LogsWithPrepTracker* prep_tracker, | |||
| prev_cpu_read_nanos = IOSTATS(cpu_read_nanos); | |||
| } | |||
| Status mempurge_s = Status::NotFound("No MemPurge."); | |||
| if (switched_to_mempurge) { | |||
| *switched_to_mempurge = 0; | |||
There was a problem hiding this comment.
Do we need to set it or it will be 0 by default?
There was a problem hiding this comment.
It is 0 by default, I added this out of paranoia. It's probably cleaner to remove this you are right, thanks for your input.
| } else { | ||
| // The mempurge process was successful, but no switch_to_mempurge | ||
| // pointer provided so no way to propagate the state of flush job. | ||
| ROCKS_LOG_WARN(db_options_.info_log, |
There was a problem hiding this comment.
Just curious, what are the reasons that switched_to_mempurge is not provided
There was a problem hiding this comment.
This is a good question. The first two arguments of FlushJob::Run, prep_tracker and file_meta, are already optional arguments, so adding a third argument required a default value as well.
In practice, the only moment sin the current code base when Run() (aka no arguments provided, including no switched_to_mempurge) is called is in flush_job_test.cc, so I am (arguably) hesitant to make it a required argument for this reason.
There was a problem hiding this comment.
Makes sense. Thanks for clarifying it.
|
@bjlemaire has updated the pull request. You must reimport the pull request before landing. |
|
@bjlemaire has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@bjlemaire has updated the pull request. You must reimport the pull request before landing. |
|
@bjlemaire has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@bjlemaire has updated the pull request. You must reimport the pull request before landing. |
|
@bjlemaire has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
…sful. Add event listeners in unit tests.
bjlemaire
force-pushed
the
piggyback_flushjobinfo
branch
from
afb0037 to
90f1444
Compare
|
@bjlemaire has updated the pull request. You must reimport the pull request before landing. |
|
@bjlemaire has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@bjlemaire merged this pull request in c625b8d. |
…. Add event listeners to mempurge tests. (#8672) Summary: Previously, when a `FlushJob` was redirected to a MemPurge, the function `DBImpl::NotifyOnFlushComplete` was called, which created a series of issues because the JobInfo was not correctly collected from the memtables. This diff aims at correcting these two issues (`FlushJobInfo` collection in `FlushJob::MemPurge` , no call to `DBImpl::NotifyOnFlushComplete` after successful mempurge). Event listeners were added to the unit tests to handle these situations. Surprisingly none of the crashtests caught this issue, I will try to add event listeners to crash tests in the future. Pull Request resolved: facebook/rocksdb#8672 Reviewed By: akankshamahajan15 Differential Revision: D30383109 Pulled By: bjlemaire fbshipit-source-id: 35a8d4295886923ee4049a6447f00022cb221c73
Previously, when a
FlushJobwas redirected to a MemPurge, the functionDBImpl::NotifyOnFlushCompletewas called, which created a series of issues because the JobInfo was not correctly collected from the memtables.This diff aims at correcting these two issues (
FlushJobInfocollection inFlushJob::MemPurge, no call toDBImpl::NotifyOnFlushCompleteafter successful mempurge).Event listeners were added to the unit tests to handle these situations.
Surprisingly none of the crashtests caught this issue, I will try to add event listeners to crash tests in the future.