impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3208: max row size option
Date Thu, 17 Aug 2017 21:55:32 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-3208: max_row_size option
......................................................................


Patch Set 10:

(3 comments)

Starting with my first high level question as I may be missing something

http://gerrit.cloudera.org:8080/#/c/7629/10/common/thrift/PlanNodes.thrift
File common/thrift/PlanNodes.thrift:

PS10, Line 503:   4: optional i64 max_row_buffer_size
              : }
maybe this will become clear to me as I review more code, but why does this have to go in
this struct? Does it really be different for different nodes? I think it'd be easier to reason
about if you didn't have to think about this extra per-node resource knob.


http://gerrit.cloudera.org:8080/#/c/7629/10/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java
File fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java:

PS10, Line 251:     // Analytic uses a single buffer size.
Why? It looks like every other node has the same behavior - can this just do the same? Then
we could avoid making the max row buffer bytes a new per-node option (i.e. it'd be the same
max).


http://gerrit.cloudera.org:8080/#/c/7629/10/fe/src/main/java/org/apache/impala/planner/Planner.java
File fe/src/main/java/org/apache/impala/planner/Planner.java:

PS10, Line 66: setMemEstimateBytes(MIN_PER_HOST_MEM_ESTIMATE_BYTES)
             :       .setMinReservationBytes(0)
I think you could give this last line another tab, for good measure


-- 
To view, visit http://gerrit.cloudera.org:8080/7629
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic70f6dddbcef124bb4b329ffa2e42a74a1826570
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message