impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-2399: Check for mem limit in allocations in parquet scanner and decompressor.
Date Thu, 03 Mar 2016 20:56:12 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Skye Wanderman-Milne <skye@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message