Todd Lipcon has posted comments on this change. Change subject: KUDU-1952 Remove round-robin for block placement ......................................................................
Patch Set 17: (18 comments) http://gerrit.cloudera.org:8080/#/c/6636/17//COMMIT_MSG Commit Message: PS17, Line 9: mitigate the : single-disk failure nit: we aren't mitigating the failure, but rather mitigating the effects of a single-disk failure PS17, Line 27: When loading : tablet data from a previous version of Kudu, the tablet's superblock : will not have a DataDirGroup. One will be generated containing all : data directories, as the tablet's data may already be spread across : any number of disks. what happens if we add a table on the new version, then downgrade, run for a bit, and then upgrade? the new version will still think that the tablet has blocks only on a subset of disks, even though in fact it has blocks on more, right? any way to avoid this issue? http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/block_manager-test.cc File src/kudu/fs/block_manager-test.cc: Line 133: int CountFiles(const string& path) { would using Env::Walk() make this easier? http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: PS17, Line 62: evolving maybe even experimental, considering this isn't quite usable yet? Line 188: FLAGS_fs_data_dirs_full_disk_cache_seconds = 0; // Don't cache device fullness. ?? PS17, Line 410: Tablet already being tracked i think this and the one below could be a bit more specific and refer to datadir groups. Remember that Statuses don't keep track of the line of code or file where they were produced, so being pretty specific is helpful. PS17, Line 422: group_target_size = FLAGS_fs_target_data_dirs_per_tablet; : if (group_target_size > data_dirs_.size()) { : group_target_size = data_dirs_.size(); : } use std::min? Line 460: // This should only be reached by some tests; in cases where there is no maybe DCHECK(IsGTest()) from test_util_prod.h? Line 535: } why not also just skip adding the full ones here? might make the lower part easier http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: PS17, Line 53: PathSetPB maybe missed something somewhere in the design, but when we get to removing and replacing disks, what happens to the PathSetPB? We'll have to keep all of the dead UUIDs in there forever, right? PS17, Line 66: uuid_indices std::move Line 70: DCHECK(pb != nullptr); can just DCHECK(pb); Line 73: *group.mutable_uuid_indices()->Add() = uuid_idx; I think you can write this as group->add_uuid_indices(uuid_idx) Line 75: *pb = group; nit: pb->Swap(group); to avoid an extra allocation PS17, Line 251: no_dirs_found' is set to true if the group is already : // larger than the limit or if all candidates are full why not return a bad status instead of a second out-param? http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/file_block_manager.h File src/kudu/fs/file_block_manager.h: PS17, Line 67: CreateBlockOptions typo? http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/fs/fs.proto File src/kudu/fs/fs.proto: Line 116: message DataDirGroupPB { can you note where this ends up stored? http://gerrit.cloudera.org:8080/#/c/6636/17/src/kudu/tablet/delta_compaction.cc File src/kudu/tablet/delta_compaction.cc: Line 65: const string& tablet_id) nit: pass by value and then std::move below -- 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: 17 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
