Adar Dembo has posted comments on this change. Change subject: KUDU-1952 Remove round-robin for block placement ......................................................................
Patch Set 12: (26 comments) Did another pass, still not done though. http://gerrit.cloudera.org:8080/#/c/6636/12/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: Line 406: tablet_id, Nit: indentation. Line 416: if (ContainsKey(group_by_tablet_map_, tablet_id)) { Between this, emplace(), and FindOrNull, I count three accesses to group_by_tablet_map_. The functions in map-util.h ought to be able to reduce that at least to two, possibly to one. PS12, Line 421: kDataDirGroupSize Nit: reserve kCamelCase notation for variables whose values never change; it's a little strange to see it modified on L423. PS12, Line 428: DCHECK(group_for_tablet); Shouldn't this be combined with L427 in FindOrDie? PS12, Line 432: uuid_and_dir_ptr How about just 'e' for entry? And can't this be a const auto& iteration? Line 433: uint16_t uuid = uuid_and_dir_ptr.first; Not an actual uuid. Line 449: UntrackTablet(tablet_id); Won't this deadlock since it'll try to acquire dir_group_lock_ recursively? Do we have test coverage for this path? Alternatively, consider restructuring this function so that modifications to shared state are deferred until after we know the function cannot fail. Then no cleanup is ever necessary, and there's an added bonus that it's much easier to keep track of the shared state changes because it's happening in one place instead of sprinkled throughout the method. PS12, Line 460: opts.tablet_id == "" Use empty() on the string instead. Line 513: bool DataDirManager::GetDirForGroup(vector<uint16_t>* group, uint16_t* uuid) { If the expectation is that dir_group_lock_ is held while calling this method, add an "Unlocked" suffix to its name, doc the synchronization requirement in the header, and DCHECK() that the lock is held when you enter the method. Line 557: tablets_by_uuid_map_[uuid1].size() > tablets_by_uuid_map_[uuid2].size()) { Nit: indentation. http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: PS9, Line 171: ricEntity> metric_ > This was intended on being here to support disk classes. Suppose it's not n Right, and these are all internal APIs, so we can add/remove as needed in the future. http://gerrit.cloudera.org:8080/#/c/6636/12/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: Line 54: // A DataDirGroup is a group of directories used by an entity for block Since this isn't used outside of data_dirs.cc, perhaps we can declare it there? Line 81: std::vector<uint16_t>* uuids() { return &uuids_; } Would it be possible to enforce that DataDirGroups are immutable once created? That would mean: 1. Returning uuids() as a cref here. 2. Providing all of the UUID indexes up front, in the constructor. Line 86: // UUIDs corresponding to the data directories within the group. It occurred to me that these aren't actually UUIDs; they're indexes into the UUID map owned by the DataDirManager. We should probably name them accordingly (certainly across DataDirGroup and maybe in other places where "UUID" is referenced); as is, it's confusing because there's no way a UUID can be specified in only two bytes. PS12, Line 209: // Stops tracking the specified tablet. Maps from tablet to group and from : // data dir to tablet set are cleared of all references to the tablet. Pointers : // to a deleted DataDirGroup are no longer valid and should not be dereferenced. : void UntrackTablet(const std::string& tablet_id); Comment needs to be updated now that data dir groups are private to the DataDirManager. Maybe also rename the method, to DeleteDataDirGroup? Line 215: DataDirGroup* GetDataDirGroupForTablet(const std::string& tablet_id); This appears to always be followed up with a ToPB conversion. Perhaps it would be simpler to do a PB conversion internally and only return the PB? Then you're not handing out pointers to internally owned state. Line 249: bool GetDirForGroup(std::vector<uint16_t>* group, uint16_t* uuid); I think it would be cleaner if 'group' was an IN parameter rather than IN/OUT. Let the caller decide whether to add uuid to group once the function returns true. PS12, Line 272: percpu_rwlock How did you arrive at this choice of lock? http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/fs/fs.proto File src/kudu/fs/fs.proto: Line 116: // List of data directory's UUIDs. Must not be empty. > Per-data dir state will be useful in specifying that certain data dirs are Sounds good. 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 and all_uuids in PathSetPB. http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: PS9, Line 105: blocks for diskrowsets > Right, should be i.e. instead of e.g., or removed altogether. Even with 'i.e.' it still reads kind of funny, because every Kudu block (be it a bloom file, a delta file, or a cfile) belongs to a diskrowset. So using "blocks for diskrowsets" as further explanation is somewhat imprecise, because _all_ blocks are "for diskrowsets"; there's no other kind of block. Put another way, when I read "(i.e. blocks for diskrowsets should be ...)", I think "okay, so blocks that belong in diskrowsets should have one kind of behavior applied to them, and blocks for other stuff shouldn't. Wait, but there are no other kind of blocks, so what distinction is this trying to draw? What did Andrew mean when he wrote this that I'm not getting?" http://gerrit.cloudera.org:8080/#/c/6636/12/src/kudu/tablet/delta_compaction.h File src/kudu/tablet/delta_compaction.h: Line 119: // The tablet containing this tablet. How about, "The ID of the tablet being compacted"? http://gerrit.cloudera.org:8080/#/c/6636/12/src/kudu/tablet/metadata.proto File src/kudu/tablet/metadata.proto: PS12, Line 128: disks data directories http://gerrit.cloudera.org:8080/#/c/6636/12/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: Line 96: RETURN_NOT_OK(ret->Flush()); If the Flush() fails, should we delete the created DataDirGroup? Line 196: fs_manager_->dd_manager()->UntrackTablet(tablet_id_); What state are we left in if this is called but Flush() below fails? 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_state, > Done You changed the variable reference in the comment, but not its actual name. -- 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: 12 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
