Adar Dembo has posted comments on this change. Change subject: KUDU-1952 Remove round-robin for block placement ......................................................................
Patch Set 17: (12 comments) http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: Line 428: ASSERT_STR_CONTAINS(s.ToString(), "DataDirGroup not found for tablet"); Before asserting on the status' string, assert on its type (i.e. ASSERT_TRUE(s.IsNotFound()) or whatever). http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/data_dirs-test.cc File src/kudu/fs/data_dirs-test.cc: PS17, Line 56: virtual void TearDown() override { : KuduTest::TearDown(); : } You can omit this since it doesn't do anything beyond what the base class does. PS17, Line 79: // Parent block manager. The selection of FBM over LBM shouldn't be relevant : // to these tests. Would it be possible to instantiate a DataDirManager and forgo the block manager altogether? That would eliminate some unnecessary test setup. Line 96: Status s = dd_manager_->GetNextDataDir(non_existent_opts, nullptr); Test the Status programmatically before testing its string representation. Elsewhere too. PS17, Line 115: FLAGS_fs_data_dirs_reserved_bytes = 1; : FLAGS_disk_reserved_bytes_free_for_testing = 2; Why is it necessary to set these? Won't GetNextDataDir() return Status::OK() even if they're unset? Line 138: FLAGS_fs_target_data_dirs_per_tablet = group_size_; Maybe do this once in the test constructor, and do it again in each test that needs to override group_size_? http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: PS17, Line 254: // Seed the RNG for data directory group operations. : std::srand(std::time(0)); I don't think it makes sense to modify process-level state deep in an object's constructor like this. Why did you need this in the first place? When we need to seed a PRNG in unit tests, we use SeedRandom(). Line 394: InsertOrDie(&tablets_by_uuid_idx_map_, idx, std::set<std::string>()); Don't need std:: prefixes here. Line 479: DataDir* candidate = data_dir_by_uuid_idx_[uuid_idx]; FindOrDie() here? PS17, Line 518: no_dirs_found Instead of the separate bool, could you wrap new_uuid_idx in a boost::optional? It'll be initialized to boost::none by default I think, and then the caller can check if it's still boost::none after the call or whether it's been initialized to a real value. Line 548: RETURN_NOT_OK(candidate->RefreshIsFull(DataDir::RefreshMode::ALWAYS)); Why do we need to refresh? http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: Line 51: // A DataDirGroup is a group of directories used by an entity for block I don't understand why you had to move DataDirGroup back to the header. The only usage I see is as a value in TabletDataDirGroupMap; can't that be forward declared? -- 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: 17 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
