Marcel Kornacker has posted comments on this change. Change subject: IMPALA-3748: minimum buffer requirements in planner ......................................................................
Patch Set 9: (8 comments) http://gerrit.cloudera.org:8080/#/c/5847/9/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: Line 392: // Minimum bytes of spillable buffers required per host. refer to reservation rather than "spillable buffers". http://gerrit.cloudera.org:8080/#/c/5847/9/fe/src/main/java/org/apache/impala/common/PrintUtils.java File fe/src/main/java/org/apache/impala/common/PrintUtils.java: Line 59: public static String printInstances(String prefix, long numInstances) { printNumInstances. i thought this would print the actual hosts. Line 63: public static String printPerInstanceMemEstimate(String prefix, why not have a print() or getExplainString() function in the resource profile? Line 78: * Print the header for a fragment in an explain plan. probably best to move explain-related functionality into PlanNode. http://gerrit.cloudera.org:8080/#/c/5847/9/fe/src/main/java/org/apache/impala/planner/PlanNode.java File fe/src/main/java/org/apache/impala/planner/PlanNode.java: Line 120: // estimated per-instance memory consumed for this plan node in bytes; i thought we agreed that plan nodes would have their own resource profile (which would subsume these instance vars). http://gerrit.cloudera.org:8080/#/c/5847/9/fe/src/main/java/org/apache/impala/planner/ResourceProfile.java File fe/src/main/java/org/apache/impala/planner/ResourceProfile.java: Line 1: package org.apache.impala.planner; missing boilerplate comments Line 7: // If the computed values are valid - i.e. there were > 1 plan nodes in the set. couldn't this be for a single node? Line 11: // Minimum number of spillable buffers required to execute. where spillable = reservation against buffer pool (because that's all that's using the buffer pool reservations right now)? in that case, rename to something that indicates the long-term purpose (minReservationBytes?) -- To view, visit http://gerrit.cloudera.org:8080/5847 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1e358182bcf2bc5fe5c73883eb97878735b12d37 Gerrit-PatchSet: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
