Michael Ho has posted comments on this change. Change subject: IMPALA-2399: Check for mem limit in allocations in parquet scanner and decompressor. ......................................................................
Patch Set 7: (5 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 st It seems a bit inconsistent to rely on another class to construct a status object, esp if we may consider moving state->LogMemLimitExceeded() as details of the status object. Since the #include are in the cc file, I suppose it's still marginally acceptable. 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 th I keep it as is to avoid overly long function call statement. 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 Done Line 268: } > not for this change, but I wonder if the stuff we pass to LogError() should That's worth considering as it may be hard to identify the details in the log when there are multiple queries in session. The only thing I am not so sure is whether we will always log the entire status string somewhere all the time. Line 273: DCHECK_GE(failed_allocation_size, 0); > this DCHECK could be in the Log function. Done -- 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
