Add listener API that notifies on IOError #9177
Conversation
akankshamahajan15
force-pushed
the
log_io_error
branch
2 times, most recently
from
a47824d
to
3da6fdd
Compare
|
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
|
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| NotifyOnFileTruncateFinish(start_ts, finish_ts, s); | ||
| if (!interim.ok()) { |
There was a problem hiding this comment.
I am not sure this is related to this specific change but this code looks generally broken to me. The NotifyOnFiileTruncateFinish passes in the status "s", but that status is not the status of Flush() at line 205. The same is true later for NotifyOnFileSyncFinish and NotifyOnFileCloseFinish.
There was a problem hiding this comment.
Yes. It doesn't seems consistent. Here we are passing the status of Flush() but at other places we are passing the status returned by FileSystem Operation.
file/writable_file_writer.cc
Outdated
| if (!interim.ok()) { | ||
| NotifyOnIOError(interim, "Fsync", file_name()); | ||
| } |
There was a problem hiding this comment.
Would the code end up being simpler if in all of the NotifyOnFileXXX methods, if the input status was an error, you called NotifyOnIOError? There seems like places now that are missing the NotifyOnIOError (cases in Flush() and Append()) ?
There was a problem hiding this comment.
I have added the error notification when writable_file_-> calls the underlying File system APIs. I will update the PR to specify this. So in Flush(), it will report error when it calls writable_file_->Flush.
Summary: Add a new API in listener.h that notifies about IOErrors on Read/Write/Append/Flush etc. The API reports about IOStatus, filename, Operation name, offset and length. Test Plan: In progress Reviewers: Subscribers: Tasks: Tags:
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
@anand1976 you mean whenever we call |
akankshamahajan15
force-pushed
the
log_io_error
branch
from
d13c062
to
bd62e8b
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
akankshamahajan15
force-pushed
the
log_io_error
branch
from
bd62e8b
to
8b318b2
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
akankshamahajan15
force-pushed
the
log_io_error
branch
from
8b318b2
to
83599ec
Compare
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
|
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
|
@akankshamahajan15 has updated the pull request. You must reimport the pull request before landing. |
|
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
1 similar comment
|
@akankshamahajan15 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@akankshamahajan15 merged this pull request in 4a7c1dc. |
Summary: Add a new API in listener.h that notifies about IOErrors on
Read/Write/Append/Flush etc. The API reports about IOStatus, filename, Operation
name, offset and length.
Test Plan: In progress
Reviewers:
Subscribers:
Tasks:
Tags: