Tim Armstrong has posted comments on this change. Change subject: IMPALA-5890: Abort queries if scanner hits IO errors ......................................................................
Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/8011/8/be/src/exec/base-sequence-scanner.cc File be/src/exec/base-sequence-scanner.cc: Line 180: state_->LogError(ErrorMsg(TErrorCode::SEQUENCE_SCANNER_PARSE_ERROR, Unfortunately it looks like when abort_on_error=1 we depend on this status wrapping to add a filename and offset for some parse errors, e.g. "Table schema is not a record". I think the filename is pretty essential to understand and fix any errors so I think we should be careful not to drop it. Ideally I think all parse errors from the scanners would just include that context when original constructed. Not sure how much of a project it would be to go through all the sequence scanners and fix that. It's much cleaner using LogOrReturnError(). Maybe in the meantime we should just do: if (!status.IsCancelled() && !status.IsMemLimitExceeded() && !status.IsDiskIoError()) { state_->LogError(ErrorMsg(TErrorCode::SEQUENCE_SCANNER_PARSE_ERROR, ...) } http://gerrit.cloudera.org:8080/#/c/8011/6/be/src/runtime/disk-io-mgr.cc File be/src/runtime/disk-io-mgr.cc: Line 544: return Status(TErrorCode::DISK_IO_ERROR, > Should we re-use DISK_IO_ERROR for those, even though they're technically n I think it makes sense to use DISK_IO_ERROR for now if we interpret its meaning broadly as "an error that prevented the disk I/O manager completing a request range". -- 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: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
