Dan Hecht has posted comments on this change. Change subject: IMPALA-3337: fix "Cancelled" warnings when LIMIT clause is specified ......................................................................
Patch Set 1: (4 comments) > (4 comments) > > There is some handling of abort_on_error in hbase-scanner - do we > need to fix that too? I assume you mean hbase-scan-node.cc code? That code only handles conversion errors already, so looks okay to me, and doesn't look like it benefits from this pattern. Let me know if you had something specific in mind. http://gerrit.cloudera.org:8080/#/c/2964/1/be/src/exec/hdfs-scanner.cc File be/src/exec/hdfs-scanner.cc: Line 651: Status HdfsScanner::CheckAbortOnError(const ErrorMsg& message) const { > The name doesn't really indicate that it logs the error. When I was reading I figured it was implied by "AbortOnError" since that's the non-abort behavior, but I'm happy to change the name. LogOrReturnError() (but then that doesn't tell you it's related to abort_on_error)? other ideas? Line 654: message.error() == TErrorCode::MEM_LIMIT_EXCEEDED || > Maybe explain why these two error codes are special? Done (without trying to restate the header commit exactly). http://gerrit.cloudera.org:8080/#/c/2964/1/be/src/exec/hdfs-scanner.h File be/src/exec/hdfs-scanner.h: Line 408: non > I had to think about this a bit but it seems correct. It's kind of confusin Removed. Let me know if you had something else in mind. http://gerrit.cloudera.org:8080/#/c/2964/1/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: Line 74: limit = (iterations * 100) % 1000 + 1 > What repro rate did this achieve? 100%. Note that I didn't change the limit values, just added extra config and validation. -- To view, visit http://gerrit.cloudera.org:8080/2964 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4a91a22608e346ca21a23ea66c855eae54bbced6 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Dan Hecht <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
