Adar Dembo has posted comments on this change. Change subject: open FS layout in presence of disk failure ......................................................................
Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/7784/3//COMMIT_MSG Commit Message: PS3, Line 15: all each just "each" PS3, Line 15: failsto fails to http://gerrit.cloudera.org:8080/#/c/7784/3/src/kudu/fs/data_dirs-test.cc File src/kudu/fs/data_dirs-test.cc: PS3, Line 244: ASSERT_DEATH({ : dd_manager_->MarkDataDirFailed(kNumDirs - 1); : }, ".*All data dirs have failed"); I still don't like this change; I prefer that "library code" like the DirectoryManager pass back an appropriate Status that would let callers decide whether to crash or not. http://gerrit.cloudera.org:8080/#/c/7784/3/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: Line 385: return Status::IOError("Could not create directory manager with disks failed"); Maybe include root.second in the error message? Line 607: MarkDataDirFailed(uuid_idx, refresh_status.ToString()); As future cleanup, it would be nice if MarkDataDirFailed() took a Status rather than a string. The former has more information in it. http://gerrit.cloudera.org:8080/#/c/7784/3/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 1592: if (dd_manager_->IsDataDirFailed(uuid_idx)) { If we stored the status passed to us in MarkDataDirFailed, we could return it to callers here so we wouldn't need to create artificial Statuses like this. -- 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: 3 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: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
