Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management ...................................................................... Patch Set 5: (22 comments) Thx for the review, please see my comments and PS5. http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.h@96 PS2, Line 96: typedef std::function<BackendDescriptorPtr()> BackendDescriptorPtrFn; > Should we document that no locks are held during by the calling thread of t Done. It's a bit fuzzy where to put the bulk of the comments for the callbacks and they're currently spread across the member, function, and typedef. Let me know if you'd prefer to collect them in a single place (which one) and refer to it at the other locations. http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@94 PS2, Line 94: > nit: no Done http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@98 PS2, Line 98: > I feel like a lot of these messages could actually be VLOG(2) or even VLOG( I agree that they help, I used -vmodule already while developing this so I changed them to VLOG(1). http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@108 PS2, Line 108: } else { > We're assuming that only one thread executes the below code at a time, righ Yes, that's the statestore contract as far as I understand, that the membership callback needs to return before being called again. DFAKE_SCOPED_LOCK_THREAD_LOCKED looks like it enforces that the actual same thread runs the method every time. I think we want DFAKE_SCOPED_LOCK, but let me know if I misunderstood your intent. http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@113 PS2, Line 113: } > Conditional fits on one line? Done http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@145 PS2, Line 145: if (!status.ok()) { > This seems like something we should definitely log at INFO or probably even Done http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@152 PS2, Line 152: // descriptor as part of the statestore update. If it is empty, then either that > Same here Done http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@201 PS2, Line 201: > typo Done http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@242 PS2, Line 242: Status status = thrift_serializer_.SerializeToString(&local_be_desc, &item.value); > It seems like this really should be a LOG(FATAL) or a CHECK since we never Done, I'm happy to fix these things while we're here. http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@252 PS2, Line 252: const BackendIdMap& current_backends) { > This is O(n^2) where n in the cluster size, in some sense because we potent Yes, this logic was there before, that on every statestore update the impalad would construct a set (not even unordered) and check it for cancellation. An easy optimization we can do is only call this method if a backend was removed. Unfortunately we cannot do that for the Frontend notification below. http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@282 PS2, Line 282: > nit: whitespace Done http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@283 PS2, Line 283: bool ClusterMembershipMgr::NeedsLocalBackendUpdate(const Snapshot& state, > I don't personally like the redundant .get() here but it is a matter of tas I got used to it and don't really mind it, so I'll keep it for consistency (hard to tell how often we rely on the cast with a simple grep). I'm happy to try and change our recommendation on it though. $ git grep -E "get.*= nullptr" | wc -l 131 http://gerrit.cloudera.org:8080/#/c/13207/2/be/src/scheduling/cluster-membership-mgr.cc@312 PS2, Line 312: if (group_be.is_quiescing) { > return false here and on l308? Done http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h File be/src/scheduling/executor-group.h: http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@38 PS3, Line 38: typedef std::vector<IpAddr> IpAddrs; > Explicit constructors? Removed it. http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@41 PS3, Line 41: /// the host is actually contained in executor_map_. The caller must make sure that the > Any reason this shouldn't be a vector? That's generally a better default. I This is probably a leftover from when we had concurrent updates to the state during scheduling. Made it a vector. http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@43 PS3, Line 43: /// membership manager returns immutable snapshots to guarantee this. > What's the thread safety story for the returned list? Added a comment. Strictly speaking it's the responsibility of the users to not update the executors while holding a reference. The CMM ensures this using the immutable snapshots. http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@47 PS3, Line 47: IpAddrs GetAllExecutorIps() const; > Can you document whether these append to the vectors or replace the content Switched to return-by-value http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@49 PS3, Line 49: /// Returns all executor backends. Note that during tests this can include multiple > I think these could do with a basic description. There may be some details Done http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@62 PS3, Line 62: /// be nullptr if the caller only wants to check whether the lookup succeeds. Use this > What happens if the executor is removed with RemoveExecutor()? Added a comment, after switching to a vector, the user must make sure that the group doesn't change. Should we apply some pattern to make the group immutable? I don't know of a good way to forbid methods from running on a non-const reference/pointer. http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@65 PS3, Line 65: bool LookUpExecutorIp(const Hostname& hostname, IpAddr* ip) const; > Maybe worth documenting lifetime/thread safety of this? Done http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.h@71 PS3, Line 71: const TBackendDescriptor* LookUpBackendDesc(const TNetworkAddress& host) const; > Maybe a good time to switch these to std::unordered_map? Done http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.cc File be/src/scheduling/executor-group.cc: http://gerrit.cloudera.org:8080/#/c/13207/3/be/src/scheduling/executor-group.cc@109 PS3, Line 109: > LIKELY annotation probably isn't necessary. Ok to leave. Done -- 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: 5 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: Thu, 02 May 2019 21:50:18 +0000 Gerrit-HasComments: Yes
