Adar Dembo has posted comments on this change. Change subject: disk failure: add persistent disk states ......................................................................
Patch Set 24: (13 comments) I didn't finish reviewing data_dirs.cc. Looked at the rest, though. http://gerrit.cloudera.org:8080/#/c/7270/24/src/kudu/fs/block_manager_util.cc File src/kudu/fs/block_manager_util.cc: PS24, Line 54: s_ How about _s and _s2? Or _s and _s_prepended? Flipping the underscore position is a super confusing way to create a second variable name. PS24, Line 106: const google::protobuf::RepeatedPtrField<string> all_uuids_pb(all_uuids.begin(), all_uuids.end()); : new_path_set->mutable_deprecated_all_uuids()->Reserve(all_uuids.size()); : new_path_set->mutable_deprecated_all_uuids()->CopyFrom(all_uuids_pb); Wouldn't it be fewer LOC to add one UUID at a time to deprecated_all_uuids in the loop on L99? This isn't exactly a hot path. http://gerrit.cloudera.org:8080/#/c/7270/24/src/kudu/fs/data_dirs-test.cc File src/kudu/fs/data_dirs-test.cc: Line 356: KuduTest::SetUp(); Add a comment explaining this is here to bypass the DataDirsTest::SetUp impl. Or just deal with the results of the (unnecessary?) call. Anyone who looks at this will instinctively think this override does nothing and should be removed. http://gerrit.cloudera.org:8080/#/c/7270/24/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: PS24, Line 523: std::unordered_map using Line 653: dd->ExecClosure(Bind(&DeleteTmpFilesRecursively, env_, dd->dir())); Didn't we already do this above? http://gerrit.cloudera.org:8080/#/c/7270/24/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: PS24, Line 236: static Status CreateNew(Env* env, std::vector<std::string> data_fs_roots, : DataDirManagerOptions opts, : std::unique_ptr<DataDirManager>* dd_manager); : static Status OpenExisting(Env* env, std::vector<std::string> data_fs_roots, : DataDirManagerOptions opts, : std::unique_ptr<DataDirManager>* dd_manager); Can we suffix these with "ForTests"? It's temporary until canonicalization moves into the DataDirManager, right? http://gerrit.cloudera.org:8080/#/c/7270/24/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: PS24, Line 207: // If the directory fails to canonicalize due to disk failure, prefix : // it with an unsanitized string. Update. Line 242: // The server cannot start if the WAL root or metadata root failed. Failed to what? Canonicalize, right? PS24, Line 355: some directories How about "at least one directory" http://gerrit.cloudera.org:8080/#/c/7270/24/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: Line 35: #include "kudu/util/env_util.h" Not used? PS24, Line 1301: directory right You mean 'right directory'? http://gerrit.cloudera.org:8080/#/c/7270/24/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 1571: vector<uint16_t> uuid_indices(dd_manager_->data_dirs().size()); No longer used? Line 1932: if (!s.ok()) { Why these changes and the other macro changes below? -- To view, visit http://gerrit.cloudera.org:8080/7270 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifddf0817fe1a82044077f5544c400c88de20769f Gerrit-PatchSet: 24 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: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
