Andrew Wong has posted comments on this change. Change subject: KUDU-1952 Remove round-robin for block placement ......................................................................
Patch Set 10: (37 comments) 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/gutil/ref_counted.h" > Don't need; just forward declare DataDirManager. Done PS9, Line 160: This is > No need for "For now" type language; it doesn't really add content and it's Done PS9, Line 253: virtual DataDirManager* dd_manager() = 0; : }; : > I don't think a simple accessor like this needs a broad justification. Some Done Line 256: // Closes a group of blocks. > Can be a const method, right? The DataDirManager is owned by the block managers, so const wouldn't work here unless we returned a const pointer. 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 are Done. Moved to data_dirs.h Line 29 > Merits some documentation. Done Line 30 > Nit: indent by one char (private too). Done PS9, Line 32: > Can use {} instead? Done Line 44 > Seems more appropriate as a DCHECK, unless you think there's a lack of test Done 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_ > I don't think this is appropriate here. We're not creating a block, so the This was intended on being here to support disk classes. Suppose it's not needed, depending on if/how disk class gets implemented. PS9, Line 183: > Also inappropriate here (though admittedly the case is weaker; we generally Here as well, could be useful in the future were we to want to get the next data dir for a bloomfile for a given tablet, but you're right that it's unnecessary at this point. I think here more than CreateDataDirGroup, it'd be important to keep this here for the future, since it's only being used for block placement. 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 place blocks based on the > Maybe reword this to be more vague, so that if the directory selection poli Done http://gerrit.cloudera.org:8080/#/c/6636/9/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 > Should document the new message itself too. Done Line 116: // List of data directory's UUIDs. Must not be empty. > Do you anticipate wanting more per-data dir state in the future? If so, it Per-data dir state will be useful in specifying that certain data dirs are of a certain disk class, for instance, but I don't think _that_ metadata needs to be stored in the data dir group, since there are more instances of this uuid than there are data dirs. I think it'd be sufficient to separate a DataDirPB and DataDirGroupPB, with just the uuids acting as a sort of foreign key between the two, when we do add that extra state. http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: PS9, Line 587: > Could just be CreateBlockOptions(). As per the other comment, I'm taking out this codepath. 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. Done Line 139: > Do we actually need to preserve this code path? How painful would it be to Ah, I'd originally preserved this because default behavior was a bit different. Each test _needed_ something specified by the opts, so each test had to manually do a CreateDataDirGroup before calling CreateNewBlock. Now it should be fine to just use the default constructor. 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 diskrowse Right, should be i.e. instead of e.g., or removed altogether. Line 215: // Returns a container appropriate for the given CreateBlockOptions, creating > Like in file block manager, perhaps it'd be better to be vague here so that Done 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/tablet/compaction.h" > No longer needed? Done Line 63: Status Compact(); > See my earlier feedback: was hoping we could pass (and store_ the tablet_id Done 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::WritableBlock; > Should come before ReadableBlock. Done http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/tablet/diskrowset.cc File src/kudu/tablet/diskrowset.cc: Line 67: using fs::ScopedWritableBlockCloser; > Before Scoped... Done http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/tablet/metadata.proto File src/kudu/tablet/metadata.proto: PS9, Line 126: set, : // it is assumed that the data is from a previous version of Kudu. In this > This part belongs to the definition of DataDirGroupPB, not here. Done 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: > Nit: { tablet_id } Done 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: : // Open and start writing the col > Comment no longer applies. Done Line 55: Status Open(); > The tablet_id doesn't change with each Open(); can you pass (and store) it Done 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 the next call to : // BootstrapTestTablet, the tablet metadata's data dir > Nit: second half of the sentence doesn't read quite right. Done http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: Line 198: // If the tablet deletion is a part of a tablet copy, a data dir group must > Why do we need to do this? I'll take my answer in the form of a comment. Done Line 368: } > Nit: we try to use a "soft" limit of 80 chars and a "hard" limit of 100 cha Done http://gerrit.cloudera.org:8080/#/c/6636/9/src/kudu/tablet/tablet_metadata.h File src/kudu/tablet/tablet_metadata.h: Line 24: #include <vector> > The new inclusions are no longer needed? Done Line 69: const Schema& schema, > Nit: I think this is implied by "Create metadata for a new tablet". If we d Done 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::WritableBlock; > Sorting. Done 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: // > Nit: separate from the above section with an empty line: Done 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/fs_manager.h" > New inclusions no longer needed? Done Line 487: // check again after calling Shutdown(), and if the check fails, try to > Nit: unless the "Note" is logically part of the TODO, could you reorder so 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_state, > I think we use delete_type pretty consistently elsewhere, so can we keep it 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: 10 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
