Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-4041: Limit catalog and admission control updates to coordinators ......................................................................
Patch Set 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/6344/2/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: Line 183: request_pool_service_.get(), metrics_.get(), backend_address_)); > no need to pass in backend_address_, you can get that with ExecEnv::GetInst Hm, I am not sure this is possible. This is the ExecEnv constructor, is it safe to call ExecEnv::GetInstance() when AdmissionController and Scheduler are initialized here? Maybe I am missing something. Line 228: if (FLAGS_use_statestore && statestore_port > 0) { > let's please not file a jira for something this small, the time involved in There is already a TODO. Done Line 349: if (admission_controller_ != NULL) RETURN_IF_ERROR(admission_controller_->Init()); > replace all NULL w/ nullptr in this file (and any other file you touch wher Done http://gerrit.cloudera.org:8080/#/c/6344/2/be/src/scheduling/admission-controller.cc File be/src/scheduling/admission-controller.cc: Line 38: DECLARE_bool(is_coordinator); > unused Done http://gerrit.cloudera.org:8080/#/c/6344/2/be/src/scheduling/admission-controller.h File be/src/scheduling/admission-controller.h: Line 210: /// Subscription manager > owned? Done http://gerrit.cloudera.org:8080/#/c/6344/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: Line 341: bind<void>(mem_fn(&ImpalaServer::CatalogUpdateCallback), this, _1, _2); > use lambda, unless impractical. you can also inline that in the call. Done Line 1484: // Check if the local backend descriptor is in the list of known > this function is already too long. break this up. Previously, it was the scheduler's responsibility to update the list of backends in the statestore. Given that the scheduler may not start in all the nodes, we need this update to happen elsewhere. I've extracted this into a separate function which is invoked only for non-coordinator nodes. Line 1903: // Only for coordinators, initialize the HS2 and Beeswax services. > rephrase to fix grammar Done http://gerrit.cloudera.org:8080/#/c/6344/2/be/src/service/impala-server.h File be/src/service/impala-server.h: Line 955: /// Impala server is a coordinator (dictated from the is_coordinator flag). > "indicated by the .. flag" Done http://gerrit.cloudera.org:8080/#/c/6344/2/tests/custom_cluster/test_coordinators.py File tests/custom_cluster/test_coordinators.py: Line 42: client = worker.service.create_beeswax_client() > same for hs2? Done -- To view, visit http://gerrit.cloudera.org:8080/6344 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5f2c74abdbcd60ac050efa323616bd41182ceff3 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Henry Robinson <he...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-HasComments: Yes
