Attila Jeges 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 2:

(15 comments)

Thanks! Few more comments:

http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@15
PS2, Line 15: In addition there were two functions, NewFile() and
            : FileAllocateSpace() that always returned Status::OK(). I made them
            : void and removed the status checks from the call sites.
> I observed this behaviour during the investigation and seemed a good idea t
Thanks, I was just curious if it was a general cleanup or something related to 
the JIRA proper.


http://gerrit.cloudera.org:8080/#/c/9420/2/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/2/be/src/runtime/io/disk-io-mgr-test.cc@384
PS2, Line 384: ExecuteWriteFailureTest
> The reason I implemented this way is that In case I followed how InvalidWri
Thanks, I think I got it now :)


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:  unique_ptr<B
nit: 'phase_to_fail'


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 string& ?


http://gerrit.cloudera.org:8080/#/c/9420/4/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@60
PS4, Line 60:
Have you considered adding a public member function to 
DiskIoMgrWithFaultInjection class to set/clear 'fault_injection_to_write_' ?

Using the class like this is a bit awkward.

Also probably DiskIoMgrTest would not have to be a friend class then.


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: st in
nit: returned


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: Function names can be "open", "fdopen", "fseek", "fwrite"
             :   // and "fclose".
> I was thinking of this as well. The reason I chose strings is that I'd like
If you add a public member function to this class to set/clear 
'faultInjectionToWrite_' (as I suggested), then passing in an enum parameter to 
that function would be more straightforward. You still need to map the enum 
literals to strings.

On the other hand it might be an overkill. Your call ;)


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

http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.cc@812
PS2, Line 812: DebugFaultInjection
> This is the point where I set errno to some desired value. Once it is set,
Thanks, my bad. I meant to comment this on GetErrorStatusFromErrno()


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.


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: ror text r
Maybe this should be called 'errno_' or something similar to avoid confusing it 
with an Impala error code. Also, the comment above should refer to the formal 
parameter name. Please update it.


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


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


http://gerrit.cloudera.org:8080/#/c/9420/2/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/2/be/src/runtime/io/errno-to-error-status-converter.cc@25
PS2, Line 25: unordered_map<int, string> 
ErrnoToErrorStatusConverter::errnoToErrorTextMap_(
            :     {{EACCES, "Access denied for the process' user"},
            :      {EINTR,   "Internal error occured."},
            :      {EINVAL,  "Invalid inputs."},
            :      {EMFILE,  "Process level opened file descriptor count is 
reached."},
            :      {ENAMETOOLONG,
            :          "Either the path length or a path component exceeds the 
maximum length."},
            :      {ENFILE,  "OS level opened file descriptor count is 
reached."},
            :      {ENOENT,  "The given path doesn't exist."},
            :      {ENOSPC,  "No space left on device."},
            :      {ENOTDIR, "It is not a directory."},
            :      {EOVERFLOW, "File size can't be represented."},
            :      {EROFS,   "The file system is read only."},
            :      {EAGAIN,  "Resource temporarily unavailable."},
            :      {EBADF,   "The given file descriptor is invalid."},
            :      {ENOMEM,  "Not enough memory."},
            :      {EFBIG,   "Maximum file size reached."},
            :      {EIO,     "Disk level I/O error occured."},
            :      {ENXIO,   "Device doesn't exist."}});
> Gathered the possible errno's from these pages:
Thanks


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: t_body)) {
Maybe this should be called 'errno_' or something similar to avoid confusing it 
with an Impala error code.


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



--
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: 2
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: Tue, 06 Mar 2018 18:54:09 +0000
Gerrit-HasComments: Yes

Reply via email to