Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21277 )
Change subject: IMPALA-12988: Calculate an unbounded version of CpuAsk ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/21277/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21277/1//COMMIT_MSG@16 PS1, Line 16: MAX_FRAGMENT_INSTANCES_PER_NODE options accordingly. > If these bounds don't make sense to apply with multiple executor groups, th The current bounding still make sense. It still enforced by to determine parallelism to run. What changes is just that we now count 2 kind of CpuAsk and compare with the unbounded one in lower EG. http://gerrit.cloudera.org:8080/#/c/21277/1/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/21277/1/common/thrift/Query.thrift@1035 PS1, Line 1035: > need to clarify what this means and why we have this concept. Is one core s Rephrase the doc. http://gerrit.cloudera.org:8080/#/c/21277/1/fe/src/main/java/org/apache/impala/planner/CostingSegment.java File fe/src/main/java/org/apache/impala/planner/CostingSegment.java: http://gerrit.cloudera.org:8080/#/c/21277/1/fe/src/main/java/org/apache/impala/planner/CostingSegment.java@80 PS1, Line 80: private CoreCount createCoreCount(boolean isUnbounded) { > Can you make this getTopNode() then call this by both createCoreCount() and Merged getFragment, createCoreCount, and createUnboundedCoreCount into one method. Note that CostingSegment either have sink_ set or none at all. If sink_ is set, then nodes_ can be empty. http://gerrit.cloudera.org:8080/#/c/21277/1/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/1/fe/src/main/java/org/apache/impala/planner/PlanFragment.java@1233 PS1, Line 1233: LOG.info("maxParallelism_=" + maxParallelism_ + " boundedParallelism=" > line too long (99 > 90) Done http://gerrit.cloudera.org:8080/#/c/21277/1/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: http://gerrit.cloudera.org:8080/#/c/21277/1/fe/src/main/java/org/apache/impala/planner/Planner.java@590 PS1, Line 590: int coresRequiredUnbounded = unboundedCores.total(); > Move this assignment to the else branch for clarity. I'm not really clear w This is a bug. Replaced with unboundedCores. http://gerrit.cloudera.org:8080/#/c/21277/1/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/1/fe/src/main/java/org/apache/impala/service/Frontend.java@313 PS1, Line 313: // Default to 1 if copyTQueryExecRequestFieldsForExplain() is not called. > Can this default to 1 instead? Done -- 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: 2 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: David Rorke <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Wed, 10 Apr 2024 06:14:23 +0000 Gerrit-HasComments: Yes
