[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9420 ) Change subject: IMPALA-3866: Improve error reporting for scratch write errors .. IMPALA-3866: Improve error reporting for scratch write errors The error messages coming from DiskIoMgr::Write() are enhanced by this change. A mapping is introduced between the errno set by open(), fdopen(), fseek(), fwrite() or fclose() low level functions and an error message for displaying purposes. If any of these functions fail then the returned error message is taken from this mapping. 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. For testing purposes a fault injection mechanism is introduced to simulate the cases when the above mentioned functions fail. Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 Reviewed-on: http://gerrit.cloudera.org:8080/9420 Reviewed-by: Tim Armstrong Tested-by: Impala Public Jenkins --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr-test.cc M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/disk-io-mgr.h A be/src/runtime/io/error-converter.cc A be/src/runtime/io/error-converter.h A be/src/runtime/io/local-file-system-with-fault-injection.cc A be/src/runtime/io/local-file-system-with-fault-injection.h A be/src/runtime/io/local-file-system.cc A be/src/runtime/io/local-file-system.h M be/src/runtime/io/request-ranges.h M be/src/runtime/tmp-file-mgr-internal.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h M be/src/util/error-util.cc M be/src/util/error-util.h 17 files changed, 624 insertions(+), 88 deletions(-) Approvals: Tim Armstrong: Looks good to me, approved Impala Public Jenkins: Verified -- 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: merged Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 Gerrit-Change-Number: 9420 Gerrit-PatchSet: 17 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Impala Public Jenkins 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 16: Verified+1 -- 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: 16 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 21 Mar 2018 20:31:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Impala Public Jenkins 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 16: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2142/ -- 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: 16 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 21 Mar 2018 16:30:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Tim Armstrong 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 16: Code-Review+2 -- 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: 16 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 21 Mar 2018 16:30:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
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 15: Hey Tim, Dan, I think I addressed all your comments. Could you please take a look and submit if you find everything fine? -- 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: 15 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 21 Mar 2018 16:05:25 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Dan Hecht 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 15: Code-Review+2 -- 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: 15 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 16 Mar 2018 22:50:25 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Tim Armstrong 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 15: Code-Review+1 (1 comment) Will let Dan +2. Looks like you addressed his comments. http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h File be/src/runtime/io/error-converter.h: http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h@19 PS12, Line 19: IMPALA_RUNTIME_IO_ERROR_CONVERTER_H > Thx, I don't know how I could have missed this. I'm pretty sure there are other cases in the codebase where its inconsistent or screwy. Not a big deal, just trying to minimise entropy in the code. -- 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: 15 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 16 Mar 2018 18:40:48 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
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 14:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc
File be/src/runtime/io/error-converter.cc:
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc@44
PS12, Line 44: {ENXIO, "Device doesn't exist."}});
> Okay. I thought GetStrErrMsg() gives similar text, but maybe not.
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: 14
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Fri, 16 Mar 2018 07:46:33 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Hello Attila Jeges, Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9420 to look at the new patch set (#15). Change subject: IMPALA-3866: Improve error reporting for scratch write errors .. IMPALA-3866: Improve error reporting for scratch write errors The error messages coming from DiskIoMgr::Write() are enhanced by this change. A mapping is introduced between the errno set by open(), fdopen(), fseek(), fwrite() or fclose() low level functions and an error message for displaying purposes. If any of these functions fail then the returned error message is taken from this mapping. 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. For testing purposes a fault injection mechanism is introduced to simulate the cases when the above mentioned functions fail. Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr-test.cc M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/disk-io-mgr.h A be/src/runtime/io/error-converter.cc A be/src/runtime/io/error-converter.h A be/src/runtime/io/local-file-system-with-fault-injection.cc A be/src/runtime/io/local-file-system-with-fault-injection.h A be/src/runtime/io/local-file-system.cc A be/src/runtime/io/local-file-system.h M be/src/runtime/io/request-ranges.h M be/src/runtime/tmp-file-mgr-internal.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h M be/src/util/error-util.cc M be/src/util/error-util.h 17 files changed, 624 insertions(+), 88 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/15 -- 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: newpatchset Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 Gerrit-Change-Number: 9420 Gerrit-PatchSet: 15 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Hello Attila Jeges, Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9420 to look at the new patch set (#14). Change subject: IMPALA-3866: Improve error reporting for scratch write errors .. IMPALA-3866: Improve error reporting for scratch write errors The error messages coming from DiskIoMgr::Write() are enhanced by this change. A mapping is introduced between the errno set by open(), fdopen(), fseek(), fwrite() or fclose() low level functions and an error message for displaying purposes. If any of these functions fail then the returned error message is taken from this mapping. 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. For testing purposes a fault injection mechanism is introduced to simulate the cases when the above mentioned functions fail. Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr-test.cc M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/disk-io-mgr.h A be/src/runtime/io/error-converter.cc A be/src/runtime/io/error-converter.h A be/src/runtime/io/local-file-system-with-fault-injection.cc A be/src/runtime/io/local-file-system-with-fault-injection.h A be/src/runtime/io/local-file-system.cc A be/src/runtime/io/local-file-system.h M be/src/runtime/io/request-ranges.h M be/src/runtime/tmp-file-mgr-internal.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h M be/src/util/error-util.cc M be/src/util/error-util.h 17 files changed, 624 insertions(+), 88 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/14 -- 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: newpatchset Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 Gerrit-Change-Number: 9420 Gerrit-PatchSet: 14 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Dan Hecht 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 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc
File be/src/runtime/io/error-converter.cc:
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc@44
PS12, Line 44: {ENXIO, "Device doesn't exist."}});
> The request was to enrich the basic error messages from GetStrErrMsg() so t
Okay. I thought GetStrErrMsg() gives similar text, but maybe not.
Is the motivation for this documented somewhere? If no, how about putting a
quick comment explaining why we have this rather than just using GetStrErrMsg()
since I don't think that's obvious.
--
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: 13
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Thu, 15 Mar 2018 16:09:26 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
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 13:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h
File be/src/runtime/io/error-converter.h:
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h@19
PS12, Line 19: IMPALA_RUNTIME_IO_ERROR_CONVERTER_H
> needs update
Thx, I don't know how I could have missed this.
Done
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h@41
PS12, Line 41: /// text is provided by the errno_to_error_text_map_ member.
The key-value pairs in
> it'd be good to comment on 'param's in the public interface (like you do be
Good point. I'll move the comment from the private method here as I don't see
the point of having it for both functions.
Done
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc
File be/src/runtime/io/error-converter.cc:
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc@44
PS12, Line 44: {ENXIO, "Device doesn't exist."}});
> out of curiosity, why do we define these rather than using GetStrErrMsg() a
The request was to enrich the basic error messages from GetStrErrMsg() so that
support can more easily identify the underlying issue for a scratch write error.
These custom error messages are the result of the consultation with Jeszy and
Tim.
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc@61
PS12, Line 61: GetStrErrMsg
> GetStrErrMsg() has a comment about saving errno early or else it might get
Good point. Made a second version of GetStrErrMsg() that receives the error
code to make sure it's not overriden.
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/local-file-system.h
File be/src/runtime/io/local-file-system.h:
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/local-file-system.h@33
PS12, Line 33: // A wrapper around open() and fdopen(). For the possible
values of oflag and mode
> I think this needs a comment explaining option1 and option2. Maybe it would
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: 13
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Thu, 15 Mar 2018 13:19:06 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Hello Attila Jeges, Tim Armstrong, Dan Hecht, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9420 to look at the new patch set (#13). Change subject: IMPALA-3866: Improve error reporting for scratch write errors .. IMPALA-3866: Improve error reporting for scratch write errors The error messages coming from DiskIoMgr::Write() are enhanced by this change. A mapping is introduced between the errno set by open(), fdopen(), fseek(), fwrite() or fclose() low level functions and an error message for displaying purposes. If any of these functions fail then the returned error message is taken from this mapping. 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. For testing purposes a fault injection mechanism is introduced to simulate the cases when the above mentioned functions fail. Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr-test.cc M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/disk-io-mgr.h A be/src/runtime/io/error-converter.cc A be/src/runtime/io/error-converter.h A be/src/runtime/io/local-file-system-with-fault-injection.cc A be/src/runtime/io/local-file-system-with-fault-injection.h A be/src/runtime/io/local-file-system.cc A be/src/runtime/io/local-file-system.h M be/src/runtime/io/request-ranges.h M be/src/runtime/tmp-file-mgr-internal.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h M be/src/util/error-util.cc M be/src/util/error-util.h 17 files changed, 621 insertions(+), 88 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/13 -- 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: newpatchset Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 Gerrit-Change-Number: 9420 Gerrit-PatchSet: 13 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Dan Hecht 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 12:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h
File be/src/runtime/io/error-converter.h:
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h@41
PS12, Line 41: /// text is provided by the errno_to_error_text_map_ member.
it'd be good to comment on 'param's in the public interface (like you do
below). I wasn't sure what that was until I read the private methods.
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc
File be/src/runtime/io/error-converter.cc:
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc@44
PS12, Line 44: {ENXIO, "Device doesn't exist."}});
out of curiosity, why do we define these rather than using GetStrErrMsg() all
the time?
http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.cc@61
PS12, Line 61: GetStrErrMsg
GetStrErrMsg() has a comment about saving errno early or else it might get
overwritten. How can we be sure errno still contains the original error by now,
given this function may be called deeper in the call chain since err_no is
passed through params?
--
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: 12
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Wed, 14 Mar 2018 23:54:08 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Tim Armstrong 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 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/9420/10/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/9420/10/be/src/runtime/io/disk-io-mgr.cc@1098 PS10, Line 1098: if (!ret_status.ok()) goto end; > Nice catch! Yeah that does seem like overkill to me too. -- 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: 10 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 14 Mar 2018 21:16:29 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Tim Armstrong 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 12: (2 comments) Thanks, I think this worked out quite nicely. http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h File be/src/runtime/io/error-converter.h: http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/error-converter.h@19 PS12, Line 19: IMPALA_RUNTIME_IO_ERRNO_TO_ERROR_STATUS_CONVERTER_H needs update http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/local-file-system.h File be/src/runtime/io/local-file-system.h: http://gerrit.cloudera.org:8080/#/c/9420/12/be/src/runtime/io/local-file-system.h@33 PS12, Line 33: Status OpenForWrite(const char* file_name, int option1, int option2, FILE** file); I think this needs a comment explaining option1 and option2. Maybe it would be clearer if the were oflags and mode to match how it's described here: http://pubs.opengroup.org/onlinepubs/009696699/functions/open.html -- 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: 12 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 14 Mar 2018 21:15:56 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
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 12: (1 comment) http://gerrit.cloudera.org:8080/#/c/9420/10/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/9420/10/be/src/runtime/io/disk-io-mgr.cc@1098 PS10, Line 1098: if (ret_status.ok() && !close_status.ok()) ret_status = close_status; > Hmm. Actually I think there was a pre-existing bug here. We should close fd Nice catch! Done Note, that I considered close() also to be fault injected, but then for testing I would have needed a way to make both fdopen() and close() fail but the current implementation of the fault injection can handle only one of them. Could have implemented a way to handle multiple failures as well but found that an overkill comparing to the logic this is meant to cover. What do you think? -- 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: 12 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 14 Mar 2018 11:29:10 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Hello Attila Jeges, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9420 to look at the new patch set (#12). Change subject: IMPALA-3866: Improve error reporting for scratch write errors .. IMPALA-3866: Improve error reporting for scratch write errors The error messages coming from DiskIoMgr::Write() are enhanced by this change. A mapping is introduced between the errno set by open(), fdopen(), fseek(), fwrite() or fclose() low level functions and an error message for displaying purposes. If any of these functions fail then the returned error message is taken from this mapping. 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. For testing purposes a fault injection mechanism is introduced to simulate the cases when the above mentioned functions fail. Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr-test.cc M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/disk-io-mgr.h A be/src/runtime/io/error-converter.cc A be/src/runtime/io/error-converter.h A be/src/runtime/io/local-file-system-with-fault-injection.cc A be/src/runtime/io/local-file-system-with-fault-injection.h A be/src/runtime/io/local-file-system.cc A be/src/runtime/io/local-file-system.h M be/src/runtime/io/request-ranges.h M be/src/runtime/tmp-file-mgr-internal.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 15 files changed, 602 insertions(+), 86 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/12 -- 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: newpatchset Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 Gerrit-Change-Number: 9420 Gerrit-PatchSet: 12 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Tim Armstrong 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 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/9420/10/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/9420/10/be/src/runtime/io/disk-io-mgr.cc@1098 PS10, Line 1098: if (!ret_status.ok()) goto end; Hmm. Actually I think there was a pre-existing bug here. We should close fd if fdopen() fails. It's probably rare but according to fdopen() docs it is possible. How about we move the open()/fdopen() sequence into a single method in LocalFileSystem, e.g. OpenForWrite(). Then we can hide the details of the error handling and also the raw file descriptor inside that function. -- 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: 10 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 13 Mar 2018 20:16:35 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Hello Attila Jeges, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9420 to look at the new patch set (#11). Change subject: IMPALA-3866: Improve error reporting for scratch write errors .. IMPALA-3866: Improve error reporting for scratch write errors The error messages coming from DiskIoMgr::Write() are enhanced by this change. A mapping is introduced between the errno set by open(), fdopen(), fseek(), fwrite() or fclose() low level functions and an error message for displaying purposes. If any of these functions fail then the returned error message is taken from this mapping. 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. For testing purposes a fault injection mechanism is introduced to simulate the cases when the above mentioned functions fail. Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr-test.cc M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/disk-io-mgr.h A be/src/runtime/io/error-converter.cc A be/src/runtime/io/error-converter.h A be/src/runtime/io/local-file-system-with-fault-injection.cc A be/src/runtime/io/local-file-system-with-fault-injection.h A be/src/runtime/io/local-file-system.cc A be/src/runtime/io/local-file-system.h M be/src/runtime/io/request-ranges.h M be/src/runtime/tmp-file-mgr-internal.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 15 files changed, 607 insertions(+), 85 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/11 -- 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: newpatchset Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 Gerrit-Change-Number: 9420 Gerrit-PatchSet: 11 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
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 9: (6 comments) http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.h File be/src/runtime/io/disk-io-mgr.h: http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.h@373 PS8, Line 373: // Function to change the underlying LocalFileSystem object used for disk I/O. > Let's make it clear that it's a test-only function. Sure, Done http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.h@425 PS8, Line 425: > We try to avoid shared_ptr as much as possible because shared ownership is Sure, it works with unique_ptr as well. One thing is that I had to do a move() when calling the setter function and also one move() inside when actually setting the member. I tried passing the ptr as ref but apparently it doesn't handle polymorphism well (accepts unique_ptr but given unique_ptr doesn't compile). http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.cc@1101 PS8, Line 1101: > I missed on my first pass that we aren't fclose()ing the file when we hit a As I see in the old code fclose() was invoked when the file_handle wasn't nullptr. So In case open() or fdopen() failed only HandleWriteFinished() was called without fclose(). So this results that fclose() is invoked only if both open() and fdopen() succeeds and no need to add fclose() to their error handling. For WriteRangeHelper() the fclose() have to be run, so that is a bug in my current code, thanks! Anyway, I haven't written gotos in years, let me do a small refactor here to fill this gap :) http://gerrit.cloudera.org:8080/#/c/9420/7/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/7/be/src/runtime/io/errno-to-error-status-converter.cc@23 PS7, Line 23: > Yeah that's a bit of an unfortunate case. We started off with boost::unorde Thanks for the nice explanation! http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/local-file-system-with-fault-injection.cc File be/src/runtime/io/local-file-system-with-fault-injection.cc: http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/local-file-system-with-fault-injection.cc@31 PS8, Line 31: return LocalFileSystem::OpenAux(file, option1, option2); > Long lines. It might be useful to run clang-format on the patches to detect Thanks for the advice! http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/local-file-system.cc File be/src/runtime/io/local-file-system.cc: http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/local-file-system.cc@57 PS8, Line 57: Status LocalFileSystem::Fseek(FILE* file_handle, off_t offset, int whence, > I'm not sure what this TODO means Ohh, my bad. This is some leftover I overlooked. Removed. -- 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: 9 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 13 Mar 2018 10:32:20 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Hello Attila Jeges, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9420 to look at the new patch set (#9). Change subject: IMPALA-3866: Improve error reporting for scratch write errors .. IMPALA-3866: Improve error reporting for scratch write errors The error messages coming from DiskIoMgr::Write() are enhanced by this change. A mapping is introduced between the errno set by open(), fdopen(), fseek(), fwrite() or fclose() low level functions and an error message for displaying purposes. If any of these functions fail then the returned error message is taken from this mapping. 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. For testing purposes a fault injection mechanism is introduced to simulate the cases when the above mentioned functions fail. Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr-test.cc M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/disk-io-mgr.h A be/src/runtime/io/error-converter.cc A be/src/runtime/io/error-converter.h A be/src/runtime/io/local-file-system-with-fault-injection.cc A be/src/runtime/io/local-file-system-with-fault-injection.h A be/src/runtime/io/local-file-system.cc A be/src/runtime/io/local-file-system.h M be/src/runtime/io/request-ranges.h M be/src/runtime/tmp-file-mgr-internal.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 15 files changed, 608 insertions(+), 86 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/9 -- 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: newpatchset Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 Gerrit-Change-Number: 9420 Gerrit-PatchSet: 9 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Tim Armstrong 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 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.h File be/src/runtime/io/disk-io-mgr.h: http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.h@373 PS8, Line 373: /// queued buffers (plus the buffer that is returned to the client) gives good Let's make it clear that it's a test-only function. http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.h@425 PS8, Line 425: /// provide a MemTracker. We try to avoid shared_ptr as much as possible because shared ownership is generally harder to reason about. I think in this case unique_ptr is sufficient if you move() it into set_local_file_system http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/disk-io-mgr.cc@1101 PS8, Line 1101: DCHECK(write_range != nullptr); I missed on my first pass that we aren't fclose()ing the file when we hit an error now. It would be ok to add it before each call to HandleWriteFinished(), but I would probably use the "goto end" pattern since we need to execute the exact same two lines of code on all exit paths, i.e.: ret_status = ...; if (!ret_status.ok()) goto end: ... end: ret_status = local_file_system_->Fclose(file_handle, write_range); HandleWriteFinished(writer_context, write_range, ret_status); http://gerrit.cloudera.org:8080/#/c/9420/7/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/7/be/src/runtime/io/errno-to-error-status-converter.cc@23 PS7, Line 23: using std::unordered_map; > I wasn't aware of this common/names.h, thx! Yeah that's a bit of an unfortunate case. We started off with boost::unordered_map. In most cases where a std:: equivalent appeared we've automatically switched, but there was some concern here that the std:: versions of the maps would use somewhat more memory and be slower because the standard requires using doubly-linked lists for buckets: http://bannalia.blogspot.co.uk/2013/10/implementation-of-c-unordered.html We could probably benchmark std::unordered_map and switch over if it looked acceptable but we haven't put the time in yet. I'd suggest using std::unordered_map here since the map is small and its not perf-critical. http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/local-file-system-with-fault-injection.cc File be/src/runtime/io/local-file-system-with-fault-injection.cc: http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/local-file-system-with-fault-injection.cc@31 PS8, Line 31: Long lines. It might be useful to run clang-format on the patches to detect some of these minor issues: https://wiki.cloudera.com/pages/viewpage.action?pageId=24646528 The downside is that clang-format is sometimes overly aggressive at reformatting surrounding lines, which adds unnecessary code churn. http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/local-file-system.cc File be/src/runtime/io/local-file-system.cc: http://gerrit.cloudera.org:8080/#/c/9420/8/be/src/runtime/io/local-file-system.cc@57 PS8, Line 57: I'm not sure what this TODO means -- 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: 7 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 12 Mar 2018 21:45:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
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 8: (16 comments) Thanks for taking a look, Tim! http://gerrit.cloudera.org:8080/#/c/9420/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9420/7//COMMIT_MSG@7 PS7, Line 7: IMPALA-3866: Improve error reporting for scratch write errors > nit: colon after JIRA number Done http://gerrit.cloudera.org:8080/#/c/9420/7/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/7/be/src/runtime/io/disk-io-mgr-test.cc@347 PS7, Line 347: void DiskIoMgrTest::ExecuteWriteFailureTest(DiskIoMgr* io_mgr, const string& file_name, > nit: extra blank line. Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc@355 PS7, Line 355: LOG(ERROR) << "Error creating temp file " << file_name << " of size 100"; > Can't we stack allocate this? The memory shouldn't be referenced once write Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc@362 PS7, Line 362: io_mgr->SetLocalFileSystem(fs); > Same with new_range - can't we stack allocate it? Removed this. http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc@380 PS7, Line 380: nullptr, nullptr, nullptr, data, > I don't think allocated the pointer on the heap is needed, is it? Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc@385 PS7, Line 385: } > We already overwrote the value of 'write_range' on l380, so this doesn't ac That's right, removed the write_range param. Also I noticed that it's not necessary in this case to pass a write_range to WriteValidateCallback() as it only uses it if the write succeeds. http://gerrit.cloudera.org:8080/#/c/9420/7/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/7/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@46 PS7, Line 46: > Let's document explicitly which functions they're wrapping. I docemented the name of the wrapped functions in disk-io-mgr.h. These are overriding the wrapper function from that class. http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@48 PS7, Line 48: > Let's add an "override" specifier to the end of overriding functions. Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@58 PS7, Line 58: > Our convention is to not use underscores on struct members, since they're m Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr.h File be/src/runtime/io/disk-io-mgr.h: http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr.h@401 PS7, Line 401: REMOTE_S3_DISK_OFFSET, > I feel like maybe there's already too much functionality in the DiskIoMgr c Good idea, thx! Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr.h@404 PS7, Line 404: }; > I also think if we're going to pull these out into functions, we should jus Makes sense! Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr.cc@1120 PS7, Line 1120: > We can unnest the body of this "else" now that we're doing an early return Thx! Done http://gerrit.cloudera.org:8080/#/c/9420/7/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/7/be/src/runtime/io/errno-to-error-status-converter.h@34 PS7, Line 34: > Maybe we can just name this ErrorConverter to be more concise? Sure, Done http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/errno-to-error-status-converter.h@43 PS7, Line 43: > Can we document what 'params' does? Done http://gerrit.cloudera.org:8080/#/c/9420/7/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/7/be/src/runtime/io/errno-to-error-status-converter.cc@23 PS7, Line 23: > We can just do #include "common/names.h" under the other includes and remov I wasn't aware of this common/names.h, thx! Done (Apparently this doesn't work for std::unordered_map, however would work for boost::unordered_map. Do you know if there is a preference between them?) http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/errno-to-error-status-converter.cc@68 PS7, Line 68: > nit: needs space 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-
[Impala-ASF-CR] IMPALA-3866: Improve error reporting for scratch write errors
Hello Attila Jeges, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9420 to look at the new patch set (#8). Change subject: IMPALA-3866: Improve error reporting for scratch write errors .. IMPALA-3866: Improve error reporting for scratch write errors The error messages coming from DiskIoMgr::Write() are enhanced by this change. A mapping is introduced between the errno set by open(), fdopen(), fseek(), fwrite() or fclose() low level functions and an error message for displaying purposes. If any of these functions fail then the returned error message is taken from this mapping. 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. For testing purposes a fault injection mechanism is introduced to simulate the cases when the above mentioned functions fail. Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr-test.cc M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/disk-io-mgr.h A be/src/runtime/io/error-converter.cc A be/src/runtime/io/error-converter.h A be/src/runtime/io/local-file-system-with-fault-injection.cc A be/src/runtime/io/local-file-system-with-fault-injection.h A be/src/runtime/io/local-file-system.cc A be/src/runtime/io/local-file-system.h M be/src/runtime/io/request-ranges.h M be/src/runtime/tmp-file-mgr-internal.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 15 files changed, 613 insertions(+), 85 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/8 -- 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: newpatchset Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 Gerrit-Change-Number: 9420 Gerrit-PatchSet: 8 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Tim Armstrong 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 7:
(16 comments)
http://gerrit.cloudera.org:8080/#/c/9420/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9420/7//COMMIT_MSG@7
PS7, Line 7: IMPALA-3866 Improve error reporting for scratch write errors
nit: colon after JIRA number
http://gerrit.cloudera.org:8080/#/c/9420/7/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/7/be/src/runtime/io/disk-io-mgr-test.cc@347
PS7, Line 347:
nit: extra blank line.
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc@355
PS7, Line 355: int32_t* data = pool_.Add(new int32_t);
Can't we stack allocate this? The memory shouldn't be referenced once writer is
unregistered. I.e.
int32_t data = rand();
AddWriteRange(..., &data, ...);
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc@362
PS7, Line 362: WriteRange** new_range = pool_.Add(new WriteRange*);
Same with new_range - can't we stack allocate it?
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc@380
PS7, Line 380: write_range = pool_.Add(new WriteRange*);
I don't think allocated the pointer on the heap is needed, is it?
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-test.cc@385
PS7, Line 385: *write_range = pool_.Add(new WriteRange(file_name, offset, 0,
callback));
We already overwrote the value of 'write_range' on l380, so this doesn't
actually return anything to the caller.
Maybe we don't need 'write_range' to be an argument, even?
http://gerrit.cloudera.org:8080/#/c/9420/7/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/7/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@46
PS7, Line 46: virtual int Open(const char* file, int option1, int option2);
Let's document explicitly which functions they're wrapping.
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@48
PS7, Line 48: );
Let's add an "override" specifier to the end of overriding functions.
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@58
PS7, Line 58: function_
Our convention is to not use underscores on struct members, since they're meant
to be public.
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr.h
File be/src/runtime/io/disk-io-mgr.h:
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr.h@401
PS7, Line 401: // Wrapper functions around open(), fdopen(), fseek(),
fwrite() and fclose(). The
I feel like maybe there's already too much functionality in the DiskIoMgr
class. Can we pull out these functions into a helper class that we can
subclass? Maybe something like LocalFilesystem?
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr.h@404
PS7, Line 404: virtual int Open(const char* file, int option1, int option2);
I also think if we're going to pull these out into functions, we should just
make them return Status so they're consistent with the usual Impala calling
conventions. This would also slightly reduce the amount of logic in the core
DiskIoMgr class, which would be a win.
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/disk-io-mgr.cc@1120
PS7, Line 1120: } else {
We can unnest the body of this "else" now that we're doing an early return on
the other branch.
http://gerrit.cloudera.org:8080/#/c/9420/7/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/7/be/src/runtime/io/errno-to-error-status-converter.h@34
PS7, Line 34: ErrnoToErrorStatusConverter
Maybe we can just name this ErrorConverter to be more concise?
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/errno-to-error-status-converter.h@43
PS7, Line 43: params
Can we document what 'params' does?
http://gerrit.cloudera.org:8080/#/c/9420/7/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/7/be/src/runtime/io/errno-to-error-status-converter.cc@23
PS7, Line 23: using std::unordered_map;
We can just do #include "common/names.h" under the other includes and remove
these using statements.
http://gerrit.cloudera.org:8080/#/c/9420/7/be/src/runtime/io/errno-to-error-status-converter.cc@68
PS7, Line 68: for(const auto& item : params) {
nit: needs space
--
To view, visit http://gerrit.cloude
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
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 7: Code-Review+1 Thanks -- 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: 7 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 08 Mar 2018 15:28:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Hello Attila Jeges, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9420 to look at the new patch set (#7). Change subject: IMPALA-3866 Improve error reporting for scratch write errors .. IMPALA-3866 Improve error reporting for scratch write errors The error messages coming from DiskIoMgr::Write() are enhanced by this change. A mapping is introduced between the errno set by open(), fdopen(), fseek(), fwrite() or fclose() low level functions and an error message for displaying purposes. If any of these functions fail then the returned error message is taken from this mapping. 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. For testing purposes a fault injection mechanism is introduced to simulate the cases when the above mentioned functions fail. Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr-test.cc A be/src/runtime/io/disk-io-mgr-with-fault-injection.cc A be/src/runtime/io/disk-io-mgr-with-fault-injection.h M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/disk-io-mgr.h A be/src/runtime/io/errno-to-error-status-converter.cc A be/src/runtime/io/errno-to-error-status-converter.h M be/src/runtime/tmp-file-mgr-internal.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 12 files changed, 488 insertions(+), 76 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/7 -- 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: newpatchset Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 Gerrit-Change-Number: 9420 Gerrit-PatchSet: 7 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
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 6:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc
File be/src/runtime/io/disk-io-mgr-with-fault-injection.cc:
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@38
PS6, Line 38: {
> nit: Parentheses not necessary (here and below).
Done
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@52
PS6, Line 52: int64_t
> fwrite() retuns size_t
Done
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@53
PS6, Line 53: return -1;
> fwrite() should return 0 in case of failure.
Done
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@58
PS6, Line 58: return -1;
> fclose() should return EOF in case of failure.
Done
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr.cc@1099
PS6, Line 1099: WriteRange*
> const WriteRange* ?
Done
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr.cc@1099
PS6, Line 1099: int64_t
> fwrite() returns size_t
Done
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr.cc@1106
PS6, Line 1106: file_handle
> file_handle != nullptr
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: 6
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Thu, 08 Mar 2018 15:10:43 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
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 6:
(7 comments)
Thanks!
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc
File be/src/runtime/io/disk-io-mgr-with-fault-injection.cc:
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@38
PS6, Line 38: {
nit: Parentheses not necessary (here and below).
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@52
PS6, Line 52: int64_t
fwrite() retuns size_t
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@53
PS6, Line 53: return -1;
fwrite() should return 0 in case of failure.
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@58
PS6, Line 58: return -1;
fclose() should return EOF in case of failure.
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr.cc
File be/src/runtime/io/disk-io-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr.cc@1099
PS6, Line 1099: int64_t
fwrite() returns size_t
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr.cc@1099
PS6, Line 1099: WriteRange*
const WriteRange* ?
http://gerrit.cloudera.org:8080/#/c/9420/6/be/src/runtime/io/disk-io-mgr.cc@1106
PS6, Line 1106: file_handle
file_handle != nullptr
--
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: 6
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Thu, 08 Mar 2018 13:26:49 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
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 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/9420/5/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/5/be/src/runtime/io/disk-io-mgr-test.cc@191
PS5, Line 191: const string& function_name, int err_no,
> nit: It would be nice to make formal parameter names consistent across func
Done
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-test.cc@350
PS5, Line 350: const string& function_name, int err_no,
> same as above
Done
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-test.cc@378
PS5, Line 378: file_nam
> nit: 'file_name' to be consistent with the formal parameter name in Execute
Done
http://gerrit.cloudera.org:8080/#/c/9420/5/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/5/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@56
PS5, Line 56: eFault
> nit: Maybe this should be called 'err_no_' for consistency?
Hmm. err_no_ just doesn't look that pretty for for me. May I leave this as it
is?
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc
File be/src/runtime/io/disk-io-mgr-with-fault-injection.cc:
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@38
PS5, Line 38: ction("op
> Another approach would be to abstract away the open() syscall itself, inste
Done
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@38
PS5, Line 38: ugFaultInjection("open")) { return -1; }
> Not sure about this, but maybe the order of operands should be reversed (he
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: 6
Gerrit-Owner: Gabor Kaszab
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Thu, 08 Mar 2018 07:55:18 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Hello Attila Jeges, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9420 to look at the new patch set (#6). Change subject: IMPALA-3866 Improve error reporting for scratch write errors .. IMPALA-3866 Improve error reporting for scratch write errors The error messages coming from DiskIoMgr::Write() are enhanced by this change. A mapping is introduced between the errno set by open(), fdopen(), fseek(), fwrite() or fclose() low level functions and an error message for displaying purposes. If any of these functions fail then the returned error message is taken from this mapping. 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. For testing purposes a fault injection mechanism is introduced to simulate the cases when the above mentioned functions fail. Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr-test.cc A be/src/runtime/io/disk-io-mgr-with-fault-injection.cc A be/src/runtime/io/disk-io-mgr-with-fault-injection.h M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/disk-io-mgr.h A be/src/runtime/io/errno-to-error-status-converter.cc A be/src/runtime/io/errno-to-error-status-converter.h M be/src/runtime/tmp-file-mgr-internal.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 12 files changed, 488 insertions(+), 76 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/6 -- 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: newpatchset Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 Gerrit-Change-Number: 9420 Gerrit-PatchSet: 6 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
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 4: (1 comment) 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: error_code > errno_ would indicate that this was some member of the object. I'd rather c agree -- 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: 4 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 07 Mar 2018 14:39:12 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
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 5:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/9420/5/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/5/be/src/runtime/io/disk-io-mgr-test.cc@191
PS5, Line 191: const string& phase_to_fail, int error_code
nit: It would be nice to make formal parameter names consistent across function
calls. SetWriteFaultInjection() uses 'function_name' and 'err_no'.
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-test.cc@350
PS5, Line 350: const string& phase_to_fail, int error_code
same as above
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-test.cc@378
PS5, Line 378: tmp_file
nit: 'file_name' to be consistent with the formal parameter name in
ExecuteWriteFailureTest().
http://gerrit.cloudera.org:8080/#/c/9420/5/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/5/be/src/runtime/io/disk-io-mgr-with-fault-injection.h@56
PS5, Line 56: errno_
nit: Maybe this should be called 'err_no_' for consistency?
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc
File be/src/runtime/io/disk-io-mgr-with-fault-injection.cc:
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@38
PS5, Line 38: CheckOpen
Another approach would be to abstract away the open() syscall itself, instead
of te error-checking logic.
We should have an Open() function instead of CheckOpen():
int DiskIoMgr::Open(..) {
// Just call open() syscall
return open(..);
}
int DiskIoMgrWithFaultInjection::Open(..) {
if (DebugFaultInjection("open")) {
// Return -1 to simulate failure.
// (errno has been set by DebugFaultInjection())
return -1;
}
return DiskIoMgr::Open(..);
}
The advantage of this approach would be that if fault injection is used, we
don't even call open(), we just fail with the preset errno.
What do you think?
http://gerrit.cloudera.org:8080/#/c/9420/5/be/src/runtime/io/disk-io-mgr-with-fault-injection.cc@38
PS5, Line 38: DiskIoMgr::CheckOpen(file_desc) && !DebugFaultInjection("open");
Not sure about this, but maybe the order of operands should be reversed (here
and below) to guarantee that open() will fail with the preset errno if fault
injection is used.
--
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
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Wed, 07 Mar 2018 14:38:10 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
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
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Wed, 07 Mar 2018 12:47:53 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Hello Attila Jeges, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9420 to look at the new patch set (#5). Change subject: IMPALA-3866 Improve error reporting for scratch write errors .. IMPALA-3866 Improve error reporting for scratch write errors The error messages coming from DiskIoMgr::Write() are enhanced by this change. A mapping is introduced between the errno set by open(), fdopen(), fseek(), fwrite() or fclose() low level functions and an error message for displaying purposes. If any of these functions fail then the returned error message is taken from this mapping. 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. For testing purposes a fault injection mechanism is introduced to simulate the cases when the above mentioned functions fail. Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr-test.cc A be/src/runtime/io/disk-io-mgr-with-fault-injection.cc A be/src/runtime/io/disk-io-mgr-with-fault-injection.h M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/disk-io-mgr.h A be/src/runtime/io/errno-to-error-status-converter.cc A be/src/runtime/io/errno-to-error-status-converter.h M be/src/runtime/tmp-file-mgr-internal.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 12 files changed, 458 insertions(+), 76 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/5 -- 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: newpatchset 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
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
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_ptrhttp://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
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,
: "E
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
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 4: (31 comments) Thanks for taking a look, Attila and Tim! http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@9 PS2, Line 9: enhanced > typo: enhanced Done http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@11 PS2, Line 11: function > typo: functions Done http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@12 PS2, Line 12: If a > If Done http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@13 PS2, Line 13: then > typo: then Done http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@20 PS2, Line 20: function > typo: functions Done 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@242 PS2, Line 242: > Probably you should move L242-L255 after L236 Rebase solved this :) http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@242 PS2, Line 242: _1) > making Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@245 PS2, Line 245:(*new_range)->SetData(reint > 'phase_to_fail' specifies the name Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@246 PS2, Line 246: PECT_OK( > "fdopen" Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@250 PS2, Line 250: > ExecuteWriteFailureTest() Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@376 PS2, Line 376: > The name is misleading as errors in functions other than fdopen() are also Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@384 PS2, Line 384: write_range, nu > Mayb it woyld be cleaner if you passed "/tmp/disk_io_mgr_test.txt" to Execu Made "/tmp/disk_io_mgr_test.txt" a parameter, also removed it being hard-coded from the error text. http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@418 PS2, Line 418: > tmp_file would probably work too? 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@443 PS2, Line 443: > nit: fault_injection_to_write_ Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.h@443 PS2, Line 443: > It might be more readable if we define a struct here. Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.h@445 PS2, Line 445: disk_thread_group_; > 'phase' parameter Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.h@448 PS2, Line 448: ached_ > const string& ? Done 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@765 PS2, Line 765: disk_id = disk_queue->disk_id; > Seems a good idea not to mess the prod code for fault injection. I'll give Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.cc@782 PS2, Line 782: // Get the next reader and remove the reader so that another disk > Shouldn't this be: Indeed, thx. Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.cc@812 PS2, Line 812: > const string& ? Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.cc@812 PS2, Line 812: > It would be safer to pass in 'errno' as a parameter. Remember that any syst This is the point where I set errno to some desired value. Once it is set, not much happening until GetErrorStatusFromErrno() is called. I modified that function to receive errno as param. Is this fine or should I think of any other issues? http://gerrit.cloudera.org:8080/#/c/9420/2/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/2/be/src/runtime/io/errno-to-error-status-converter.h@28 PS2, Line 28: /// This class translates 'errno' values set by disk I/O related functions to Status > Avoid "using" declarations in headers. Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.h@31 PS2, Line 31: > Maybe something like this instead : Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.h@41 PS2, Line 41: TextM > set Done http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.h@43 PS2, Line 43: e, const Params& param > corresponding to 'errno' Done http://gerrit.cloudera.org:8
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Hello Attila Jeges, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9420 to look at the new patch set (#4). Change subject: IMPALA-3866 Improve error reporting for scratch write errors .. IMPALA-3866 Improve error reporting for scratch write errors The error messages coming from DiskIoMgr::Write() are enhanced by this change. A mapping is introduced between the errno set by open(), fdopen(), fseek(), fwrite() or fclose() low level functions and an error message for displaying purposes. If any of these functions fail then the returned error message is taken from this mapping. 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. For testing purposes a fault injection mechanism is introduced to simulate the cases when the above mentioned functions fail. Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr-test.cc A be/src/runtime/io/disk-io-mgr-with-fault-injection.cc A be/src/runtime/io/disk-io-mgr-with-fault-injection.h M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/disk-io-mgr.h A be/src/runtime/io/errno-to-error-status-converter.cc A be/src/runtime/io/errno-to-error-status-converter.h M be/src/runtime/tmp-file-mgr-internal.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 12 files changed, 449 insertions(+), 76 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/4 -- 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: newpatchset Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 Gerrit-Change-Number: 9420 Gerrit-PatchSet: 4 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
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 3: Rebased with master + resolved conflicts. -- 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: 3 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 06 Mar 2018 08:10:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Hello Attila Jeges, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/9420 to look at the new patch set (#3). Change subject: IMPALA-3866 Improve error reporting for scratch write errors .. IMPALA-3866 Improve error reporting for scratch write errors The error messages coming from DiskIoMgr::Write() are anhanced by this change. A mapping is introduced between the errno set by open(), fdopen(), fseek(), fwrite() or fclose() low level function and an error message for displaying purposes. Once any of these functions fail than the returned error message is taken from this mapping. 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. For testing purposes a fault injection mechanism is introduced to simulate the cases when the above mentioned funtions fail. Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr-test.cc M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/disk-io-mgr.h A be/src/runtime/io/errno-to-error-status-converter.cc A be/src/runtime/io/errno-to-error-status-converter.h M be/src/runtime/tmp-file-mgr-internal.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 10 files changed, 322 insertions(+), 75 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/3 -- 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: newpatchset Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 Gerrit-Change-Number: 9420 Gerrit-PatchSet: 3 Gerrit-Owner: Gabor Kaszab Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Gabor Kaszab Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
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 2:
(5 comments)
Thx Attila for taking a look! Let me answer the ones I could from the top of my
head and get back with the rest tomorrow(ish).
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.
> Any reason you included these changes?
I observed this behaviour during the investigation and seemed a good idea to
make them void. Somewhat related to error message enhancement, but more of a
dead code removal. Do you think I should leave them as they are?
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
> Mayb it woyld be cleaner if you passed "/tmp/disk_io_mgr_test.txt" to Execu
The reason I implemented this way is that In case I followed how InvalidWrite
executes 2 test then I would end up having 5 writes running in parallel and
then I couldn't guarantee the execution order. ExecuteWriteFailureTest waits
until the triggered read finishes.
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".
> Why not use an enum instead?
I was thinking of this as well. The reason I chose strings is that I'd like to
include the function name to the error text, so in case I had an enum I would
also need some map to translate it to string, right?
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@765
PS2, Line 765: fd < 0 || DebugFaultInjection("open")
> Maybe it would be cleaner if we used inheritance and virtual functions to i
Seems a good idea not to mess the prod code for fault injection. I'll give this
a try, thx!
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
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."}});
> Where did you get the content of the map from?
Gathered the possible errno's from these pages:
http://pubs.opengroup.org/onlinepubs/009696699/functions/open.html
http://pubs.opengroup.org/onlinepubs/009696699/functions/fdopen.html
http://pubs.opengroup.org/onlinepubs/009696699/functions/fseek.html
http://pubs.opengroup.org/onlinepubs/009696699/functions/fwrite.html
http://pubs.opengroup.org/onlinepubs/009696699/functions/fclose.html
I also collected inputs from Tim and Jeszy about these. I sent the related doc.
--
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
Gerrit-Reviewer: Attila Jeges
Gerrit-Reviewer: Gabor Kaszab
Gerrit-Reviewer:
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Tim Armstrong 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: (5 comments) I took a quick pass to understand it at a high level. Left a few comments but I think the approach makes sense. I'll let Attila finish the review before taking another look. 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. > Any reason you included these changes? Seems like good cleanup to me. 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@443 PS2, Line 443: faultInjectionToWrite_ nit: fault_injection_to_write_ http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.h@443 PS2, Line 443: pair It might be more readable if we define a struct here. http://gerrit.cloudera.org:8080/#/c/9420/2/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/2/be/src/runtime/io/errno-to-error-status-converter.h@28 PS2, Line 28: using std::unordered_map; Avoid "using" declarations in headers. http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/tmp-file-mgr.cc File be/src/runtime/tmp-file-mgr.cc: http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/tmp-file-mgr.cc@315 PS2, Line 315: (*tmp_file)->AllocateSpace(scratch_range_bytes, file_offset); Thanks for the cleanup, this makes it simpler. -- 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 Gerrit-Reviewer: Attila Jeges Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 05 Mar 2018 17:26:28 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
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:
(32 comments)
http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@9
PS2, Line 9: anhanced
typo: enhanced
http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@11
PS2, Line 11: function
typo: functions
http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@12
PS2, Line 12: Once
If
http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@13
PS2, Line 13: than
typo: then
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.
Any reason you included these changes?
http://gerrit.cloudera.org:8080/#/c/9420/2//COMMIT_MSG@20
PS2, Line 20: funtions
typo: functions
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@242
PS2, Line 242: make
making
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@242
PS2, Line 242: //
Probably you should move L242-L255 after L236
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@245
PS2, Line 245: phase_to_fail - Gives the name
'phase_to_fail' specifies the name
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@246
PS2, Line 246: 'fdopen'
"fdopen"
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@250
PS2, Line 250: ExecuteWriteFailureTest
ExecuteWriteFailureTest()
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@376
PS2, Line 376: WriteErrorInFdopen
The name is misleading as errors in functions other than fdopen() are also
tested.
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@384
PS2, Line 384: ExecuteWriteFailureTest
Even better, you should get rid off ExecuteWriteFailureTest() completely.
Move L415-L421 to the beginning of WriteErrorInFdopen and also L427-L433 to the
end of WriteErrorInFdopen, also set num_of_writes to 5.
Maybe I'm missing something here. Any reason you chose to implement tests this
way?
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@384
PS2, Line 384: ExecuteWriteFailureTest
Mayb it woyld be cleaner if you passed "/tmp/disk_io_mgr_test.txt" to
ExecuteWriteFailureTest() as a parameter.
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr-test.cc@418
PS2, Line 418: tmp_file.c_str()
tmp_file would probably work too?
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".
Why not use an enum instead?
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.h@445
PS2, Line 445: the given phase parameter
'phase' parameter
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.h@448
PS2, Line 448: string
const string& ?
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@765
PS2, Line 765: fd < 0 || DebugFaultInjection("open")
Maybe it would be cleaner if we used inheritance and virtual functions to
implement fault injection?
class DiskIoMgr {
virtual bool chk_open(int fd) { return fd >= 0; }
};
All the fault injection related members can be moved to an inherited test class:
class DiskIoMgrWithFaultInjection : public DiskIoMgr {
virtual bool chk_open(int fd) { return fd >= 0 && !DebugFaultInjection("open");
}
bool DebugFaultInjection(..);
boost::optional > faultInjectionToWrite_;
};
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.cc@782
PS2, Line 782: (ret_status.ok() && success != 0) ||
DebugFaultInjection("fclose")
Shouldn't this be:
ret_status.ok() && (success != 0 || DebugFaultInjection("fclose"))
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.cc@812
PS2, Line 812: string
const string& ?
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/disk-io-mgr.cc@812
PS2, Line 812: DebugFaultInjection
It would be safer to pass in 'errno' as a parameter. Remember that any system
call inside this function may reset errno.
http://gerrit.cloudera.org:8080/#/c/9420/2/be/src/runtime/io/errno-to-error-status-converter.h
Fil
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Gabor Kaszab has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/9420 ) Change subject: IMPALA-3866 Improve error reporting for scratch write errors .. IMPALA-3866 Improve error reporting for scratch write errors The error messages coming from DiskIoMgr::Write() are anhanced by this change. A mapping is introduced between the errno set by open(), fdopen(), fseek(), fwrite() or fclose() low level function and an error message for displaying purposes. Once any of these functions fail than the returned error message is taken from this mapping. 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. For testing purposes a fault injection mechanism is introduced to simulate the cases when the above mentioned funtions fail. Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr-test.cc M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/disk-io-mgr.h A be/src/runtime/io/errno-to-error-status-converter.cc A be/src/runtime/io/errno-to-error-status-converter.h M be/src/runtime/tmp-file-mgr-internal.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 10 files changed, 322 insertions(+), 74 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/2 -- 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: newpatchset Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 Gerrit-Change-Number: 9420 Gerrit-PatchSet: 2 Gerrit-Owner: Gabor Kaszab
[Impala-ASF-CR] IMPALA-3866 Improve error reporting for scratch write errors
Gabor Kaszab has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9420 Change subject: IMPALA-3866 Improve error reporting for scratch write errors .. IMPALA-3866 Improve error reporting for scratch write errors The error messages coming from DiskIoMgr::Write() are anhanced by this change. A mapping is introduced between the errno set by open(), fdopen(), fseek(), fwrite() or fclose() low level function and an error message for displaying purposes. Once any of these functions fail than the returned error message is taken from this mapping. 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. For testing purposes a fault injection mechanism is introduced to simulate the cases when the above mentioned funtions fail. Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 --- M be/src/runtime/io/CMakeLists.txt M be/src/runtime/io/disk-io-mgr-test.cc M be/src/runtime/io/disk-io-mgr.cc M be/src/runtime/io/disk-io-mgr.h A be/src/runtime/io/errno-to-error-status-converter.cc A be/src/runtime/io/errno-to-error-status-converter.h M be/src/runtime/tmp-file-mgr-internal.h M be/src/runtime/tmp-file-mgr-test.cc M be/src/runtime/tmp-file-mgr.cc M be/src/runtime/tmp-file-mgr.h 10 files changed, 320 insertions(+), 73 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/20/9420/1 -- 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: newchange Gerrit-Change-Id: I5aa7b424209b1a5ef8dc7d04c5ba58788e91aad7 Gerrit-Change-Number: 9420 Gerrit-PatchSet: 1 Gerrit-Owner: Gabor Kaszab
