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 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/2203/7/be/src/common/status.cc File be/src/common/status.cc: Line 20: #include "runtime/runtime-state.h" it seems a bit unfortunate to add the dependency in this direction since status is such a low-level primitive. What do you think about instead making the new method a member of MemTracker or RuntimeState? http://gerrit.cloudera.org:8080/#/c/2203/7/be/src/exec/hdfs-parquet-scanner.cc File be/src/exec/hdfs-parquet-scanner.cc: Line 755: len, "StringValue"); nit: get rid of 'details' variables, but okay to ignore if you prefer it this way. Line 757: parent_->state_, details, (int64_t)len); i agree its better to not introduce more SetMemLimitExceeded() calls since we want to get rid of that as part of this work. http://gerrit.cloudera.org:8080/#/c/2203/7/be/src/runtime/runtime-state.cc File be/src/runtime/runtime-state.cc: Line 244: void RuntimeState::LogFailedAllocDetails(const MemTracker* tracker, "Log" could mean LOG/VLOG rather than LogError() and failed allocations are not exactly the same as mem limit exceeded (they could happen for other reasons), so how about calling this thing: LogErrorMemLimitExceeded()? Line 268: } not for this change, but I wonder if the stuff we pass to LogError() should instead become detail to the MemLimitExceeded status itself. Line 273: DCHECK_GE(failed_allocation_size, 0); this DCHECK could be in the Log function. -- 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: 7 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
