YangSong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14920 )
Change subject: KUDU-2975: Spread WAL across multiple directories ...................................................................... Patch Set 6: (27 comments) http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/fs_manager.h@223 PS5, Line 223: Status GetWalDirFromDisk(const std::string& tablet_id, std::string* wal_dir); : : Status GetWalRecoveryDirFromDisk(const std::string& tablet_id, : std::string* wal_recovery_dir); > Can you add comments explaining what "orphaned" means here? Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/fs_manager.cc@a704 PS5, Line 704: : : > nit: maybe use the emplace_back() pattern here too? Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/fs_manager.cc@153 PS5, Line 153: nager::kInstan > nit: add "using strings::Split()" Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/fs_manager.cc@149 PS5, Line 149: const char *FsManager::kWalFileNamePrefix = "wal"; : const char *FsManager::kWalsRecoveryDirSuffix = ".recovery"; : const char *FsManager::kTabletMetadataDirName = "tablet-meta"; : const char *FsManager::kDataDirName = "data"; : const char *FsManager::kInstanceMetadataFileName = "instance"; : con > How about adding a flag validator that validates that at most one of {fs_wa Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/fs_manager.cc@195 PS5, Line 195: anager_->SetE > nit: "wal roots" Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/fs_manager.cc@271 PS5, Line 271: : for (const string& data_ > nit: If you use Substitute() for this, you could easily do something like Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/fs_manager.cc@282 PS5, Line 282: (INFO) << "Data d > nit: "use the first WAL root." Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/fs_manager.cc@305 PS5, Line 305: at > nit: roots Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/fs_manager.cc@441 PS5, Line 441: block_manager_type > This should be "wal" or something similar, right? use "wals". http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/fs_manager.cc@530 PS5, Line 530: ilesystem > nit: prefer using emplace_back in new code Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/fs_manager.cc@556 PS5, Line 556: DataDirManagerOptions dm_opts; > nit: how about "creating WAL directory manager" and "creating data director Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/fs_manager.cc@730 PS5, Line 730: continue; : } : > We should probably consider this a programmer error, in which case a DCHECK Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/fs_manager.cc@735 PS5, Line 735: : > nit: use Substitute() here for improved readability. Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/fs_manager.cc@781 PS5, Line 781: } > We should use CHECK or DCHECK on this too so we don't have to condition on Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/wal_dirs.h File src/kudu/fs/wal_dirs.h: http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/wal_dirs.h@114 PS5, Line 114: has already > nit: "has already been registered" Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/wal_dirs.h@117 PS5, Line 117: > Document what 'suffix' does? Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/wal_dirs.h@120 PS5, Line 120: ing* wal_dir); > nit: "has no WAL dir" Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/wal_dirs.h@121 PS5, Line 121: : // Results in an error if the table > nit: fix spacing Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/wal_dirs.h@123 PS5, Line 123: rning with an error, > nit: could you rename this "FindAndRegisterWalDirOnDisk()" or something sim Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/wal_dirs.h@135 PS5, Line 135: // Finds a WAL directory by uuid, returning null if it can't be found. : WalDir* FindWalDirByUuid(const std::string& uuid) const; : > nit: this isn't quite true, so maybe remove it? Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/wal_dirs.cc File src/kudu/fs/wal_dirs.cc: http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/wal_dirs.cc@113 PS5, Line 113: es() const { > Shouldn't this be kWalDirName? Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/wal_dirs.cc@128 PS5, Line 128: l > nit: could you annotate this with the variable name so it's easy to underst Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/wal_dirs.cc@297 PS5, Line 297: int uuid_idx = tablets_by_uuid.first; : if (ContainsKey(failed_dirs_, uuid_idx)) { : continue; : } : a > How can we ever reach this block of code? Would a CHECK or DCHECK be more a Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/fs/wal_dirs.cc@339 PS5, Line 339: > nit: you can write this as Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/tablet/tablet_metadata.cc@234 PS5, Line 234: data dir group > nit: "data dir group and WAL dir" Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/tablet/tablet_metadata.cc@474 PS5, Line 474: Substitute(" > tiny-nit: this could probably be on the previous line Done http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: http://gerrit.cloudera.org:8080/#/c/14920/5/src/kudu/tserver/tablet_copy_client.cc@348 PS5, Line 348: for t > nit: it's not much of a group if there's only one directory! Done -- To view, visit http://gerrit.cloudera.org:8080/14920 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic3490b2bbc0544e7c8a5f987fb83855ff155ac2c Gerrit-Change-Number: 14920 Gerrit-PatchSet: 6 Gerrit-Owner: YangSong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: YangSong <[email protected]> Gerrit-Comment-Date: Thu, 09 Jan 2020 08:52:20 +0000 Gerrit-HasComments: Yes
