Andrew Wong has posted comments on this change. Change subject: open FS layout in presence of disk failure ......................................................................
Patch Set 4: (16 comments) http://gerrit.cloudera.org:8080/#/c/7784/3//COMMIT_MSG Commit Message: PS3, Line 20: mangers' > looks like a typo It is, indeed, "managers'," referring to the startups of the directory manager, log block manager, and fs manager. http://gerrit.cloudera.org:8080/#/c/7784/4/src/kudu/fs/block_manager_util.cc File src/kudu/fs/block_manager_util.cc: PS4, Line 189: it's > nit its Done PS4, Line 189: them > nit s/them/it Done http://gerrit.cloudera.org:8080/#/c/7784/4/src/kudu/fs/block_manager_util.h File src/kudu/fs/block_manager_util.h: PS4, Line 56: // On success, either 'metadata_' is overwritten with the contents of the : // file, or, in the case of disk failure, returns OK and sets : // 'health_status_' to a non-OK value. : Status LoadFromDisk(); > this is weird. whats the reason for this not to return a non-ok status righ Disk failure errors here would be ignored anyway. By adding this to the contract, we don't have to sully the call-site with disk-handling code, and we can keep the error handling (i.e. setting health_status_) internal. PS4, Line 83: // Whether or not the instance is healthy. If the instance file lives on a : // disk that has failed, this should return false. : bool healthy() const { : return health_status_.ok(); : } > we usually don't have this extra getter (i.e. consider just doing pimf.heal As per Mike's comment, I think it helps for readability. Also, I get the feeling that we don't use this elsewhere (at least for tablets/replicas) because other components have multiple states. In this case, it's pretty cut-and-dry: healthy, not healthy. PS4, Line 83: // Whether or not the instance is healthy. If the instance file lives on a : // disk that has failed, this should return false. : bool healthy() const { : return health_status_.ok(); : } > I had the same initial reaction but it seems to make the code that uses it Done http://gerrit.cloudera.org:8080/#/c/7784/4/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: Line 116: using std::make_pair; > warning: using decl 'make_pair' is unused [misc-unused-using-decls] Done PS4, Line 794: if (FindUuidByRoot(root, &uuid)) { : return FindUuidIndexByUuid(uuid, uuid_idx); : } : return false; > there are no chances of races here, right? Nope, the first call is guarded by a scoped lock, as is the second. These values don't change after they're first set in Open(). http://gerrit.cloudera.org:8080/#/c/7784/4/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: PS4, Line 366: Finds the UUID for the given canonicalized root directory name. > nit, for consistency append the same suffix as the previous new method (ret Done http://gerrit.cloudera.org:8080/#/c/7784/3/src/kudu/fs/fs_manager-test.cc File src/kudu/fs/fs_manager-test.cc: Line 279: FLAGS_env_inject_eio = 0; > nit: shouldn't need this due to flag saver in KuduTest unless I'm missing s See Adar's comment. Line 279: FLAGS_env_inject_eio = 0; > Unfortunately, KuduTest doesn't restore the flags until _after_ TearDown() Thanks for the explanation. Line 302: FLAGS_env_inject_eio = 0; > same See above. http://gerrit.cloudera.org:8080/#/c/7784/3/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: PS3, Line 180: set<string> all_roots; : all_roots.insert(wal_fs_root_); : for (const string& data_fs_root : data_fs_roots_) { : all_roots.insert(data_fs_root); : } > nit: consider Done PS3, Line 228: if (!ContainsKey(unique_roots, root.path)) { : canonicalized_data_fs_roots_.emplace_back(root); : canonicalized_all_fs_roots_.emplace_back(root); : InsertOrDie(&unique_roots, root.path); : } > Consider using InsertIfNotPresent() instead to avoid double look-ups: Done PS3, Line 248: string wal_root > Is it necessary to make a copy here? Why not to use const reference here i Done PS3, Line 251: string meta_root > ditto Done -- To view, visit http://gerrit.cloudera.org:8080/7784 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib2a1c824526ed52a6b90ddfbc735cecc4c491118 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
