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 <aser...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com>
Gerrit-Comment-Date: Thu, 04 Oct 2018 20:35:03 +0000
Gerrit-HasComments: Yes

Reply via email to