Marcel Kornacker 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::GetInstance()->backend_address(). same for scheduler c'tor. Line 228: if (FLAGS_use_statestore && statestore_port > 0) { please move all of the duplicated code, incl. initializer list, into a shared c'tor or something like that, there's too much copy & paste at this point. 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 where that hasn't happened yet) 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 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? 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. Line 1484: // Check if the local backend descriptor is in the list of known this function is already too long. break this up. where did this block come from? was it moved? Line 1903: // Only for coordinators, initialize the HS2 and Beeswax services. rephrase to fix grammar 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" 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? -- 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 <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
