Adar Dembo has posted comments on this change. Change subject: disk failure: error-handling macros in blocks ......................................................................
Patch Set 17: (7 comments) http://gerrit.cloudera.org:8080/#/c/7030/17/src/kudu/fs/file_block_manager.cc File src/kudu/fs/file_block_manager.cc: PS17, Line 534: Status sync = env_->SyncDir(s); : RETURN_NOT_OK_HANDLE_DISK_FAILURE(sync, : error_manager_->RunErrorNotificationCb(location.data_dir())); Combine? RETURN_NOT_OK_HANDLE_DISK_FAILURE(env_->SyncDir(s), error_manager_->RunErrorNotificationCb(location.data_dir())); http://gerrit.cloudera.org:8080/#/c/7030/14/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 1095: } > HandleError() is called any place with RETURN_NOT_OK_HANDLE_ERROR(). You're Right but I don't see any RETURN_NOT_OK_HANDLE_ERROR calls in LogWritableBlock functions. AFAICT, all the macro sites will resolve to other HandleError() instances, such as the one in LogBlockContainer. Same goes for LogReadableBlock::HandleError. Line 1745: } > This still indicates that part of the tablet's data is probably lost due to I see. I think the failure handling policy for a tablet is out of scope for the block managers (and thus they should operate in a way that's most predictable), but there's no point to implementing functionality that's never used. Perhaps add a comment here (and in the equivalent place in FileBlockManager) explaining that we _could_ satisfy the block on a different data dir, but there's currently no point in doing that? http://gerrit.cloudera.org:8080/#/c/7030/17/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 1304: void HandleError(const Status& s) const { Also unnecessar Line 2336: RETURN_NOT_OK_PREPEND(env_->SyncDir(dir->dir()), What about this one? Line 2345: Status LogBlockManager::RewriteMetadataFile(const string& metadata_file_name, And the calls in here? http://gerrit.cloudera.org:8080/#/c/7030/17/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: There are still some changes to this file that don't belong in this patch. They're minor, but we can avoid some confusion if we move them. -- To view, visit http://gerrit.cloudera.org:8080/7030 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia03bfb711a1b022d7516f4adb37fe9fb28ec949c Gerrit-PatchSet: 17 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: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
