Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16369 )
Change subject: IMPALA-9636: Don't run retried query on the blacklisted nodes ...................................................................... Patch Set 16: (3 comments) http://gerrit.cloudera.org:8080/#/c/16369/13/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/16369/13/be/src/scheduling/admission-controller.cc@1579 PS13, Line 1579: } : if (output_schedules->empty()) { : // Retried query could not be scheduled since all executors are blacklisted. : // To keep consistent with the other blacklisting logic, set not_admitted_reason as : // REASON_NO_EXECUTOR_GROUPS. : queue_node->not_admitted_reason = REASON_NO_EXECUTOR_GROUPS; : > I can move into the loop and add TODO for supporting multiple executor grou Actually, looks fine as is. http://gerrit.cloudera.org:8080/#/c/16369/16/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/16369/16/be/src/scheduling/admission-controller.cc@1557 PS16, Line 1557: bool all_blacklisted; why do you need this? can't you just check if 'NumExecutors() == 0'? http://gerrit.cloudera.org:8080/#/c/16369/16/be/src/scheduling/executor-group.cc File be/src/scheduling/executor-group.cc: http://gerrit.cloudera.org:8080/#/c/16369/16/be/src/scheduling/executor-group.cc@52 PS16, Line 52: if (!blacklisted_executor_found) return nullptr; I think it a bit confusing to return nullptr if either (1) nothing was filtered out, or (2) all executors were removed. these seem like two very different conditions. I think the best way to simply this is to just always return the filtered_group and never return nullptr. the call can check if NumExecutors == 0, and it doesn't really matter if the filtered_group didn't have anything filtered. -- To view, visit http://gerrit.cloudera.org:8080/16369 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I00bc1b5026efbd0670ffbe57bcebc457d34cb105 Gerrit-Change-Number: 16369 Gerrit-PatchSet: 16 Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Mon, 14 Sep 2020 23:01:42 +0000 Gerrit-HasComments: Yes
