Dan Hecht has posted comments on this change. Change subject: IMPALA-2399: Check for mem limit in allocations in parquet scanner and decompressor ......................................................................
Patch Set 10: (6 comments) http://gerrit.cloudera.org:8080/#/c/2203/10/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: Line 1681: if (UNLIKELY(!parse_status_.ok())) continue_execution = false; i'm not sure why the old code dropped query_status on the floor, but it might have been on purpose (if so, we should clean up the code to make it clear). But check with Skye as to why the old code did what it did. http://gerrit.cloudera.org:8080/#/c/2203/10/be/src/exec/hdfs-scanner.cc File be/src/exec/hdfs-scanner.cc: Line 85: // batch_ != NULL so Close() can be called for proper cleanup in case of failure. I don't follow this comment -- where does Close() care about batch_? Line 114: obj_pool_.Clear(); where's the dependency on batch_? http://gerrit.cloudera.org:8080/#/c/2203/10/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: Line 301: failed_allocation how about failed_alloc_size to make it clearer that this is a length. Line 305: return status; this is fine, but the only thing that does any real work is RuntimeState class, so maybe we should just make: RuntimeState::CreateMemLimitExceeded() which does both the LogError and return Status. And that can be called by SetMemLimitExceeded() too (until that function goes away). But okay to leave it like this too. http://gerrit.cloudera.org:8080/#/c/2203/10/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: Line 244: logging logging to the error log (to differentiate with just the log file logging). -- To view, visit http://gerrit.cloudera.org:8080/2203 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic70400407b7662999332448f4d1bce2cc344ca89 Gerrit-PatchSet: 10 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Skye Wanderman-Milne <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
