Andrew Wong has posted comments on this change. Change subject: KUDU-1952 Remove round-robin for block placement ......................................................................
Patch Set 14: (27 comments) I moved the definition of DataDirGroup back to the .h since it's used by one of the private members. Also added data_dirs-test.cc http://gerrit.cloudera.org:8080/#/c/6636/14/src/kudu/fs/block_manager-stress-test.cc File src/kudu/fs/block_manager-stress-test.cc: PS14, Line 433: "test_tablet" > Define this once in the test fixture and use it in both places. Done http://gerrit.cloudera.org:8080/#/c/6636/14/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: Line 118: CHECK_OK(bm_->dd_manager()->LoadDataDirGroupFromPB("test_tablet", test_group_pb_)); > Why not RETURN_NOT_OK here too? Done Line 126: void RunBlockDistributionTest(const vector<string>& paths); > Maybe add some coverage for DeleteDataDirGroup too? Done Line 141: ASSERT_STR_CONTAINS(s.ToString(), "DataDirGroup not found for tablet"); > Create a separate test for this; don't overload setUp() with tests. Done Line 155: int count_files(Env* env, const string& path, const string& instance_name) { > Should be CountFiles(). Also, perhaps make it a member of BlockManagerTest Done PS14, Line 171: instance_name > Isn't this always "block_manager_instance"? If so, can you hardcode it? Done PS14, Line 190: CHECK_OK > ASSERT_OK Done PS14, Line 228: CHECK_OK > ASSERT_OK Done Line 284: CHECK_OK(bm_->dd_manager()->CreateDataDirGroup("multipath_test")); > ASSERT_OK Done PS14, Line 313: CHECK_OK > ASSERT_OK Done PS14, Line 385: CreateBlockOptions({ "test_tablet" }) > Maybe you can define this once as a BlockManagerTest member and refer to it Done http://gerrit.cloudera.org:8080/#/c/6636/14/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: PS14, Line 160: A group is represented by a list of indices within the list of : // all UUIDs found in the PathSetPB. > Maybe "A group is represented by a list of 2-byte indices, which index into Done Line 478: tablets_by_uuid_idx_map_[uuid_idx].insert(tablet_id); > Should this use InsertOrDie(), to emphasize that there's no reason the set Done Line 528: for (int16_t uuid_index : group->uuid_index_list()) { > Should be uint16_t Done Line 529: tablets_by_uuid_idx_map_[uuid_index].erase(tablet_id); > Should we use FindOrDie() to get the set of tablets out, since it'd be an e Done Line 573: candidate = data_dir_by_uuid_idx_[dir_uuids[iter1]]; > FindOrDie(). L580 too. Done Line 590: tablets_by_uuid_idx_map_[uuid1].size() > tablets_by_uuid_idx_map_[uuid2].size()) { > Use FindOrDie() here too. Done http://gerrit.cloudera.org:8080/#/c/6636/14/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: Line 27: #include "kudu/fs/fs.pb.h" > Do you still need this? Maybe you can get away with forward-declaring DataD I can and Done PS14, Line 162: Adds data directories to a specific tablet's dir group, until the limit : // specified by fs_target_data_dirs_per_tablet, or until there is no more space. > Yes, but we should also emphasize that this also creates a dir group for th Done PS14, Line 170: tablet > Nit: tablet's dir group. Done PS14, Line 171: tablet will be untracked. > Nit: "tablet's dir group will be deleted" (to emphasize the symmetry with D Done PS14, Line 174: tracking > Nit: since this method is no longer UntrackTablet, I think it'd be clearer Done Line 221: bool GetDirForGroupUnlocked(const std::vector<uint16_t>& group, uint16_t* uuid_idx); > Maybe the parameters would be more clear as 'uuid_idxes' and 'new_uuid_idx' Hrm, I changed it to 'group_indices' and 'new_uuid_index', hopefully that's clearer. http://gerrit.cloudera.org:8080/#/c/6636/14/src/kudu/fs/fs.proto File src/kudu/fs/fs.proto: PS14, Line 117: indexes > Nit: indices or indexes? Pick one and apply it consistently here (and where Done. http://gerrit.cloudera.org:8080/#/c/6636/14/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: PS14, Line 67: "test_tablet" > Define just once. Done PS14, Line 247: CreateBlockOptions({ "test_tablet" } > Maybe define just once and use repeatedly? Done http://gerrit.cloudera.org:8080/#/c/6636/14/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: Line 96: fs_manager->dd_manager()->DeleteDataDirGroup(tablet_id); > So a convention we like to use in these situations is to wrap the cleanup t That is _neat_ Done -- 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: 14 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
