Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9420 )

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


Patch Set 5:

(11 comments)

http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr-test.cc
File be/src/runtime/io/disk-io-mgr-test.cc:

http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr-test.cc@188
PS4, Line 188: 'phase_to_fai
> nit: 'phase_to_fail'
Done


http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr-with-fault-injection.h
File be/src/runtime/io/disk-io-mgr-with-fault-injection.h:

http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@53
PS4, Line 53: const
> const string& ?
Done


http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@60
PS4, Line 60: // DiskIoMgr::Write(). The user who sets this member is also 
respon
> 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.
Done


http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr.h
File be/src/runtime/io/disk-io-mgr.h:

http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr.h@402
PS4, Line 402: retur
> nit: returned
Done


http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.h
File be/src/runtime/io/disk-io-mgr.h:

http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.h@440
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.


http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr.cc@1107
PS4, Line 1107: !
> nit: parentheses are unnecessary.
Done


http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/errno-to-error-status-converter.h
File be/src/runtime/io/errno-to-error-status-converter.h:

http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/errno-to-error-status-converter.h@43
PS4, Line 43: err_no, co
> Maybe this should be called 'errno_' or something similar to avoid confusin
Done


http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/errno-to-error-status-converter.h@52
PS4, Line 52: err_no, Pa
> Same here
Done


http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/errno-to-error-status-converter.h@56
PS4, Line 56: err_no);
> Same here
Done


http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/errno-to-error-status-converter.cc
File be/src/runtime/io/errno-to-error-status-converter.cc:

http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/errno-to-error-status-converter.cc@54
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?
Done


http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/errno-to-error-status-converter.cc@76
PS4, Line 76: err_no) {
> Same as above
Done



--
To view, visit http://gerrit.cloudera.org:8080/9420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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 <gaborkas...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Mar 2018 12:47:53 +0000
Gerrit-HasComments: Yes

Reply via email to