Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management ...................................................................... Patch Set 6: Code-Review+1 (5 comments) http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/runtime/exec-env.cc@454 PS5, Line 454: impala_server_ = server; : // Register the ImpalaServer with the cluster membership manager : cluster_membership_mgr_->SetLocalBeDescFn([server]() { : return server->GetLocalBackendDescriptor(); : }); : cluster_membership_mgr_->SetUpdateLocalServerFn( : [server](const ClusterMembershipMgr::BackendAddressSet& current_backends) { : > These rely only on the server still being alive, not the ExecEnv, so the ri Thanks the DCHECK would definitely help. I was also just thinking of a situation during teardown where there can be a really small window where the impala_server got destroyed and the subscriber receives an update on membership and ClusterMembershipManager tries to call the function pointer. But I think it doesnt matter because impala is already shutting down at that point http://gerrit.cloudera.org:8080/#/c/13207/6/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/13207/6/be/src/scheduling/cluster-membership-mgr.h@54 PS6, Line 54: as indicated by their backend : /// descriptor. should this also be after a TODO tag? http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.h@117 PS5, Line 117: ClusterMembershipMgr(std::string local_backend_id, StatestoreSubscriber* subscriber); : : /// Initializes instances of this class. This only sets up the statestore subscription. : /// Callbacks to the local ImpalaServer and Frontend must be registered in separate : /// steps. : Status Init(); : : /// Registers a callback to provide the local backend descriptor. : void SetLocalBeDescFn(BackendDescriptorPtrFn fn); : : /// Registers a callback to notify the local I > Both clients have different needs: The ImpalaServer only needs to learn abo Thanks, that sums it up perfectly! http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/cluster-membership-mgr.cc@255 PS5, Line 255: cons > Done. The const is redundant but I added it for readability. (See https://g oh interesting! I suspected that too, but i checked in eclipse and it was not automatically deducing the const for some reason. http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/scheduler.cc File be/src/scheduling/scheduler.cc: http://gerrit.cloudera.org:8080/#/c/13207/5/be/src/scheduling/scheduler.cc@654 PS5, Line 654: // This can happen in the short time period after the ImpalaServer has finished : // starting up (which makes the local backend available) and the next statestore : > There's a race between the ImpalaServer starting up and the statestore topi I see. Thanks for the explanation -- To view, visit http://gerrit.cloudera.org:8080/13207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3cf9a8bb060d0c6e9ec8868b7b21ce01f8740a3 Gerrit-Change-Number: 13207 Gerrit-PatchSet: 6 Gerrit-Owner: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 08 May 2019 18:11:00 +0000 Gerrit-HasComments: Yes
