Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13207 )
Change subject: IMPALA-8460: Simplify cluster membership management ...................................................................... Patch Set 10: (11 comments) Thanks for the comments. Please see PS10. http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h File be/src/scheduling/cluster-membership-mgr.h: http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@67 PS8, Line 67: typedef std::shared_ptr<const TBackendDescriptor> BackendDescriptorPtr; > I think I would prefer if there was something in the name of this and Snaps Would BackendDescriptorSPtr work for you? I'd like to keep it from getting out-of-hand long, Maybe BeDescSharedPtr would work, too? http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@70 PS8, Line 70: typedef std::unordered_map<std::string, TBackendDescriptor> BackendIdMap; > nit: std::string in headers here and elsewhere. I guess a "using" is in a h Done http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@79 PS8, Line 79: // implicitly-defined default and copy constructors. > Probably worth documenting that we expect this to be copy-constructed. I'm not sure I'm following your thoughts behind documenting it. Should we document it for users of the struct to mark it as the intended behavior? Or for future developers who want to make changes and need to keep copy-construction supported? For now I assumed the former and added some documentation in comment and the code. http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@121 PS8, Line 121: /// steps. > Can you document thread safety for these functions and the basics of the ca Done http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.h@133 PS8, Line 133: /// membership. This callback will only be called when backends are deleted from the > Valid to call any time before Init()? Done http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc File be/src/scheduling/cluster-membership-mgr.cc: http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@39 PS8, Line 39: LOG(INFO) << "Starting cluster membership manager"; > Consider DCHECK(TestInfo::is_test()); Thx, TIL. http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@188 PS8, Line 188: ckend explicit > Yeah I like 1) since it makes the comparison more explicit. We should add a I went with 1. Do you think we should expose the fact that we rely on a host:port combination through the interface? I.e., should if be RemoveExecutor(string krpc_address) or similar instead of passing the full be desc? http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@235 PS8, Line 235: AddLo > nit: unnecessary std:: Done http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@299 PS8, Line 299: LOG(WARNING) << "Error updating frontend membership snapshot: " << status.GetDetail(); > This method lacks a check to compare the local be desc inside current_backe Done http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/cluster-membership-mgr.cc@313 PS8, Line 313: auto it = state.current_backends.find(local_backend_id_); > Would this consistency check have detected the bug you found? Do we need to Yes, that's how I found it, a host trying to quiesce itself will hit the CheckConsistency DCHECK after trying to remove its own backend descriptor. http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/executor-group.h File be/src/scheduling/executor-group.h: http://gerrit.cloudera.org:8080/#/c/13207/8/be/src/scheduling/executor-group.h@41 PS8, Line 41: /// host/IP address. > Worth making explicit that this can be copy-constructed. Either by document 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: 10 Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 16 May 2019 23:37:56 +0000 Gerrit-HasComments: Yes
