Adar Dembo has posted comments on this change. Change subject: KUDU-1952 Remove round-robin for block placement ......................................................................
Patch Set 9: (37 comments) I reviewed everything but the data_dir changes and the new test. Mostly style stuff. http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 28: #include "kudu/fs/data_dirs.h" Don't need; just forward declare DataDirManager. PS9, Line 160: For now, No need for "For now" type language; it doesn't really add content and it's just one more thing that will need to be updated (or go stale, if no one notices) when the struct grows a new field. The "In the future" stuff is OK though, since that adds useful information. PS9, Line 253: // Exposes an interface for the DataDirManager, granting other entities, like : // the FsManager, the ability to manage data dir groups, which is a function : // of each tablet. I don't think a simple accessor like this needs a broad justification. Something as simple as "Exposes the underlying DataDirManager" can suffice. Line 256: virtual DataDirManager* dd_manager() = 0; Can be a const method, right? http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/fs/data_dir_group.h File src/kudu/fs/data_dir_group.h: I think the content here can be merged into data_dirs.{h,cc}, those two aren't massive yet. Line 29: class DataDirGroup { Merits some documentation. Line 30: public: Nit: indent by one char (private too). PS9, Line 32: std::vector<uint16_t>() Can use {} instead? Line 44: CHECK(pb != nullptr); Seems more appropriate as a DCHECK, unless you think there's a lack of test coverage for this path? http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: PS9, Line 171: CreateBlockOptions I don't think this is appropriate here. We're not creating a block, so the use of CreateBlockOptions confuses more than helps. If you need a tablet_id, pass a tablet_id. PS9, Line 183: CreateBlockOptions Also inappropriate here (though admittedly the case is weaker; we generally are trying to create a block when we call this function). http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/fs/file_block_manager.h File src/kudu/fs/file_block_manager.h: Line 65: // When creating blocks, the block manager will select a random directory in Maybe reword this to be more vague, so that if the directory selection policy changes further in the future, this comment won't need to be updated? http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/fs/fs.proto File src/kudu/fs/fs.proto: Line 113: message DataDirGroupPB { Should document the new message itself too. Line 116: repeated uint32 uuids = 1; Do you anticipate wanting more per-data dir state in the future? If so, it might be better future proofing to define a DataDirPB message right now (with just the uuid as a field in it). http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: PS9, Line 587: CreateBlockOptions({""}) Could just be CreateBlockOptions(). http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: Line 135: std::unique_ptr<fs::WritableBlock>* block); Nit: indentation. Line 139: // This should only be used in tests. Do we actually need to preserve this code path? How painful would it be to get rid of it? 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 Is there any other kind of block? Doesn't every block belong to a diskrowset in some form or another? Line 215: // Returns a randomly-selected container within the data directory group Like in file block manager, perhaps it'd be better to be vague here so that the comment doesn't become stale were the selection policy to change again. http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/tablet/delta_compaction.h File src/kudu/tablet/delta_compaction.h: Line 27: #include "kudu/fs/block_manager.h" No longer needed? Line 63: Status Compact(const std::string& tablet_id); See my earlier feedback: was hoping we could pass (and store_ the tablet_id in the constructor and avoid plumbing here (or in the private methods). http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/tablet/delta_tracker.cc File src/kudu/tablet/delta_tracker.cc: Line 43: using fs::CreateBlockOptions; Should come before ReadableBlock. http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/tablet/diskrowset.cc File src/kudu/tablet/diskrowset.cc: Line 67: using fs::CreateBlockOptions; Before Scoped... http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/tablet/metadata.proto File src/kudu/tablet/metadata.proto: PS9, Line 126: The : // group is represented by the UUIDs of the data directories it consists of. This part belongs to the definition of DataDirGroupPB, not here. But you should explain what the behavior is when data_dir_group isn't set. http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/tablet/multi_column_writer.cc File src/kudu/tablet/multi_column_writer.cc: Line 50: CreateBlockOptions block_opts({tablet_id}); Nit: { tablet_id } (so that the braces are more noticeable. Applies to other files too). http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/tablet/multi_column_writer.h File src/kudu/tablet/multi_column_writer.h: PS9, Line 53: Input options indicate to which blocks : // the columns should be written. Comment no longer applies. Line 55: Status Open(const std::string& tablet_id); The tablet_id doesn't change with each Open(); can you pass (and store) it in the constructor? http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/tablet/tablet_bootstrap-test.cc File src/kudu/tablet/tablet_bootstrap-test.cc: PS9, Line 255: // Untrack the tablet in the data dir manager so upon next bootstrap, the : // tablet can be metadata's data dir group can be read. Nit: second half of the sentence doesn't read quite right. http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: Line 198: RETURN_NOT_OK(fs_manager_->dd_manager()->CreateDataDirGroup(CreateBlockOptions({tablet_id_}))); Why do we need to do this? I'll take my answer in the form of a comment. Line 368: Nit: we try to use a "soft" limit of 80 chars and a "hard" limit of 100 chars per line (see http://kudu.apache.org/docs/contributing.html#_line_length for details). Since the rest of this function appears to line up more at 80 than 100, could you wrap the new code more aggressively? This feedback also applies to changes in other files. http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/tablet/tablet_metadata.h File src/kudu/tablet/tablet_metadata.h: Line 24: #include <set> The new inclusions are no longer needed? Line 69: // This also creates a data dir group for the tablet. Nit: I think this is implied by "Create metadata for a new tablet". If we documented every piece of new metadata, the comment would get far too long. http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: Line 78: using fs::CreateBlockOptions; Sorting. http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/tserver/tablet_copy_client.h File src/kudu/tserver/tablet_copy_client.h: Line 97: // Upon success, tablet metadata will be created and the tablet will be Nit: separate from the above section with an empty line: // ... // ... // // Upon success, ... http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: Line 35: #include "kudu/fs/block_manager.h" New inclusions no longer needed? Line 487: // Nit: unless the "Note" is logically part of the TODO, could you reorder so the Note comes first? 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, I think we use delete_type pretty consistently elsewhere, so can we keep it named that way? -- 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: 9 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
