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 9: (7 comments) 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: > https://gerrit.cloudera.org/#/c/11402/ Sure. 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: existing.push_back(ts_desc); Nit: emplace_back in new code. 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: > The idea was to use proximity-based iterators. I.e. location '/mega/turbo0 Makes sense, but do we currently take advantage of key sorting? Do we iterate on map entries? 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: // If the load is the same, we can just pick randomly. : return two_choices[rng->Uniform(2)]; > Actually, those should be multi_map; good catch. Could you add a test case that would fail if these weren't multi-maps? http://gerrit.cloudera.org:8080/#/c/11207/8/src/kudu/master/placement_policy.cc@379 PS8, Line 379: } // namespace master > Nope, I don't think so. The problem with raw emplace() is that it doesn't explain what your intent is re: existing keys. That's why the Emplace* (or Insert*) functions from map-util.h are nice; even when they don't reduce LOC, they do a better job of articulating intent. 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: //const auto it = ltd.find(location); : //CHECK(it != ltd.end()); Remove? http://gerrit.cloudera.org:8080/#/c/11207/9/src/kudu/master/placement_policy.cc@191 PS9, Line 191: Nit: extra space here. -- 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: Will Berkeley <[email protected]> Gerrit-Comment-Date: Mon, 10 Sep 2018 20:23:21 +0000 Gerrit-HasComments: Yes
