Adar Dembo has posted comments on this change. Change subject: KUDU-1952 Improve block placement ......................................................................
Patch Set 1: (5 comments) I didn't do a full review, but left you a couple comments to get started. http://gerrit.cloudera.org:8080/#/c/6636/1/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: 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 with maps. For example, one of the LookupOrInsert() variants might be more appropriate here. http://gerrit.cloudera.org:8080/#/c/6636/1/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: Line 47: typedef std::vector<uint16_t> DataDirGroup; I think it makes sense to define this as a class with a corresponding PB class (in fs.proto). Then, you can encapsulate ToPB() and FromPB() methods here. After that, maybe you can rework the DataDirGroup functions below such that it's clear which ones take DataDirGroupPB (because they're loading it from serialized state) and which ones take DataDirGroup? 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 ID when we construct the MajorDeltaCompaction and avoid this plumbing? 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 plumbing the tablet's ID down into the WritableBlock. Instead, let's call a spade a spade and just pass the tablet ID explicitly instead. Reserve CreateBlockOptions for interacting with the FsManager (or block manager). http://gerrit.cloudera.org:8080/#/c/6636/1/src/kudu/tablet/tablet_metadata.h File src/kudu/tablet/tablet_metadata.h: Line 335: fs::DataDirGroup* data_dir_group_; It would simplify a lot if we could avoid this link between the tablet metadata and the group. Can we fetch it as needed from the dd_manager? Even better, when we need to serialize it, can we offer a mutable pointer of the superblock's DataDirGroupPB to the dd_manager, and have it deal with serialization and writing? -- 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: 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
