-
Notifications
You must be signed in to change notification settings - Fork 6.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Log GetCurrentTime failures during Flush and Compaction #3231
Conversation
Fixing some tech debt from TTL-based FIFO compaction: T19827304 (internal). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sagar0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
db/compaction_job.cc
Outdated
auto status = db_options_.env->GetCurrentTime(&_current_time); | ||
// Safe to proceed even if GetCurrentTime fails. So, log and proceed. | ||
if (!status.ok()) { | ||
ROCKS_LOG_DEBUG(db_options_.info_log, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth to be on warning level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mention in the log the time is for getting creation_time in compactions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all good points. Updated the patch with these suggestions.
db_options_.env->GetCurrentTime(&_current_time); // ignore error | ||
auto status = db_options_.env->GetCurrentTime(&_current_time); | ||
// Safe to proceed even if GetCurrentTime fails. So, log and proceed. | ||
if (!status.ok()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@sagar0 has updated the pull request. View: changes, changes since last import |
thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sagar0 is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
GetCurrentTime()
is used to populatecreation_time
table property during flushes and compactions. It is safe to ignoreGetCurrentTime()
failures here but they should be logged.(Note that
creation_time
property was introduced as part of TTL-based FIFO compaction in #2480.)Tes Plan:
make check