Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/21277 )
Change subject: IMPALA-12988: Calculate an unbounded version of CpuAsk ...................................................................... Patch Set 13: Code-Review+1 (7 comments) http://gerrit.cloudera.org:8080/#/c/21277/13//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21277/13//COMMIT_MSG@35 PS13, Line 35: "max-parallelism" max-parallelism is only shown on CPC=1 plan fragments? http://gerrit.cloudera.org:8080/#/c/21277/13//COMMIT_MSG@37 PS13, Line 37: shows show http://gerrit.cloudera.org:8080/#/c/21277/13/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/21277/13/common/thrift/Query.thrift@1038 PS13, Line 1038: executor group "Used by Frontend to do executor group-set assignment for the query. Should either be unset or set with positive value." Same for L1022 http://gerrit.cloudera.org:8080/#/c/21277/13/fe/src/main/java/org/apache/impala/planner/PlanFragment.java File fe/src/main/java/org/apache/impala/planner/PlanFragment.java: http://gerrit.cloudera.org:8080/#/c/21277/13/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@949 PS13, Line 949: return getNumInstances() % getNumNodes() == 0 ? getNumNodes() : 1; This is existing logic, but not clear why we use this approach for step count? Maybe add a comment here explaining the logic? http://gerrit.cloudera.org:8080/#/c/21277/13/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@1133 PS13, Line 1133: findUnboundedCount ? scanNode.estScanRangeAfterRuntimeFilter() : Some comment explaining why RF based estimation is used for unbounded case. scanNode.maxScannerThreads_ does take into account RF but it's also capped by max global threads. Is that the reasoning here? http://gerrit.cloudera.org:8080/#/c/21277/13/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@1304 PS13, Line 1304: thisTreeCpuCore_ = thisTreeCpuCore.total(); Not entirely clear why calculating total here is a bounded case. Same for subtreeCpuCore computation below. http://gerrit.cloudera.org:8080/#/c/21277/13/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/21277/13/fe/src/main/java/org/apache/impala/service/Frontend.java@2197 PS13, Line 2197: boolean isLastEG = (i >= numExecutorGroupSets - 1); i == (numExecutorGroupSets - 1) i can never be greater than (numExecutorGroupSets - 1) because of loop condition. -- To view, visit http://gerrit.cloudera.org:8080/21277 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5441e31088f90761062af35862be4ce09d116923 Gerrit-Change-Number: 21277 Gerrit-PatchSet: 13 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: David Rorke <dro...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Fri, 19 Apr 2024 14:04:30 +0000 Gerrit-HasComments: Yes
