Andrew Wong has posted comments on this change. Change subject: KUDU-1952 Improve block placement ......................................................................
Patch Set 1: (17 comments) http://gerrit.cloudera.org:8080/#/c/6636/1//COMMIT_MSG Commit Message: Line 10: single-disk failure. Throughout the code, the term "DataDir" refers to > would be good to link to the design doc somewhere here Done PS1, Line 14: This patch adds a mapping from tablet to a set of disks and uses it to : replace the existing round-robin placement of blocks. Tablets are : m > it would be good in the commit message to explain the impact of this patch. Good points about both regression and upgrade impact. I'll set a default to use all disks to avoid the performance regression. As for upgrades/backwards compatibility, this is something that I haven't covered and should. If no disk groups are defined for a tablet, I think we can assume (if they're not tombstoned) that the tablet data was written with a previous version of kudu, in which case just add all directories to the tablet's group on boostrap. http://gerrit.cloudera.org:8080/#/c/6636/1/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: Line 57: "Indicates the target number of data dirs to spread each " > what's the behavior here if the configuration is larger than the number of If this is larger than the number of data dirs, the data group size will be set to the max number of directories currently available when creating a new group. Fair point. Will set a default for now indicating the full range of data dirs should be used for each tablet, which does at least address KUDU-1952, although it does still have added memory/storage usage. Will add a note about this. Line 402: if (group_by_tablet_map_.find(tablet_id) == group_by_tablet_map_.end()) { > Take a look at map-util.h; it's got a lot of idiomatic methods for dealing Done. Used InsertOrReturnExisting; I don't think you can verify whether LookupOrInsert actually inserts. Line 403: group_by_tablet_map_[tablet_id] = std::move(group_to_add); > warning: std::move of the const variable has no effect; remove std::move() Done Line 428: if (group_for_tablet->size() == 0) { > warning: the 'empty' method should be used to check for emptiness instead o Done Line 524: } else if (iter2 == dir_uuids.size() || > warning: don't use else after return [readability-else-after-return] Done http://gerrit.cloudera.org:8080/#/c/6636/1/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: Line 202: bool GetDirForGroup(DataDirGroup& group, uint16_t* uuid); > warning: non-const reference parameter 'group', make it const or use a poin Done http://gerrit.cloudera.org:8080/#/c/6636/1/src/kudu/tablet/delta_compaction.cc File src/kudu/tablet/delta_compaction.cc: Line 43: using cfile::CFileIterator; > warning: using decl 'CFileIterator' is unused [misc-unused-using-decls] Done Line 44: using cfile::CFileReader; > warning: using decl 'CFileReader' is unused [misc-unused-using-decls] Done Line 45: using cfile::IndexTreeIterator; > warning: using decl 'IndexTreeIterator' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/6636/1/src/kudu/tablet/delta_compaction.h File src/kudu/tablet/delta_compaction.h: Line 67: Status Compact(const fs::CreateBlockOptions& opts); > A compaction is only ever for a single tablet; can we just get the tablet I Done http://gerrit.cloudera.org:8080/#/c/6636/1/src/kudu/tablet/multi_column_writer.h File src/kudu/tablet/multi_column_writer.h: Line 40: struct CreateBlockOptions; > In general it feels weird to use CreateBlockOptions as the vehicle for plum Done http://gerrit.cloudera.org:8080/#/c/6636/1/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: Line 60: using kudu::consensus::RaftConfigPB; > warning: using decl 'RaftConfigPB' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/6636/1/src/kudu/tablet/tablet_metadata.h File src/kudu/tablet/tablet_metadata.h: Line 270: TabletMetadata(TabletSuperBlockPB superblock); > warning: single-argument constructors must be marked explicit to avoid unin Unused. Will remove. Line 335: fs::DataDirGroup* data_dir_group_; > It would simplify a lot if we could avoid this link between the tablet meta Done. Now just calls dd_manager()->GetDirGroupForTablet() http://gerrit.cloudera.org:8080/#/c/6636/1/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: Line 81: using consensus::RaftPeerPB; > warning: using decl 'RaftPeerPB' is unused [misc-unused-using-decls] 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: 1 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
