Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/19033 )
Change subject: IMPALA-11604 Planner changes for CPU usage ...................................................................... Patch Set 45: (13 comments) Many thanks for the rework. Added comments for the commit message in this review session, mainly to help me understand the adjustment algorithm better. Plan to do another round review of the adjustment algorithm itself once I get the feedback. http://gerrit.cloudera.org:8080/#/c/19033/44//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19033/44//COMMIT_MSG@144 PS44, Line 144: same segment will have their Process Each blocking segment is a subtree of the PlanNodes in the fragment with a blocking root and blocking/non-blocking leaves. All other nodes in the subtree are non-blocking. http://gerrit.cloudera.org:8080/#/c/19033/44//COMMIT_MSG@146 PS44, Line 146: called Input ProcessingCost, while the last cost in nit the http://gerrit.cloudera.org:8080/#/c/19033/44//COMMIT_MSG@154 PS44, Line 154: t.co nit missing 's' http://gerrit.cloudera.org:8080/#/c/19033/44//COMMIT_MSG@158 PS44, Line 158: May need to further explain, say with the concept of a blocking fragment. It is not clear to me which fragment is adjusted. http://gerrit.cloudera.org:8080/#/c/19033/44//COMMIT_MSG@161 PS44, Line 161: just the nit decides http://gerrit.cloudera.org:8080/#/c/19033/44//COMMIT_MSG@177 PS44, Line 177: meaning that nit "At the blocking fragment" is better here as it 'blocking fragment' is well defined. http://gerrit.cloudera.org:8080/#/c/19033/44//COMMIT_MSG@177 PS44, Line 177: s can run in same. use of blocking segment is better. http://gerrit.cloudera.org:8080/#/c/19033/44//COMMIT_MSG@178 PS44, Line 178: core per fragme It is better to define CoreRequirement first before use. It is not clear whether it is for a fragment or for a node. http://gerrit.cloudera.org:8080/#/c/19033/44//COMMIT_MSG@188 PS44, Line 188: pared against the total CPU Old name. http://gerrit.cloudera.org:8080/#/c/19033/44//COMMIT_MSG@195 PS44, Line 195: Co query_cpu_requirement_scale or its new form should be listed here as well. http://gerrit.cloudera.org:8080/#/c/19033/44//COMMIT_MSG@200 PS44, Line 200: m i Do we need a min_threads control? If not, please add a comment here to explain the adjustment is always increasing. http://gerrit.cloudera.org:8080/#/c/19033/44//COMMIT_MSG@212 PS44, Line 212: : for completeness, better to list them here as like the three FE ones. http://gerrit.cloudera.org:8080/#/c/19033/44/testdata/workloads/functional-planner/queries/PlannerTest/tpcds-processing-cost.test File testdata/workloads/functional-planner/queries/PlannerTest/tpcds-processing-cost.test: http://gerrit.cloudera.org:8080/#/c/19033/44/testdata/workloads/functional-planner/queries/PlannerTest/tpcds-processing-cost.test@75 PS44, Line 75: ss_sold_date_sk = > Done. Add max-parallelism and fragment-costs as well. Nice. Thanks! -- To view, visit http://gerrit.cloudera.org:8080/19033 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If32dc770dfffcdd0be2b5555a789a7720952c68a Gerrit-Change-Number: 19033 Gerrit-PatchSet: 45 Gerrit-Owner: Qifan Chen <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Fri, 10 Feb 2023 16:24:56 +0000 Gerrit-HasComments: Yes
