[kudu-CR] [master] extra tests for the placement policy
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11562 ) Change subject: [master] extra tests for the placement policy .. [master] extra tests for the placement policy Added a few more tests for the replica placement logic. This is to verify how an extra replica is placed in case of a tablet with RF=5. RF=5 test scenarios are interesting in exercising the replica placement logic vs RF=3 scenarios because placing two replicas into one location does not constitute a violation of placement policy in case of RF=5. So, those RF=5 scenarios allows to verify how the placement logic works during initial replica placement and while adding an extra replica when it's possible to place more than one replica into same location. This is a follow-up to ebb2852d99ed27c26e65c3569d5cb515754b2937. Change-Id: I49748eca01743fb94846ccd611c5f1c7ddb20c04 Reviewed-on: http://gerrit.cloudera.org:8080/11562 Tested-by: Kudu Jenkins Reviewed-by: Will Berkeley --- M src/kudu/master/placement_policy-test.cc M src/kudu/master/placement_policy.h 2 files changed, 200 insertions(+), 5 deletions(-) Approvals: Kudu Jenkins: Verified Will Berkeley: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11562 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I49748eca01743fb94846ccd611c5f1c7ddb20c04 Gerrit-Change-Number: 11562 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [master] extra tests for the placement policy
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11562 ) Change subject: [master] extra tests for the placement policy .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11562 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I49748eca01743fb94846ccd611c5f1c7ddb20c04 Gerrit-Change-Number: 11562 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 11 Oct 2018 16:58:08 + Gerrit-HasComments: No
[kudu-CR] [master] extra tests for the placement policy
Hello Will Berkeley, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11562 to look at the new patch set (#3). Change subject: [master] extra tests for the placement policy .. [master] extra tests for the placement policy Added a few more tests for the replica placement logic. This is to verify how an extra replica is placed in case of a tablet with RF=5. RF=5 test scenarios are interesting in exercising the replica placement logic vs RF=3 scenarios because placing two replicas into one location does not constitute a violation of placement policy in case of RF=5. So, those RF=5 scenarios allows to verify how the placement logic works during initial replica placement and while adding an extra replica when it's possible to place more than one replica into same location. This is a follow-up to ebb2852d99ed27c26e65c3569d5cb515754b2937. Change-Id: I49748eca01743fb94846ccd611c5f1c7ddb20c04 --- M src/kudu/master/placement_policy-test.cc M src/kudu/master/placement_policy.h 2 files changed, 200 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/11562/3 -- To view, visit http://gerrit.cloudera.org:8080/11562 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I49748eca01743fb94846ccd611c5f1c7ddb20c04 Gerrit-Change-Number: 11562 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [master] extra tests for the placement policy
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/11562 )
Change subject: [master] extra tests for the placement policy
..
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11562/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11562/1//COMMIT_MSG@11
PS1, Line 11: The test scenario for RF=5 is chosen to exercise the logic where
two
: replicas per a location does not constitute yet a placement policy
: violation.
> I'm not sure what this sentence says.
Done
http://gerrit.cloudera.org:8080/#/c/11562/1/src/kudu/master/placement_policy-test.cc
File src/kudu/master/placement_policy-test.cc:
http://gerrit.cloudera.org:8080/#/c/11562/1/src/kudu/master/placement_policy-test.cc@598
PS1, Line 598: 3
> I think it's worth naming this as 'num_replicas', here and below.
Done
http://gerrit.cloudera.org:8080/#/c/11562/1/src/kudu/master/placement_policy-test.cc@601
PS1, Line 601: ASSERT_GE(1, m.count("A_ts0") + m.count("A_ts1"));
: ASSERT_GE(1, m.count("B_ts0") + m.count("B_ts1"));
: ASSERT_GE(1, m.count("C_ts0"));
: ASSERT_GE(1, m.count("D_ts0"));
: ASSERT_GE(1, m.count("E_ts0"));
: ASSERT_EQ(3, m.count("A_ts0") + m.count("A_ts1") +
: m.count("B_ts0") + m.count("B_ts1") +
: m.count("C_ts0") + m.count("D_ts0") +
m.count("E_ts0"));
> Could you write a quick note explaining what the intent of the checks is, i
Done
http://gerrit.cloudera.org:8080/#/c/11562/1/src/kudu/master/placement_policy-test.cc@625
PS1, Line 625: talbet
> tablet
Done
http://gerrit.cloudera.org:8080/#/c/11562/1/src/kudu/master/placement_policy-test.cc@697
PS1, Line 697: ASSERT_TRUE(
: extra_ts->permanent_uuid() == "A_ts2" ||
: extra_ts->permanent_uuid() == "C_ts0")
> Why is this expected?
Among the locations where an additional replica can be placed, location A and
location C have the least load. As for the preferences within location A, the
replica is placed using power-of-two choices among the available servers.
I added corresponding comment.
http://gerrit.cloudera.org:8080/#/c/11562/1/src/kudu/master/placement_policy-test.cc@704
PS1, Line 704: // This is similar to PlaceExtraTablet_L5_TS10_RF5 but 16 tablet
servers instead
: // of 10 and slightly different replica distribution.
> What specifically is this testing then?
Actually, this assumed to be a test that verifies how evenly the replicas are
placed among the servers in locations A and B. I updated the test.
http://gerrit.cloudera.org:8080/#/c/11562/1/src/kudu/master/placement_policy-test.cc@726
PS1, Line 726: ASSERT_TRUE(extra_ts->permanent_uuid() == "A_ts1" ||
: extra_ts->permanent_uuid() == "A_ts2" ||
: extra_ts->permanent_uuid() == "A_ts3" ||
: extra_ts->permanent_uuid() == "B_ts1" ||
: extra_ts->permanent_uuid() == "B_ts2" ||
: extra_ts->permanent_uuid() == "B_ts3")
> Why is this pick expected?
Updated the overall description.
--
To view, visit http://gerrit.cloudera.org:8080/11562
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49748eca01743fb94846ccd611c5f1c7ddb20c04
Gerrit-Change-Number: 11562
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Thu, 04 Oct 2018 20:35:03 +
Gerrit-HasComments: Yes
[kudu-CR] [master] extra tests for the placement policy
Hello Will Berkeley, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11562 to look at the new patch set (#2). Change subject: [master] extra tests for the placement policy .. [master] extra tests for the placement policy Added a few more tests for the replica placement logic. This is to verify how an extra replica is placed in case of a tablet with RF=5. The test scenario for RF=5 is chosen to exercise the logic where two replicas per a location does not constitute yet a placement policy violation. This is a follow-up to ebb2852d99ed27c26e65c3569d5cb515754b2937. Change-Id: I49748eca01743fb94846ccd611c5f1c7ddb20c04 --- M src/kudu/master/placement_policy-test.cc M src/kudu/master/placement_policy.h 2 files changed, 200 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/11562/2 -- To view, visit http://gerrit.cloudera.org:8080/11562 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I49748eca01743fb94846ccd611c5f1c7ddb20c04 Gerrit-Change-Number: 11562 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley
[kudu-CR] [master] extra tests for the placement policy
Will Berkeley has posted comments on this change. (
http://gerrit.cloudera.org:8080/11562 )
Change subject: [master] extra tests for the placement policy
..
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11562/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11562/1//COMMIT_MSG@11
PS1, Line 11: The test scenario for RF=5 is chosen to exercise the logic where
two
: replicas per a location does not constitute yet a placement policy
: violation.
I'm not sure what this sentence says.
http://gerrit.cloudera.org:8080/#/c/11562/1/src/kudu/master/placement_policy-test.cc
File src/kudu/master/placement_policy-test.cc:
http://gerrit.cloudera.org:8080/#/c/11562/1/src/kudu/master/placement_policy-test.cc@598
PS1, Line 598: 3
I think it's worth naming this as 'num_replicas', here and below.
http://gerrit.cloudera.org:8080/#/c/11562/1/src/kudu/master/placement_policy-test.cc@601
PS1, Line 601: ASSERT_GE(1, m.count("A_ts0") + m.count("A_ts1"));
: ASSERT_GE(1, m.count("B_ts0") + m.count("B_ts1"));
: ASSERT_GE(1, m.count("C_ts0"));
: ASSERT_GE(1, m.count("D_ts0"));
: ASSERT_GE(1, m.count("E_ts0"));
: ASSERT_EQ(3, m.count("A_ts0") + m.count("A_ts1") +
: m.count("B_ts0") + m.count("B_ts1") +
: m.count("C_ts0") + m.count("D_ts0") +
m.count("E_ts0"));
Could you write a quick note explaining what the intent of the checks is, i.e.
"Make sure that X".
http://gerrit.cloudera.org:8080/#/c/11562/1/src/kudu/master/placement_policy-test.cc@625
PS1, Line 625: talbet
tablet
http://gerrit.cloudera.org:8080/#/c/11562/1/src/kudu/master/placement_policy-test.cc@697
PS1, Line 697: ASSERT_TRUE(
: extra_ts->permanent_uuid() == "A_ts2" ||
: extra_ts->permanent_uuid() == "C_ts0")
Why is this expected?
http://gerrit.cloudera.org:8080/#/c/11562/1/src/kudu/master/placement_policy-test.cc@704
PS1, Line 704: // This is similar to PlaceExtraTablet_L5_TS10_RF5 but 16 tablet
servers instead
: // of 10 and slightly different replica distribution.
What specifically is this testing then?
http://gerrit.cloudera.org:8080/#/c/11562/1/src/kudu/master/placement_policy-test.cc@726
PS1, Line 726: ASSERT_TRUE(extra_ts->permanent_uuid() == "A_ts1" ||
: extra_ts->permanent_uuid() == "A_ts2" ||
: extra_ts->permanent_uuid() == "A_ts3" ||
: extra_ts->permanent_uuid() == "B_ts1" ||
: extra_ts->permanent_uuid() == "B_ts2" ||
: extra_ts->permanent_uuid() == "B_ts3")
Why is this pick expected?
--
To view, visit http://gerrit.cloudera.org:8080/11562
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I49748eca01743fb94846ccd611c5f1c7ddb20c04
Gerrit-Change-Number: 11562
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Thu, 04 Oct 2018 17:43:40 +
Gerrit-HasComments: Yes
[kudu-CR] [master] extra tests for the placement policy
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11562 ) Change subject: [master] extra tests for the placement policy .. Patch Set 1: Verified+1 Unrelated flake in RaftConsensusParamReplicationModesITest.Test_KUDU_1735/1 -- To view, visit http://gerrit.cloudera.org:8080/11562 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I49748eca01743fb94846ccd611c5f1c7ddb20c04 Gerrit-Change-Number: 11562 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 02 Oct 2018 04:14:05 + Gerrit-HasComments: No
[kudu-CR] [master] extra tests for the placement policy
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11562 ) Change subject: [master] extra tests for the placement policy .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11562 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I49748eca01743fb94846ccd611c5f1c7ddb20c04 Gerrit-Change-Number: 11562 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Will Berkeley
[kudu-CR] [master] extra tests for the placement policy
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11562 Change subject: [master] extra tests for the placement policy .. [master] extra tests for the placement policy Added a few more tests for the replica placement logic. This is to verify how an extra replica is placed in case of a tablet with RF=5. The test scenario for RF=5 is chosen to exercise the logic where two replicas per a location does not constitute yet a placement policy violation. This is a follow-up to ebb2852d99ed27c26e65c3569d5cb515754b2937. Change-Id: I49748eca01743fb94846ccd611c5f1c7ddb20c04 --- M src/kudu/master/placement_policy-test.cc M src/kudu/master/placement_policy.h 2 files changed, 163 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/11562/1 -- To view, visit http://gerrit.cloudera.org:8080/11562 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I49748eca01743fb94846ccd611c5f1c7ddb20c04 Gerrit-Change-Number: 11562 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
