Andrew Wong has posted comments on this change. Change subject: KUDU-1952 Remove round-robin for block placement ......................................................................
Patch Set 14: (24 comments) http://gerrit.cloudera.org:8080/#/c/6636/12/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: Line 406: JoinStrings(paths_, ","))); > Nit: indentation. Done Line 416: idx = i; > Between this, emplace(), and FindOrNull, I count three accesses to group_by An initial ContainsKey is warranted as to avoid the work below, reduced to two. PS12, Line 421: (idx > max_data_ > Nit: reserve kCamelCase notation for variables whose values never change; i Done PS12, Line 428: > Shouldn't this be combined with L427 in FindOrDie? Restructured based on keeping DataDirGroups immutable, only a single InsertOrDie is needed (after the initial ContainsKey). PS12, Line 432: s::OK(); > How about just 'e' for entry? And can't this be a const auto& iteration? Done Line 433: } > Not an actual uuid. Done Line 449: if (ContainsKey(group_by_tablet_map_, tablet_id)) { > Won't this deadlock since it'll try to acquire dir_group_lock_ recursively? Restructured and added test coverage PS12, Line 460: ize == 0 || use_all_ > Use empty() on the string instead. Done Line 513: } > If the expectation is that dir_group_lock_ is held while calling this metho Done Line 557: for (auto uuid_and_dir_ptr : data_dir_by_uuid_idx_) { > Nit: indentation. Done http://gerrit.cloudera.org:8080/#/c/6636/12/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: Line 54: > Since this isn't used outside of data_dirs.cc, perhaps we can declare it th Done Line 81: // If 'mode' is EXPIRED_ONLY, performs the test only if the dir was last > Would it be possible to enforce that DataDirGroups are immutable once creat Done Line 86: // reflected via is_full(). > It occurred to me that these aren't actually UUIDs; they're indexes into th Done PS12, Line 209: // 'uuid_idx' is an output denoting which uuid_idx should be added to : // 'group'. Returns false if the group is already larger than the limit or if : // all candidates are full. : // > Comment needs to be updated now that data dir groups are private to the Dat Done Line 215: // 'uuid_idx' to 'group') warrants that these calls fall within the scope of > This appears to always be followed up with a ToPB conversion. Perhaps it wo Done Line 249: > I think it would be cleaner if 'group' was an IN parameter rather than IN/O Done http://gerrit.cloudera.org:8080/#/c/6636/12/src/kudu/fs/fs.proto File src/kudu/fs/fs.proto: Line 113: // Tablet data are spread across a specified number of data directories. The > These are UUID indexes, right? Not UUIDs? Could draw the connection to uuid Done, good pointer to PathSetPB. http://gerrit.cloudera.org:8080/#/c/6636/12/src/kudu/tablet/delta_compaction.h File src/kudu/tablet/delta_compaction.h: Line 119: // The ID of the tablet being compacted. > How about, "The ID of the tablet being compacted"? Done http://gerrit.cloudera.org:8080/#/c/6636/12/src/kudu/tablet/metadata.proto File src/kudu/tablet/metadata.proto: PS12, Line 128: a new > data directories Done http://gerrit.cloudera.org:8080/#/c/6636/12/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: Line 60: > warning: using decl 'CreateBlockOptions' is unused [misc-unused-using-decls Done Line 96: fs_manager->dd_manager()->DeleteDataDirGroup(tablet_id); > If the Flush() fails, should we delete the created DataDirGroup? Yep, without a functioning TabletMetadata, tablet creation won't proceed. Line 196: // Keep a copy of the old data dir group in case of flush failure. > What state are we left in if this is called but Flush() below fails? Good catch, if we fail, we have to roll-back the DataDirManager state. http://gerrit.cloudera.org:8080/#/c/6636/12/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: Line 481: // TODO(unknown): Because we begin shutdown of the tablet after we check our > warning: missing username/bug in TODO [google-readability-todo] Done http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/tserver/ts_tablet_manager.h File src/kudu/tserver/ts_tablet_manager.h: Line 188: tablet::TabletDataState delete_type, > You changed the variable reference in the comment, but not its actual name. 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
