David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1952 Remove round-robin for block placement ......................................................................
Patch Set 35: (12 comments) http://gerrit.cloudera.org:8080/#/c/6636/35/src/kudu/fs/data_dirs-test.cc File src/kudu/fs/data_dirs-test.cc: PS35, Line 78: non_existent_opts nit" non_existent_tablet_opts PS35, Line 107: CreateDataDirGroup maybe this should be names CreateDataDirGroupForTablet? PS35, Line 109: dd_manager_->DeleteDataDirGroup(test_tablet_name_); this doesn't return Status? PS35, Line 110: LoadDataDirGroupFromPB maybe this should be named LoadDataDirGroupForTabletFromPB? PS35, Line 112: create a duplicate tablet. this is weird. you mean that it won't crate a duplicate data dir group for the tablet, right? PS35, Line 115: Tried to load DataDirGroup but tablet already exists similarly here you mean: "Tried to load DataDirGroup for tablet but it already exists" right? PS35, Line 125: DataDirGroup not found for tablet similar comment as in the test above PS35, Line 140: FLAGS_fs_target_data_dirs_per_tablet = 3; : const double kNumTablets = 20; may make this relative then? something like FLAGS_fs_target_data_dirs_per_tablet = kNumDirs / 3; const double kNumTablets = kNumDirs * 2; PS35, Line 143: 20 refer to the constant name here and at the end of the sentence PS35, Line 158: static_cast<double>( dont think you need this static cast since the numerator is already a double PS35, Line 159: Standard deviation: add more info PS35, Line 161: // Looping this 1000 times with an assertion of 2.0 resulted in a couple failures. : // Asserting 3.0 should give a safe non-flaky buffer. : ASSERT_LE(stddev, 3.0); think we're setting ourselves up for flakyness here, unfortunately it's hard to come up with a number that is full proof unless you want to somehow run this with a huge number of data dirs same comment for the test below. I think these could be made into bechmarks or somesuch instead and you could try with different counts and report the numbers on the commit message that should be proof enough that it worked without having to make assertions that are hard to make specially with such a low data dir count. If you decide to go the huge number of data dirs route instead I'd like to see somewhat tight assertions based on the theory behind the "power of two choices" -- To view, visit http://gerrit.cloudera.org:8080/6636 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9828147f4fa5c4d7f6ed23441dca5a116b8cb11b Gerrit-PatchSet: 35 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
