Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11207 )
Change subject: [location_awareness] replica selection honors placement policy ...................................................................... Patch Set 8: (29 comments) http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/catalog_manager.h@878 PS8, Line 878: // Select the tablet servers where the given newly-created tablet's replica : // will be placed, populating the consensus configuration sub-object of : // the in-out parameter 'tablet'. If there are not enough live tablet servers : // to host the required number of replicas, return Status::InvalidArgument. : // 'tablet' must not be null. I might reformat/reword this a bit: // Selects the tservers where the newly-created tablet's replicas will be placed, populating its consensus configuration in the process. // // Returns InvalidArgument if there are not enough live tservers to host the required number of replicas. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/catalog_manager.cc@3402 PS8, Line 3402: replacement It's not necessarily a 'replacement' though, is it? http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/catalog_manager.cc@4339 PS8, Line 4339: table_guard.data().pb.name() table_guard.data().name() is shorter http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy-test.cc File src/kudu/master/placement_policy-test.cc: http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy-test.cc@51 PS8, Line 51: Auxiliary class Nit: gtest calls these "test fixtures". http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy-test.cc@67 PS8, Line 67: Nit: extra space here http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy-test.cc@72 PS8, Line 72: ThreadSafeRandom* rng_ptr() { return &rng_; } Just "rng()" is fine, I think. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy-test.cc@75 PS8, Line 75: // Populate LocationToDescriptorsMap given the information on the cluster. Wrong comment? http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy-test.cc@154 PS8, Line 154: Nit: extra line here. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.h File src/kudu/master/placement_policy.h: http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.h@20 PS8, Line 20: #include <stddef.h> Not cstddef? http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.h@41 PS8, Line 41: but in the future : // it could eventually be a base class for subclasses representing other : // placement policies. I like this forward thinking, but we needn't constrain ourselves to inheritance. Perhaps just "but it could enforce other placement policies in the future"? http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.h@65 PS8, Line 65: typedef std::map<std::string, TSDescriptorVector> LocationToDescriptorsMap; Why map and not unordered_map here? http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.h@84 PS8, Line 84: (must not be null) In the interest of brevity I think you can omit this; I think it's fair to assume that an OUT parameter must not be null (unless it is explicitly said otherwise). http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.h@91 PS8, Line 91: const std::set<std::shared_ptr<TSDescriptor>>& existing, Maybe we can make this a TSDescriptorVector and convert it into a set internally if necessary? That'll give the API a little more consistency. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.h@127 PS8, Line 127: The mutable specifier is added : // because the rng_ here is used as a proxy reference to the object passed : // to the constructor. It's aggregated in here just for convenience. Not really understanding this; I thought a 'mutable' member is one that you can call non-const methods on while inside a const method. That is, it's just syntactic sugar that indicates that you don't really care about the mutations happening to this member. To be clear, I think it's fine to use it on rng_ (or a lock), just the explanation seems odd. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc File src/kudu/master/placement_policy.cc: http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@70 PS8, Line 70: const auto it = ltd.find(location); : CHECK(it != ltd.end()); FindOrDie. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@84 PS8, Line 84: const auto liit = locations_info.find(location); Find* http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@92 PS8, Line 92: by load can omit http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@103 PS8, Line 103: boost::optional<string> SelectLocation( If a "" location is the same as no location, why not return that instead of wrapping in an optional? http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@113 PS8, Line 113: map<double, string> location_per_load; : map<double, string> location_per_load_overflow; May want to add that we're using map and not unordered_map in order to maintain the entries in load-sorted order. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@117 PS8, Line 117: location_tserver_num Nit: I think location_num_tservers is clearer w.r.t. conveying that this is a quantity. tserver_num suggests a specific numbered tserver, like an index. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@118 PS8, Line 118: const auto lit = locations_info.find(location); : if (lit != locations_info.end()) { Find* from map-util.h. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@131 PS8, Line 131: location_per_load_overflow.emplace( : GetLocationLoad(location, ltd, locations_info), location); Should these emplace() calls be EmplaceOrDie()? What's your expectation regarding overwrites? http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@227 PS8, Line 227: const auto it = ltd_.find(loc); : CHECK(it != ltd_.end()); FindOrDie() from map-util.h, perhaps? http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@245 PS8, Line 245: location_info.emplace(std::move(location), 0).first->second++; Ugh, something from map-util.h to improve the readability here? http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@253 PS8, Line 253: auto loc = SelectLocation(replicas_num, ltd_, location_info, rng_); If you're passing class state into SelectLocation, why not just make it a class member instead of a free function? Also, what do you think of returning a Status from SelectLocation and passing back the location as an OUT parameter? It means you could RETURN_NOT_OK_PREPEND() around it here. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@258 PS8, Line 258: const auto it = ltd_.find(location); Find* from map-util.h. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@379 PS8, Line 379: auto ins_info = ltd->emplace(std::move(location), TSDescriptorVector()); Would any of the Emplace* functions in map-util.h improve readability here? http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@380 PS8, Line 380: ins_info.first->second.emplace_back(std::move(desc)); This doesn't empty out 'descs', does it? I guess it can't because 'descs' is a cref, but it sorta looks like it does. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/ts_manager.h File src/kudu/master/ts_manager.h: http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/ts_manager.h@41 PS8, Line 41: typedef std::vector<std::shared_ptr<TSDescriptor>> TSDescriptorVector; : typedef std::map<std::string, TSDescriptorVector> LocationToDescriptorsMap; Aren't these duplicated in placement_policy.h? -- To view, visit http://gerrit.cloudera.org:8080/11207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd Gerrit-Change-Number: 11207 Gerrit-PatchSet: 8 Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Fri, 07 Sep 2018 04:26:23 +0000 Gerrit-HasComments: Yes
