Andrew Wong has posted comments on this change. Change subject: KUDU-1952 Remove round-robin for block placement ......................................................................
Patch Set 21: (8 comments) http://gerrit.cloudera.org:8080/#/c/6636/18/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: Line 134: if (basename == kInstanceMetadataFileName) { > warning: parameter 'dirname' is unused [misc-unused-parameters] Done Line 144: // hierarchy, ignoring '.', '..', and file 'kInstanceMetadataFileName'. > Nit: got some extra whitespace here. Done Line 325: // Verify the results. Each path has dot, dotdot, instance file. > Should ASSERT_OK() here. Done http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/data_dirs-test.cc File src/kudu/fs/data_dirs-test.cc: Line 96: for (int i = 0; i < pb.uuid_indices().size(); i++) { > Not done? Rrg good catch, done. http://gerrit.cloudera.org:8080/#/c/6636/18/src/kudu/fs/data_dirs-test.cc File src/kudu/fs/data_dirs-test.cc: PS18, Line 47: ), > Use env_ Done Line 72: DataDirGroupPB pb_; > This is a little unusual; use the Kudu Random from util/random. More than unusual, unneeded. These tests don't require any RNG of their own, just that of the DataDirManager. http://gerrit.cloudera.org:8080/#/c/6636/18/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: Line 21: #include <cerrno> > Nit: this belongs with the gflags/glog includes since boost is part of the Ah I see, it's a dependency. Done. Line 540: } > Let's make this a member of the DataDirManager and initialize it just once, The output of std::default_random_engine() would be constant; it's the input r.Next() that's changing to instantiate the engine, providing different rng across runs. I went with your first suggestion. Actually that resulted in a race since GetNextDataDir only read-locks, and since the shuffle needs a new seed every run to select different directories every time, it modifies the RNG state. -- 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: 21 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@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
