Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/21257 )
Change subject: IMPALA-12980: Translate CpuAsk into admission control slots ...................................................................... Patch Set 5: (10 comments) http://gerrit.cloudera.org:8080/#/c/21257/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21257/5//COMMIT_MSG@11 PS5, Line 11: the number of processors and can be overridden with processors per executor? http://gerrit.cloudera.org:8080/#/c/21257/5//COMMIT_MSG@16 PS5, Line 16: instances of any fragment on that backend. This is simplistic, because change "is simplistic" to "can lead to underestimation" http://gerrit.cloudera.org:8080/#/c/21257/5//COMMIT_MSG@44 PS5, Line 44: AvgAdmissionSlotsPerExecutor It's not clear how this is calculated. Is it the maximum expected parallelism from above? http://gerrit.cloudera.org:8080/#/c/21257/5//COMMIT_MSG@45 PS5, Line 45: think thinks http://gerrit.cloudera.org:8080/#/c/21257/5//COMMIT_MSG@47 PS5, Line 47: AvgAdmissionSlotsPerExecutor, depending on what scheduled on that is scheduled http://gerrit.cloudera.org:8080/#/c/21257/5/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: http://gerrit.cloudera.org:8080/#/c/21257/5/be/src/scheduling/scheduler.cc@1184 PS5, Line 1184: for (auto& finstance : backend.second.exec_params->instance_params()) { This is kind of hard to follow. There seems be assumptions that fragment indexes are ordered and not unique. http://gerrit.cloudera.org:8080/#/c/21257/5/be/src/scheduling/scheduler.cc@1193 PS5, Line 1193: be_max_instances = max(be_max_instances, curr_instance_count); Can this be moved out of the loop? http://gerrit.cloudera.org:8080/#/c/21257/5/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/21257/5/fe/src/main/java/org/apache/impala/planner/CostingSegment.java@84 PS5, Line 84: Preconditions.checkState(!nodes_.isEmpty()); Can this just get computed once at the top and passed down as a recursion arg? http://gerrit.cloudera.org:8080/#/c/21257/5/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/21257/5/fe/src/main/java/org/apache/impala/service/Frontend.java@2338 PS5, Line 2338: derived is derived http://gerrit.cloudera.org:8080/#/c/21257/5/fe/src/main/java/org/apache/impala/service/Frontend.java@2399 PS5, Line 2399: cores_requirement may want to assert cores_requirement > 0 and numExecutors > 0 here to detect problems -- To view, visit http://gerrit.cloudera.org:8080/21257 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I338ca96555bfe8d07afce0320b3688a0861663f2 Gerrit-Change-Number: 21257 Gerrit-PatchSet: 5 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Tue, 09 Apr 2024 14:26:05 +0000 Gerrit-HasComments: Yes
