Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/21257 )
Change subject: IMPALA-12980: Translate CpuAsk into admission control slots ...................................................................... Patch Set 6: (9 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 per executor and can be overridden with > processors per executor? Done http://gerrit.cloudera.org:8080/#/c/21257/5//COMMIT_MSG@16 PS5, Line 16: instances of any fragment on that backend. This can lead to > change "is simplistic" to "can lead to underestimation" Done 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 paralleli Describe the formula in the following line. AvgAdmissionSlotsPerExecutor is informational only and does not affect the actual scheduling. http://gerrit.cloudera.org:8080/#/c/21257/5//COMMIT_MSG@45 PS5, Line 45: think > thinks Done http://gerrit.cloudera.org:8080/#/c/21257/5//COMMIT_MSG@47 PS5, Line 47: > is scheduled Done 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: // the vector. > This is kind of hard to follow. There seems be assumptions that fragment in exec_params->instance_params() is indeed ordered by fragment id. I added comment in L1182 as illustration. http://gerrit.cloudera.org:8080/#/c/21257/5/be/src/scheduling/scheduler.cc@1193 PS5, Line 1193: curr_fragment_idx = finstance.fragment_idx(); > Can this be moved out of the loop? be_max_instances should be updated every time this loop proceed to the next fragment group. Moved this to the line just before where counting related variables are reset. 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: is deri > is derived Done http://gerrit.cloudera.org:8080/#/c/21257/5/fe/src/main/java/org/apache/impala/service/Frontend.java@2399 PS5, Line 2399: t > 0); > may want to assert cores_requirement > 0 and numExecutors > 0 here to detec Done -- 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: 6 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 15:51:12 +0000 Gerrit-HasComments: Yes
