Marcel Kornacker has posted comments on this change. Change subject: IMPALA-3748: minimum buffer requirements in planner ......................................................................
Patch Set 13: (23 comments) http://gerrit.cloudera.org:8080/#/c/5847/13/be/src/scheduling/query-schedule.cc File be/src/scheduling/query-schedule.cc: Line 212 do we still need this? http://gerrit.cloudera.org:8080/#/c/5847/13/be/src/service/query-exec-state.cc File be/src/service/query-exec-state.cc: Line 396: // Add info strings consumed by CM: Estimated mem and tables missing stats. update Line 402: if (query_exec_request.__isset.per_host_min_reservation_bytes) { can we ditch the _bytes everywhere and declare it's in bytes? http://gerrit.cloudera.org:8080/#/c/5847/13/fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java File fe/src/main/java/org/apache/impala/planner/AnalyticEvalNode.java: Line 252: long perInstanceMinBufferBytes = 2 * SPILLABLE_BUFFER_BYTES; is this covered by a test case? http://gerrit.cloudera.org:8080/#/c/5847/13/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: Line 103: private final static int MAX_THREAD_TOKENS_PER_CORE = 3; no idea where that's from, but it seems wrong. http://gerrit.cloudera.org:8080/#/c/5847/13/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java File fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java: Line 80: long numPartitions = rename to perinstance- http://gerrit.cloudera.org:8080/#/c/5847/13/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java File fe/src/main/java/org/apache/impala/planner/KuduScanNode.java: Line 268: // TODO: add an estimate remove todo (we already have a todo to remove the estimates) http://gerrit.cloudera.org:8080/#/c/5847/13/fe/src/main/java/org/apache/impala/planner/KuduTableSink.java File fe/src/main/java/org/apache/impala/planner/KuduTableSink.java: Line 60: if (explainLevel.ordinal() >= TExplainLevel.EXTENDED.ordinal()) { this if block is in every data sink's explainstring. move it into a function. http://gerrit.cloudera.org:8080/#/c/5847/13/fe/src/main/java/org/apache/impala/planner/PipelinedPlanNodeSet.java File fe/src/main/java/org/apache/impala/planner/PipelinedPlanNodeSet.java: Line 85: long minBufferBytes = 0L; minReservationBytes Line 93: node.computeResourceProfile(queryOptions); rather than calling this here, call it after creating plan fragments. Line 126: sink.computeResourceProfile(queryOptions); same here http://gerrit.cloudera.org:8080/#/c/5847/13/fe/src/main/java/org/apache/impala/planner/PlanFragment.java File fe/src/main/java/org/apache/impala/planner/PlanFragment.java: Line 202: public int getNumNodes() { do we still need this? Line 228: * Estimates the per-fragment-instance number of distinct values of exprs based on the not super-important, but: should it be "per-fragment instance number"? Line 297: str.append(String.format("%s:PLAN FRAGMENT [%s]", fragmentId_.toString(), also call getfragmentheaderstring() here http://gerrit.cloudera.org:8080/#/c/5847/13/fe/src/main/java/org/apache/impala/planner/PlanNode.java File fe/src/main/java/org/apache/impala/planner/PlanNode.java: Line 276: TQueryOptions queryOptions, TExplainLevel detailLevel) { tqueryoptions already contains the explain level. either pass in mt_dop explicitly or remove detaillevel. Line 621: * Compute resources consumed when executing this PlanNode. also mention that you're populating resourceProfile_ http://gerrit.cloudera.org:8080/#/c/5847/13/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: Line 344 this disappeared (but the comment for it didn't) Line 361: long perHostMinReservationBytes = -1; remove -bytes http://gerrit.cloudera.org:8080/#/c/5847/13/fe/src/main/java/org/apache/impala/planner/ResourceProfile.java File fe/src/main/java/org/apache/impala/planner/ResourceProfile.java: Line 33: private final long memBytesEstimate_; memEstimateBytes_ http://gerrit.cloudera.org:8080/#/c/5847/13/fe/src/main/java/org/apache/impala/planner/SortNode.java File fe/src/main/java/org/apache/impala/planner/SortNode.java: Line 241: long perInstanceMinBufferBytes = 3 * SPILLABLE_BUFFER_BYTES; -reservation instead of bufferbytes http://gerrit.cloudera.org:8080/#/c/5847/13/testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test File testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test: Line 21: ---- PARALLELPLANS you should set mt_dop explicitly if you want to test this. this doesn't look any different than the distributed plan, what are you testing for? would be good to point that out at the top of this file. also, what about single-node plans? Line 504: Per-Host Resource Reservation: Memory=0B what happened here? Line 871: Per-Host Resource Reservation: Memory=0B ? -- 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: 13 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
