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 8: (7 comments) Thank you for the review! http://gerrit.cloudera.org:8080/#/c/11207/9/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11207/9/src/kudu/master/catalog_manager.cc@3394 PS9, Line 3394: } > Nit: emplace_back in new code. I'm going to address this in a separate small patch: Will pointed at the fact that iterating over all tablet servers is not that effective vs iterating just over the members of the tablet Raft group. 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@65 PS8, Line 65: typedef std::map<std::string, TSDescriptorVector> LocationToDescriptorsMap; > Makes sense, but do we currently take advantage of key sorting? Do we itera Nope, right now there is no code that would take advantage of the fact that the key are sorted. I assume making this unordered_map for a while would be OK? 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@131 PS8, Line 131: location_per_load_overflow.emplace( : GetLocationLoad(location, ltd, locations_info), location); > Could you add a test case that would fail if these weren't multi-maps? Added PlacementPolicyTest.SelectLocationTest -- it also verifies that the placement policy selects locations randomly (uniform distribution). 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()); > The problem with raw emplace() is that it doesn't explain what your intent Yes, that makes sense, but that time LookupOrEmplace() didn't exist yet, and there was code at line 380 that would help understand the intention. However, now we have LookupOrEmplace() and I updated the code to use it. 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)); > I'm not sure I understand: descs is a const ref, and move is for a copy of But it's possible to use move semantics for 'descs', sure. http://gerrit.cloudera.org:8080/#/c/11207/9/src/kudu/master/placement_policy.cc File src/kudu/master/placement_policy.cc: http://gerrit.cloudera.org:8080/#/c/11207/9/src/kudu/master/placement_policy.cc@71 PS9, Line 71: CHECK(it != ltd.end()); : // Get information on the > Remove? Done http://gerrit.cloudera.org:8080/#/c/11207/9/src/kudu/master/placement_policy.cc@191 PS9, Line 191: > Nit: extra space here. 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: 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: Tue, 11 Sep 2018 22:19:11 +0000 Gerrit-HasComments: Yes
