Tim Armstrong has posted comments on this change. Change subject: IMPALA-3748: minimum buffer requirements in planner ......................................................................
Patch Set 9: (8 comments) Made the requested changes. I think we still haven't converged on some of the finer details of how the explain plan should look, but I don't think that is likely to change the rest of the logic in the patch. 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". Done 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. Done Line 63: public static String printPerInstanceMemEstimate(String prefix, > why not have a print() or getExplainString() function in the resource profi Done Line 78: * Print the header for a fragment in an explain plan. > probably best to move explain-related functionality into PlanNode. Done - moved to PlanFragment. 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 ( I don't think we were entirely on the same page when I pushed out this patchset. I have done that in the new one. 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 Done 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? That's true. I just removed that part of the comment because it's could be invalid for other reasons. Line 11: // Minimum number of spillable buffers required to execute. > where spillable = reservation against buffer pool (because that's all that' Done -- 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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
