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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
