Gabor Kaszab has posted comments on this change. ( )

Change subject: IMPALA-3866 Improve error reporting for scratch write errors

Patch Set 5:

File be/src/runtime/io/
PS4, Line 188: 'phase_to_fai
> nit: 'phase_to_fail'
File be/src/runtime/io/disk-io-mgr-with-fault-injection.h:
PS4, Line 53: const
> const string& ?
PS4, Line 60: // DiskIoMgr::Write(). The user who sets this member is also 
> Have you considered adding a public member function to DiskIoMgrWithFaultIn
My plan was to implement this fault injection logic writing as less code as 
possible even if it requires some 'awkward' decisions.
However, adding public members to set/reset the injection member is not much 
extra work so I gladly implement these.
File be/src/runtime/io/disk-io-mgr.h:
PS4, Line 402: retur
> nit: returned
File be/src/runtime/io/disk-io-mgr.h:
PS2, Line 440:
             :   /// The minimum
> If you add a public member function to this class to set/clear 'faultInject
I gave it a try. Started implementing the enum approach, but then realised that 
both my new classes would use it that would make it a bit more complicated to 
implement (move enum to separate class, or leavi it in one class but then the 
orhet class should use from there that wouldn't be elegant either). It's 
definitely feasible, however, I feel that it's an overkill considering that the 
functionality that requires this is only for testing purposes.
File be/src/runtime/io/
PS4, Line 1107: !
> nit: parentheses are unnecessary.
File be/src/runtime/io/errno-to-error-status-converter.h:
PS4, Line 43: err_no, co
> Maybe this should be called 'errno_' or something similar to avoid confusin
PS4, Line 52: err_no, Pa
> Same here
PS4, Line 56: err_no);
> Same here
File be/src/runtime/io/
PS4, Line 54: err_no, Pa
> Maybe this should be called 'errno_' or something similar to avoid confusin
errno_ would indicate that this was some member of the object. I'd rather 
choose err_no instead. What do you think?
PS4, Line 76: err_no) {
> Same as above

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7
Gerrit-Change-Number: 9420
Gerrit-PatchSet: 5
Gerrit-Owner: Gabor Kaszab <>
Gerrit-Reviewer: Attila Jeges <>
Gerrit-Reviewer: Gabor Kaszab <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Comment-Date: Wed, 07 Mar 2018 12:47:53 +0000
Gerrit-HasComments: Yes

Reply via email to