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 <gaborkas...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 15 Mar 2018 13:19:06 +0000 Gerrit-HasComments: Yes
