Adar Dembo has posted comments on this change. Change subject: KUDU-1952 Remove round-robin for block placement ......................................................................
Patch Set 18: (9 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 144: Nit: got some extra whitespace here. Line 325: CountFiles(path, &blocks_in_path); Should ASSERT_OK() here. 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: } > Done Not 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: Env::Default() Use env_ Line 72: testing::internal::Random r_; This is a little unusual; use the Kudu Random from util/random. http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: Line 460: // natural tablet_id, select a data dir randomly. > Done Looks like you didn't add the DCHECK() though. http://gerrit.cloudera.org:8080/#/c/6636/18/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: Line 21: #include <boost/optional.hpp> Nit: this belongs with the gflags/glog includes since boost is part of the "project", not in the "system". Line 540: Random r(GetRandomSeed32()); Let's make this a member of the DataDirManager and initialize it just once, when constructing the DataDirManager. Actually, let's make the output of std::default_random_engine() a member, and seed it with GetRandomSeed32(). http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: Line 51: > No, forward declaration usually doesn't work for STL containers since I'm n Right, I forgot about that. Sorry. Could you put the DataDirGroup into a namespace called 'internal' to make it clear that it's not for use outside of data_dirs.{h,cc}? -- 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: 18 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
