Adar Dembo has posted comments on this change. Change subject: disk failure: don't open tablets on failed disks ......................................................................
Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/7766/3/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: Line 986: if (PREDICT_FALSE(!failed_dirs.empty())) { Why do we need this condition? Won't the for loop no-op if failed_dirs is empty? http://gerrit.cloudera.org:8080/#/c/7766/3/src/kudu/fs/file_block_manager.cc File src/kudu/fs/file_block_manager.cc: PS3, Line 581: // Don't report failed directories. > shoudln't we _report_ failed directories? ok if you want to do it in a foll There's some confusion about this, I think. The FsReport is intended for the block manager to report fine grained details about the filesystem; if a data directory has "failed", the block manager can't open its on-disk state on that directory and thus can't include it in the report. This is more of an issue for the log block manager, where we actually report something meaningful. http://gerrit.cloudera.org:8080/#/c/7766/3/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 1694: // Ignore attempts to delete blocks in failed directories. Shouldn't this precede the RemoveLogBlockUnlocked? This is deleting the block from in-memory maps, but not from disk. That makes LogBlockManager::DeleteBlock differ from FileBlockManager::DeleteBlock in semantics. Line 1702: return Status::OK(); > also: if these are the semantics we want (not sure), shouldn't we have a un Agreed with Mike: if return an error, don't callers handle it appropriately (i.e. WARN or whatever since we don't care that much if block deletion fails)? -- To view, visit http://gerrit.cloudera.org:8080/7766 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id3fae98355657f6aa4b134c542f92fc07f5c0aa1 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
