Tim Armstrong has posted comments on this change. Change subject: IMPALA-5890: Abort queries if scanner hits IO errors ......................................................................
Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/8011/3//COMMIT_MSG Commit Message: Line 19: - Then during error handling, the BaseSequenceScanner calls SkipToSync() > I tried my repro for text files and looked at the code and saw that the tex Thanks, this is good to know. I'm not too concerned about the parquet case - more fuzzing seems like a good idea but not specifically related to this change. Line 32: I tested this by running the repro from the JIRA and impalad did not Do you think it's reasonable to turn this into an automated test? Would be nice to include it with the fix but could be a follow-up. http://gerrit.cloudera.org:8080/#/c/8011/3/be/src/exec/scanner-context.cc File be/src/exec/scanner-context.cc: Line 365: Status ScannerContext::Stream::WrapIoError(const Status& status) { > It doesn't. The profile only show the "General IO error" and the error log Yeah I think there's some underlying issue with MergeStatus(). Not sure how hard it is to fix it. Since this wrapping thing is temporary, maybe we should just include the error message directly in the status instead of merging it? Kind of ugly but it will work ok I think http://gerrit.cloudera.org:8080/#/c/8011/4/be/src/exec/scanner-context.cc File be/src/exec/scanner-context.cc: Line 367: if (status.ok() || status.code() == TErrorCode::SCANNER_CONTEXT_WRAPPED_IO_ERROR) { I looked at the other errors coming from the I/O mgr. I don't think we should wrap CANCELLED or MEM_LIMIT_EXCEEDED, which I think this does now. -- To view, visit http://gerrit.cloudera.org:8080/8011 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I44dc95184c241fbcdbdbebad54339530680d3509 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
