[kudu-CR] [location awareness] replica selection honors placement policy
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11207 ) Change subject: [location_awareness] replica selection honors placement policy .. [location_awareness] replica selection honors placement policy This patch introduces placement policy into the catalog manager's replica selection process. The replica selection logic is factored out into the new PlacementPolicy class. In essence (for details see [1]), the placement policy is about: * in case of N locations, N > 2, not placing the majority of replicas in one location * spreading replicas evenly among available locations * within a location, spreading replicas evenly among tablet servers This patch also adds a few test scenarios for the new functionality. [1] https://s.apache.org/location-awareness-design Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd Reviewed-on: http://gerrit.cloudera.org:8080/11207 Reviewed-by: Adar Dembo Reviewed-by: Will Berkeley Tested-by: Kudu Jenkins --- M src/kudu/gutil/map-util.h M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h A src/kudu/master/placement_policy-test.cc A src/kudu/master/placement_policy.cc A src/kudu/master/placement_policy.h M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h 11 files changed, 1,228 insertions(+), 224 deletions(-) Approvals: Adar Dembo: Looks good to me, but someone else must approve Will Berkeley: Looks good to me, approved Kudu Jenkins: Verified -- 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: merged Gerrit-Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd Gerrit-Change-Number: 11207 Gerrit-PatchSet: 13 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] replica selection honors placement policy
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11207 ) Change subject: [location_awareness] replica selection honors placement policy .. Patch Set 12: Code-Review+2 -- 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: 12 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 12 Sep 2018 22:51:07 + Gerrit-HasComments: No
[kudu-CR] [location awareness] replica selection honors placement policy
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 12: Code-Review+1 -- 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: 12 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 12 Sep 2018 22:38:57 + Gerrit-HasComments: No
[kudu-CR] [location awareness] replica selection honors placement policy
Hello Will Berkeley, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11207 to look at the new patch set (#12). Change subject: [location_awareness] replica selection honors placement policy .. [location_awareness] replica selection honors placement policy This patch introduces placement policy into the catalog manager's replica selection process. The replica selection logic is factored out into the new PlacementPolicy class. In essence (for details see [1]), the placement policy is about: * in case of N locations, N > 2, not placing the majority of replicas in one location * spreading replicas evenly among available locations * within a location, spreading replicas evenly among tablet servers This patch also adds a few test scenarios for the new functionality. [1] https://s.apache.org/location-awareness-design Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd --- M src/kudu/gutil/map-util.h M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h A src/kudu/master/placement_policy-test.cc A src/kudu/master/placement_policy.cc A src/kudu/master/placement_policy.h M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h 11 files changed, 1,228 insertions(+), 224 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/11207/12 -- 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: newpatchset Gerrit-Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd Gerrit-Change-Number: 11207 Gerrit-PatchSet: 12 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] replica selection honors placement policy
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 11: (3 comments) http://gerrit.cloudera.org:8080/#/c/11207/11/src/kudu/master/placement_policy.cc File src/kudu/master/placement_policy.cc: http://gerrit.cloudera.org:8080/#/c/11207/11/src/kudu/master/placement_policy.cc@134 PS11, Line 134: located > Nit: "are located" Done http://gerrit.cloudera.org:8080/#/c/11207/11/src/kudu/master/placement_policy.cc@145 PS11, Line 145: existing_set.emplace(std::move(ts)); > EmplaceOrDie? Or do you expect duplicates? Done http://gerrit.cloudera.org:8080/#/c/11207/11/src/kudu/master/placement_policy.cc@178 PS11, Line 178: // Get the load of the location: a location with N tablet servers and R replicas : // has load R/N. : // : // Parameters: : // 'location' The location in question. : // 'locations_info' Information on tablet replicas slated for placement, : //but not created yet. That's the placement information : //on to-be-replicas in the context of optimizing tablet : //replica distribution in the cluster. > Move this to the header file. 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: 11 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 12 Sep 2018 22:38:09 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] replica selection honors placement policy
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11207 ) Change subject: [location_awareness] replica selection honors placement policy .. Patch Set 11: LGTM besides Adar's comments. -- 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: 11 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 12 Sep 2018 22:17:18 + Gerrit-HasComments: No
[kudu-CR] [location awareness] replica selection honors placement policy
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 11: (3 comments) http://gerrit.cloudera.org:8080/#/c/11207/11/src/kudu/master/placement_policy.cc File src/kudu/master/placement_policy.cc: http://gerrit.cloudera.org:8080/#/c/11207/11/src/kudu/master/placement_policy.cc@134 PS11, Line 134: located Nit: "are located" http://gerrit.cloudera.org:8080/#/c/11207/11/src/kudu/master/placement_policy.cc@145 PS11, Line 145: existing_set.emplace(std::move(ts)); EmplaceOrDie? Or do you expect duplicates? http://gerrit.cloudera.org:8080/#/c/11207/11/src/kudu/master/placement_policy.cc@178 PS11, Line 178: // Get the load of the location: a location with N tablet servers and R replicas : // has load R/N. : // : // Parameters: : // 'location' The location in question. : // 'locations_info' Information on tablet replicas slated for placement, : //but not created yet. That's the placement information : //on to-be-replicas in the context of optimizing tablet : //replica distribution in the cluster. Move this to the header file. -- 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: 11 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 12 Sep 2018 22:14:16 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] replica selection honors placement policy
Hello Will Berkeley, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11207 to look at the new patch set (#11). Change subject: [location_awareness] replica selection honors placement policy .. [location_awareness] replica selection honors placement policy This patch introduces placement policy into the catalog manager's replica selection process. The replica selection logic is factored out into the new PlacementPolicy class. In essence (for details see [1]), the placement policy is about: * in case of N locations, N > 2, not placing the majority of replicas in one location * spreading replicas evenly among available locations * within a location, spreading replicas evenly among tablet servers This patch also adds a few test scenarios for the new functionality. [1] https://s.apache.org/location-awareness-design Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd --- M src/kudu/gutil/map-util.h M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h A src/kudu/master/placement_policy-test.cc A src/kudu/master/placement_policy.cc A src/kudu/master/placement_policy.h M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h 11 files changed, 1,229 insertions(+), 224 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/11207/11 -- 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: newpatchset Gerrit-Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd Gerrit-Change-Number: 11207 Gerrit-PatchSet: 11 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] replica selection honors placement policy
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11207 ) Change subject: [location_awareness] replica selection honors placement policy .. Patch Set 10: Code-Review+2 -- 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: 10 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 12 Sep 2018 15:32:13 + Gerrit-HasComments: No
[kudu-CR] [location awareness] replica selection honors placement policy
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 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc@3391
PS7, Line 3391: for (const auto& ts_desc : ts_descs) {
: if (IsRaftConfigMember(ts_desc->permanent_uuid(),
cstate_.committed_config())) {
: InsertOrDie(&existing, ts_desc);
: }
: }
> It isn't very important because this code path isn't hot and the number of
Done:
http://gerrit.cloudera.org:8080/11429
--
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: 7
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Wed, 12 Sep 2018 04:45:24 +
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] replica selection honors placement policy
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 10: Code-Review+1 Will should +2 since there have been some changes since his last review. -- 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: 10 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 11 Sep 2018 22:28:59 + Gerrit-HasComments: No
[kudu-CR] [location awareness] replica selection honors placement policy
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 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 11 Sep 2018 22:19:11 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] replica selection honors placement policy
Hello Will Berkeley, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11207 to look at the new patch set (#10). Change subject: [location_awareness] replica selection honors placement policy .. [location_awareness] replica selection honors placement policy This patch introduces placement policy into the catalog manager's replica selection process. The replica selection logic is factored out into the new PlacementPolicy class. In essence (for details see [1]), the placement policy is about: * in case of N locations, N > 2, not placing the majority of replicas in one location * spreading replicas evenly among available locations * within a location, spreading replicas evenly among tablet servers This patch also adds a few test scenarios for the new functionality. [1] https://s.apache.org/location-awareness-design Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd --- M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h A src/kudu/master/placement_policy-test.cc A src/kudu/master/placement_policy.cc A src/kudu/master/placement_policy.h M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h 10 files changed, 1,217 insertions(+), 211 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/11207/10 -- 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: newpatchset Gerrit-Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd Gerrit-Change-Number: 11207 Gerrit-PatchSet: 10 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] replica selection honors placement policy
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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 10 Sep 2018 20:23:21 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] replica selection honors placement policy
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: (2 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: replacement > If I'm not mistaken, in all current use cases this method is called to add https://gerrit.cloudera.org/#/c/11402/ 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@245 PS8, Line 245: location_info.emplace(std::move(location), 0).first->second++; > I used LookupOrInsert(), but I think I'll add EmplaceOrInsert() wrapper. https://gerrit.cloudera.org/#/c/11401/ -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Sat, 08 Sep 2018 00:05:17 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] replica selection honors placement policy
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11207 ) Change subject: [location_awareness] replica selection honors placement policy .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- 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: deleteReviewer Gerrit-Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd Gerrit-Change-Number: 11207 Gerrit-PatchSet: 9 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] replica selection honors placement policy
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: Verified+1 Unrelated flake in General.OutsideOfAnyTestCase -- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 07 Sep 2018 20:30:44 + Gerrit-HasComments: No
[kudu-CR] [location awareness] replica selection honors placement policy
Hello Will Berkeley, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11207 to look at the new patch set (#9). Change subject: [location_awareness] replica selection honors placement policy .. [location_awareness] replica selection honors placement policy This patch introduces placement policy into the catalog manager's replica selection process. The replica selection logic is factored out into the new PlacementPolicy class. In essence (for details see [1]), the placement policy is about: * in case of N locations, N > 2, not placing the majority of replicas in one location * spreading replicas evenly among available locations * within a location, spreading replicas evenly among tablet servers This patch also adds a few test scenarios for the new functionality. [1] https://s.apache.org/location-awareness-design Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd --- M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h A src/kudu/master/placement_policy-test.cc A src/kudu/master/placement_policy.cc A src/kudu/master/placement_policy.h M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h 10 files changed, 1,167 insertions(+), 199 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/11207/9 -- 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: newpatchset Gerrit-Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd Gerrit-Change-Number: 11207 Gerrit-PatchSet: 9 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] replica selection honors placement policy
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& 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 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
> 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* 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
[kudu-CR] [location awareness] replica selection honors placement policy
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
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
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>& 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_po
[kudu-CR] [location awareness] replica selection honors placement policy
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 7:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/11207/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11207/7//COMMIT_MSG@16
PS7, Line 16: avaialable
> available
Done
http://gerrit.cloudera.org:8080/#/c/11207/7//COMMIT_MSG@17
PS7, Line 17: load
> replicas
Done
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h@878
PS7, Line 878: Select required number tablet servers to place replicas for the
given newly
: // created table
> Howabout "Select the tablet servers where the given newly-created tablet's
Woops, indeed that was a strange wording. It seems I wrote that while heaving
that headache last Friday, sorry.
Updated.
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h@880
PS7, Line 880: is
> are
Done
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h@882
PS7, Line 882: The
> nit: Remove
Done
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc@4336
PS7, Line 4336: RETURN_NOT_OK(policy.PlaceTabletReplicas(nreplicas,
&descriptors));
> Can we add the tablet ID and table name to the status returned here?
Done
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc@4345
PS7, Line 4345: // TODO(unknown): this is temporary, we will use only UUIDs
> nit: I don't think this is temporary anymore :). If there's no JIRA related
Done
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.h
File src/kudu/master/placement_policy.h:
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.h@41
PS7, Line 41: the
> Remove
Done
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.h@73
PS7, Line 73: of
> Remove
Done
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc
File src/kudu/master/placement_policy.cc:
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@55
PS7, Line 55: a
> the
Done
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@62
PS7, Line 62: deltas: information location of replicas that not yet placed
> Stale?
Done
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@63
PS7, Line 63: ltd
> Out of order w.r.t the function parameters.
Done
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@74
PS7, Line 74: auto num_live_replicas = accumulate(
: ts_descriptors.begin(), ts_descriptors.end(), 0,
: [](int val, const shared_ptr& desc) {
: return val + desc->num_live_replicas();
: });
: const auto liit = locations_info.find(location);
: if (liit != locations_info.end()) {
: num_live_replicas += liit->second;
: }
> Is this computing R then adding R to it, so we return 2x the load?
Nope, that's computing R for already existing replicas, and then adding number
of replicas just slated for placement, but not instantiated yet. I'll add a
comment to clarify on that.
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@107
PS7, Line 107: >
> Should we (D?)CHECK that > doesn't happen?
That's a good idea; added CHECK_LE().
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@115
PS7, Line 115: for
> Remove
Done
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@115
PS7, Line 115: to
> of
Done.
I really need to re-read my writings at least twice.
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@125
PS7, Line 125: if (location_per_load.empty()) {
: // If placing an additional replica to any location will
make it containing
: // the majority of replilcas of the same tablet, place the
replica into
: // one of the less loaded locations (see below).
: location_per_load.swap(location_per_load_majority);
: }
> If this happens and the RF is 2k, then each location must have k replicas,
That's right, but there is a case when it's not enough tablet serves to place
the requested number of replicas.
The way how it's written is a convenient way to avoid placing a majority of
replicas at the same location, even if the load-based criterion would favor
that placement.
It seems some comments were wrong/misleading
[kudu-CR] [location awareness] replica selection honors placement policy
Hello Will Berkeley, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11207 to look at the new patch set (#8). Change subject: [location_awareness] replica selection honors placement policy .. [location_awareness] replica selection honors placement policy This patch introduces placement policy into the catalog manager's replica selection process. The replica selection logic is factored out into the new PlacementPolicy class. In essence (for details see [1]), the placement policy is about: * in case of N locations, N > 2, not placing the majority of replicas in one location * spreading replicas evenly among available locations * within a location, spreading replicas evenly among tablet servers This patch also adds a few test scenarios for the new functionality. [1] https://s.apache.org/location-awareness-design Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd --- M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h A src/kudu/master/placement_policy-test.cc A src/kudu/master/placement_policy.cc A src/kudu/master/placement_policy.h M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h 10 files changed, 1,099 insertions(+), 194 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/11207/8 -- 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: newpatchset Gerrit-Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd Gerrit-Change-Number: 11207 Gerrit-PatchSet: 8 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] replica selection honors placement policy
Will Berkeley has posted comments on this change. (
http://gerrit.cloudera.org:8080/11207 )
Change subject: [location_awareness] replica selection honors placement policy
..
Patch Set 7:
(21 comments)
Just small things and I think this is good to go.
http://gerrit.cloudera.org:8080/#/c/11207/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11207/7//COMMIT_MSG@16
PS7, Line 16: avaialable
available
http://gerrit.cloudera.org:8080/#/c/11207/7//COMMIT_MSG@17
PS7, Line 17: load
replicas
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h@878
PS7, Line 878: Select required number tablet servers to place replicas for the
given newly
: // created table
Howabout "Select the tablet servers where the given newly-created tablet's
replica will be placed"?
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h@880
PS7, Line 880: is
are
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.h@882
PS7, Line 882: The
nit: Remove
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc@3391
PS7, Line 3391: for (const auto& ts_desc : ts_descs) {
: if (IsRaftConfigMember(ts_desc->permanent_uuid(),
cstate_.committed_config())) {
: InsertOrDie(&existing, ts_desc);
: }
: }
It isn't very important because this code path isn't hot and the number of TS
will be small, but wouldn't it be better to iterate over the UUIDs of the peers
in the config to populate the set, rather than filtering all TS using the
config?
The difference is that 'ts_descs' excludes presumed dead TS, so if we changed
how 'existing' is populated it might contain presumed dead TS. This should be
harmless since we'd never propose to use a presumed dead TS since our options
come from 'ts_descs'.
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc@4336
PS7, Line 4336: RETURN_NOT_OK(policy.PlaceTabletReplicas(nreplicas,
&descriptors));
Can we add the tablet ID and table name to the status returned here?
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/catalog_manager.cc@4345
PS7, Line 4345: // TODO(unknown): this is temporary, we will use only UUIDs
nit: I don't think this is temporary anymore :). If there's no JIRA related to
this I feel like we can drop it.
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.h
File src/kudu/master/placement_policy.h:
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.h@41
PS7, Line 41: the
Remove
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.h@73
PS7, Line 73: of
Remove
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc
File src/kudu/master/placement_policy.cc:
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@55
PS7, Line 55: a
the
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@62
PS7, Line 62: deltas: information location of replicas that not yet placed
Stale?
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@63
PS7, Line 63: ltd
Out of order w.r.t the function parameters.
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@74
PS7, Line 74: auto num_live_replicas = accumulate(
: ts_descriptors.begin(), ts_descriptors.end(), 0,
: [](int val, const shared_ptr& desc) {
: return val + desc->num_live_replicas();
: });
: const auto liit = locations_info.find(location);
: if (liit != locations_info.end()) {
: num_live_replicas += liit->second;
: }
Is this computing R then adding R to it, so we return 2x the load?
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@107
PS7, Line 107: >
Should we (D?)CHECK that > doesn't happen?
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@115
PS7, Line 115: to
of
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@115
PS7, Line 115: for
Remove
http://gerrit.cloudera.org:8080/#/c/11207/7/src/kudu/master/placement_policy.cc@125
PS7, Line 125: if (location_per_load.empty()) {
: // If placing an additional replica to any location will
make it containing
: // the majority of replilcas of the same tablet, place the
replica into
: // one of the less loaded locations (see below).
: location_per_load.swa
[kudu-CR] [location awareness] replica selection honors placement policy
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 7: Verified+1 Unrelated flake in: RaftConsensusParamReplicationModesITest.Test_KUDU_1735/1 -- 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: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Sat, 01 Sep 2018 06:05:43 + Gerrit-HasComments: No
[kudu-CR] [location awareness] replica selection honors placement policy
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11207 ) Change subject: [location_awareness] replica selection honors placement policy .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- 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: deleteReviewer Gerrit-Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd Gerrit-Change-Number: 11207 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] replica selection honors placement policy
Hello Will Berkeley, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11207 to look at the new patch set (#7). Change subject: [location_awareness] replica selection honors placement policy .. [location_awareness] replica selection honors placement policy This patch introduces placement policy into the catalog manager's replica selection process. The replica selection logic is factored out into the new PlacementPolicy class. In essence (for details see [1]), the placement policy is about: * in case of N locations, N > 2, not placing the majority of replicas in one location * spreading replicas evenly among avaialable locations * within a location, spreading load evenly among tablet servers This patch also adds a few test scenarios for the new functionality. [1] https://s.apache.org/location-awareness-design Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd --- M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h A src/kudu/master/placement_policy-test.cc A src/kudu/master/placement_policy.cc A src/kudu/master/placement_policy.h M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h 10 files changed, 1,086 insertions(+), 194 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/11207/7 -- 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: newpatchset Gerrit-Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd Gerrit-Change-Number: 11207 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] replica selection honors placement policy
Hello Will Berkeley, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11207 to look at the new patch set (#6). Change subject: [location_awareness] replica selection honors placement policy .. [location_awareness] replica selection honors placement policy This patch introduces placement policy into the catalog manager's replica selection process. The replica selection logic is factored out into the new PlacementPolicy class. In essence (for details see [1]), the placement policy is about: * in case of N locations, N > 2, not placing the majority of replicas in one location * spreading replicas evenly among avaialable locations * within a location, spreading load evenly among tablet servers This patch also adds a few test scenarios for the new functionality. [1] https://s.apache.org/location-awareness-design Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd --- M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h A src/kudu/master/placement_policy-test.cc A src/kudu/master/placement_policy.cc A src/kudu/master/placement_policy.h M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h 10 files changed, 1,085 insertions(+), 194 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/11207/6 -- 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: newpatchset Gerrit-Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd Gerrit-Change-Number: 11207 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] replica selection honors placement policy
Hello Will Berkeley, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11207 to look at the new patch set (#5). Change subject: [location_awareness] replica selection honors placement policy .. [location_awareness] replica selection honors placement policy This patch introduces placement policy into the catalog manager's replica selection process. The replica selection logic is factored out into the new PlacementPolicy class. In essence (for details see [1]), the placement policy is about: * in case of N locations, N > 2, not placing the majority of replicas in one location * spreading replicas evenly among avaialable locations * within a location, spreading load evenly among tablet servers This patch also adds a few test scenarios for the new functionality. [1] https://s.apache.org/location-awareness-design Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd --- M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h A src/kudu/master/placement_policy-test.cc A src/kudu/master/placement_policy.cc A src/kudu/master/placement_policy.h M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h 10 files changed, 1,089 insertions(+), 194 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/11207/5 -- 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: newpatchset Gerrit-Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd Gerrit-Change-Number: 11207 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] replica selection honors placement policy
Hello Will Berkeley, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11207 to look at the new patch set (#4). Change subject: [location_awareness] replica selection honors placement policy .. [location_awareness] replica selection honors placement policy This patch introduces placement policy into the catalog manager's replica selection process. The replica selection logic is factored out into the new PlacementPolicy class. In essence (for details see [1]), the placement policy is about: * in case of N locations, N > 2, not placing the majority of replicas in one location * spreading replicas evenly among avaialable locations * within a location, spreading load evenly among tablet servers This patch also adds a few test scenarios for the new functionality. [1] https://s.apache.org/location-awareness-design Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd --- M src/kudu/master/CMakeLists.txt M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h A src/kudu/master/placement_policy-test.cc A src/kudu/master/placement_policy.cc A src/kudu/master/placement_policy.h M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h 10 files changed, 1,090 insertions(+), 194 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/11207/4 -- 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: newpatchset Gerrit-Change-Id: I4169098abf17d5591d4c1675561afc15b5477fcd Gerrit-Change-Number: 11207 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] replica selection honors placement policy
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 3:
(30 comments)
http://gerrit.cloudera.org:8080/#/c/11207/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11207/3//COMMIT_MSG@7
PS3, Line 7: [master]
> nit: Consider making the label [location_awareness] to be consistent with t
Done
http://gerrit.cloudera.org:8080/#/c/11207/3//COMMIT_MSG@9
PS3, Line 9: placement policy
> Would you mind adding another sentence explaining the single policy that is
Done
http://gerrit.cloudera.org:8080/#/c/11207/3//COMMIT_MSG@10
PS3, Line 10:
> nit: One space.
Done
http://gerrit.cloudera.org:8080/#/c/11207/3//COMMIT_MSG@13
PS3, Line 13: Added
> nit: This patch also adds a few...
Done
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/catalog_manager.h@524
PS3, Line 524: std::string, int
> Please add a short comment explaining what the key and value types are.
I removed this since it moved into the PlacementPolicy class.
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/catalog_manager.h@880
PS3, Line 880: online
> nit: I think the standard term in "live".
Done
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/catalog_manager.h@880
PS3, Line 880: grouped by location
: // (as specified by 'ltd')
> What is 'ltd'? Looks like this comment is stale.
Done
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/catalog_manager.h@887
PS3, Line 887: PlacementPolicy*
> nit: Naively I expected this to be a cref because something called a Placem
Ah, that's because of the random generator pointer being the member of the
PlacementPolicy class. Maybe a better approach would be passing the generator
as a parameter in all those methods since the Placement policy just keeps the
raw pointer anyway. As of now I added 'mutable' specified for the rng_ raw
pointer member.
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h
File src/kudu/master/placement_policy.h:
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@39
PS3, Line 39: placing
> place
Done
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@39
PS3, Line 39: at proper tablet servers
> on tablet servers according to some rules.
Done
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@40
PS3, Line 40: class PlacementPolicy {
> Please add another sentence with a brief description of the specific policy
Done
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@49
PS3, Line 49: destructed
> "destroyed", or say that 'rng' must outlive of the PlacementPolicy.
Done
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@82
PS3, Line 82: // TODO (aserbin): add the reference to the document once it's
in the repo
> nit: End with .
Done
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@100
PS3, Line 100: is
> Remove extra word.
Done
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.h@108
PS3, Line 108: : it's in constructor and cached
> I think if we make this member const it enforces these two things.
Ah, good idea.
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc
File src/kudu/master/placement_policy.cc:
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@51
PS3, Line 51:
> a
Done
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@51
PS3, Line 51:
> the
Done
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@51
PS3, Line 51: repilcas
> replicas
Done
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@52
PS3, Line 52: of
> Remove extra 'of'.
Done
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@55
PS3, Line 55: unordered_map
> Is this a ReplicaLocationsInfo type?
Done
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@57
PS3, Line 57: DCHECK(it != ltd.end());
: if (it == ltd.end()) {
: return numeric_limits::max();
: }
> Is there a reason we shouldn't always crash if we break the invariant that
Done
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@62
PS3, Line 62: DCHECK(!ts_descriptors.empty());
> Same thing here. Shouldn't every location we consider here have at least on
Done
http://gerrit.cloudera.org:8080/#/c/11207/3/src/kudu/master/placement_policy.cc@81
PS3, Line 81: unordered_map
> This too is a ReplicaLocationsInfo, right?
Done
http://gerrit.cloudera.org:8080/#/c/112
