Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11207 )
Change subject: [location_awareness] replica selection honors placement policy ...................................................................... Patch Set 9: (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: // 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. 'tablet' must not be null. : Status SelectReplicasForTable > I might reformat/reword this a bit: Done 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: > It's not necessarily a 'replacement' though, is it? If I'm not mistaken, in all current use cases this method is called to add a replacement replica: either because of replica movement or because of auto re-replication of a failed one. However, I agree it's better to have error messages that doesn't rely on the knowledge of the contexts they are being called from. In this context, I also found it would be nice to update the messages at around line 3425. Do you mind if I post that update as a separate changelist? http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/catalog_manager.cc@4339 PS8, Line 4339: , > table_guard.data().name() is shorter Done 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: Fixture to run > Nit: gtest calls these "test fixtures". Done http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy-test.cc@67 PS8, Line 67: r > Nit: extra space here Done http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy-test.cc@72 PS8, Line 72: ThreadSafeRandom* rng() { return &rng_; } > Just "rng()" is fine, I think. Done http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy-test.cc@75 PS8, Line 75: TSDescriptorVector GetDescriptors(const vector<string>& uuids) const { > Wrong comment? Done http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy-test.cc@154 PS8, Line 154: multiset<string> locations; > Nit: extra line here. Done 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 <cstddef> > Not cstddef? Done http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.h@41 PS8, Line 41: ness placement policy : // is about: : // * in case of N lo > I like this forward thinking, but we needn't constrain ourselves to inherit Done http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.h@65 PS8, Line 65: > Why map and not unordered_map here? The idea was to use proximity-based iterators. I.e. location '/mega/turbo0' is close to '/mega/turbo1' and outage in '/mega' affects both. Also, the number of locations should not be big (I think it's in order of tens). I added a comment. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.h@84 PS8, Line 84: letReplicas(int nr > In the interest of brevity I think you can omit this; I think it's fair to After some consideration I think it's not worth pursuing brevity in comments and documentation, but pursue clarity and avoid ambiguity. Also, we have many cases where an output parameter can be null and that's not indicated in the comments, so that seems like a moot point to me since it's not a part of the code style guide. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.h@91 PS8, Line 91: std::shared_ptr<TSDescriptor>* ts_desc) const; > Maybe we can make this a TSDescriptorVector and convert it into a set inter That's a really good point, thanks. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.h@127 PS8, Line 127: et replicas slated for placement, : // but not created yet. That's the placement information : // on to-be-replicas in the context of optimizing > Not really understanding this; I thought a 'mutable' member is one that you Right -- that's the mechanics of the mutable specifier and I didn't think I needed to document that since that's well known. The highlighted sentence was an attempt to justify using mutable here at the conceptual level: explaining why the state of the RNG may be not counted as a part of the PlacementPolicy's state. I think it's better to remove that since instead of clarification it brought confusion. Done. 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 PlacementPolicy::ReplicaLocationsInfo& locations_info) { : //const auto it = ltd.f > FindOrDie. Done http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@84 PS8, Line 84: // location. > Find* Done http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@92 PS8, Line 92: > can omit Done http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@103 PS8, Line 103: > If a "" location is the same as no location, why not return that instead of I consider boost::optional for location in TSDescriptor as a part of a given interface (that's a part of other independent changelist). I updated this to return Status as suggested in one of the comments below, so this is addressed. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@113 PS8, Line 113: // been reformatted and re-joined). : // > May want to add that we're using map and not unordered_map in order to main Done http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@117 PS8, Line 117: ported by the server > Nit: I think location_num_tservers is clearer w.r.t. conveying that this is Done http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@118 PS8, Line 118: // we batch the selection process before sending any creation commands to the : // servers themselves. > Find* from map-util.h. Done http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@131 PS8, Line 131: // If the load is the same, we can just pick randomly. : return two_choices[rng->Uniform(2)]; > Should these emplace() calls be EmplaceOrDie()? What's your expectation reg Actually, those should be multi_map; good catch. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@227 PS8, Line 227: // violate the 'one tablet replica per tablet server' policy. : string location; > FindOrDie() from map-util.h, perhaps? Done http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@245 PS8, Line 245: int nreplicas, > Ugh, something from map-util.h to improve the readability here? I used LookupOrInsert(), but I think I'll add EmplaceOrInsert() wrapper. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@253 PS8, Line 253: // Keep track of servers we've already selected, so that we don't attempt to > If you're passing class state into SelectLocation, why not just make it a c Done http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@258 PS8, Line 258: CHECK(ts); > Find* from map-util.h. Done http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@379 PS8, Line 379: } // namespace master > Would any of the Emplace* functions in map-util.h improve readability here? Nope, I don't think so. http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@380 PS8, Line 380: } // namespace kudu > This doesn't empty out 'descs', does it? I guess it can't because 'descs' i I'm not sure I understand: descs is a const ref, and move is for a copy of shared_ptr created in the for() scope. 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: // Note that TSDescriptors are never deleted, even if the TS crashes : // and has not heartbeated in quite a while. This makes it simpler to > Aren't these duplicated in placement_policy.h? Done -- 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: 9 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Fri, 07 Sep 2018 19:08:52 +0000 Gerrit-HasComments: Yes
