Andrew Wong has posted comments on this change. Change subject: open FS layout in presence of disk failure ......................................................................
Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/7784/5/src/kudu/fs/block_manager_util.cc File src/kudu/fs/block_manager_util.cc: PS5, Line 68: > nit: this is not needed, right? Usually those macros are used as 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(); > I'm not sure I fully agree. For one the error message is confusing: "LOG(ER Hrm, I'd prefer to keep this internal. If anything of the PIMF fails, it gets set to failed. I'll improve the error message though. 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(); > Andrew and I previously discussed this approach. You can see our comments h Thanks for resurfacing this. http://gerrit.cloudera.org:8080/#/c/7784/5/src/kudu/fs/data_dirs-test.cc File src/kudu/fs/data_dirs-test.cc: PS5, Line 244: Status s = dd_manager_->MarkDataDirFailed(kNumDirs - 1); : ASSERT_STR_CONTAINS(s.ToString(), "All data dirs have failed"); > As I see the call to CreateDataDirGroup() is removed. Is that correct? I Ah, good point, I suppose I can add it in again. Done PS5, Line 363: // manager. : KuduTest::SetUp(); : } : > nit: consider making this method returning Status and then using ASSERT_OK( Done http://gerrit.cloudera.org:8080/#/c/7784/5/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: PS5, Line 1315: ASSERT_NE(data_dir, test_dirs[failed_idx]); > paranoid nit: does it make sense to check that the failed dir is exactly te Sure, although there is test coverage for this in data_dirs-test -- 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: 6 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
