Wenzhe Zhou 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 13: (4 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@1555 PS13, Line 1555: const ExecutorGroup* filtered_executor_group = > Since the "executor_group" is a constant object and maybe used by other que Use unique_ptr for temporary executor group. http://gerrit.cloudera.org:8080/#/c/16369/13/be/src/scheduling/admission-controller.cc@1562 PS13, Line 1562: if (created && num_executors == 0) { : // All executors are blacklisted for retried query. : delete filtered_executor_group; : continue; : } > I'm not sure if this is correct; before this patch, 'executor_group' could Reorganized code to create temporary filtered executor group if 'executor_group' is not equal cluster_membership_mgr_->GetEmptyExecutorGroup() so that coordinator-only queries will not be affected. http://gerrit.cloudera.org:8080/#/c/16369/13/be/src/scheduling/admission-controller.cc@1568 PS13, Line 1568: cluster_membership_mgr_->GetEmptyExecutorGroup() == filtered_executor_group > This is to compare two pointers, not objects pointed by pointers. We have s Removed this line. 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; : LOG(WARNING) << queue_node->not_admitted_reason; : } > In the future, we may support multiple executor groups. If we move the code I can move into the loop and add TODO for supporting multiple executor group. But I still prefer to leave these code out of loop. -- 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: 13 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: Sat, 12 Sep 2020 02:11:15 +0000 Gerrit-HasComments: Yes
