Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/16369 )
Change subject: WIP IMPALA-9636: Don't run retried query on the blacklisted nodes ...................................................................... Patch Set 2: (6 comments) general approach LGTM, just a few comments so far http://gerrit.cloudera.org:8080/#/c/16369/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16369/2//COMMIT_MSG@15 PS2, Line 15: filter nit: filters http://gerrit.cloudera.org:8080/#/c/16369/2//COMMIT_MSG@15 PS2, Line 15: executer nit: executor http://gerrit.cloudera.org:8080/#/c/16369/2//COMMIT_MSG@15 PS2, Line 15: executers nit: executors http://gerrit.cloudera.org:8080/#/c/16369/2/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/16369/2/be/src/runtime/coordinator.cc@497 PS2, Line 497: std:: nit: you don't need to use the std:: prefix inside .cc files (they only need to be used in .h files). http://gerrit.cloudera.org:8080/#/c/16369/2/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: http://gerrit.cloudera.org:8080/#/c/16369/2/be/src/scheduling/admission-controller.cc@1218 PS2, Line 1218: const ExecutorGroup* schedule_executor_group = : ExecutorGroup::GetOrCreateExecutorGroup( : executor_group, request.blacklisted_backend_ids); would a simpler way to do this just be to create a copy of 'executor_group' and then just add a method ExecutorGroup::RemoveExecutors(unordered_set<UniqueIdPB>& blacklisted_backend_ids). RemoveExecutors would be similar to the existing RemoveExecutor method, and they two methods could potentially share some common code. I think this should simplify the code a little bit. http://gerrit.cloudera.org:8080/#/c/16369/2/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/16369/2/be/src/service/client-request-state.cc@193 PS2, Line 193: blacklisted_backend_ids_.clear(); should this method ever be called more than once? if not, it might be better to replace this was a DCHECK asserting that blacklisted_backend_ids_ is empty. -- 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: 2 Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Fri, 28 Aug 2020 00:52:06 +0000 Gerrit-HasComments: Yes
