Todd Lipcon has posted comments on this change. Change subject: disk failure: coordinate error handling ......................................................................
Patch Set 24: (6 comments) http://gerrit.cloudera.org:8080/#/c/7029/24/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 272: // Exposes the FsErrorManager used to fs errors. missing a word? http://gerrit.cloudera.org:8080/#/c/7029/24/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: PS24, Line 125: // The input to the callback is expected to be the UUID of a failed DataDir. I think it's clearer to have this information with the typedef of ErrorNotificationCb instead. Here, it sounds like it's describing some input to this call, not the parameter passed when the callback is invoked. Maybe: "If a disk error is detected, this callback will be invoked with the relevant DataDir UUID as its parameter." or something? Line 130: // This must be called before the callback's callee is destroyed. nit: can you add whether this is idempotent? safe to call if nothing is set yet? http://gerrit.cloudera.org:8080/#/c/7029/24/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: PS24, Line 169: in-memory what's this mean? I don't think this line adds much value since the fact that it's a constructor already sort of tells us that it creates an instance Line 195: FsErrorManager* error_manager() override { return error_manager_; } is this used? tried grepping for it and didn't see any call sites, but maybe it's coming in the next patch http://gerrit.cloudera.org:8080/#/c/7029/24/src/kudu/tserver/ts_tablet_manager.h File src/kudu/tserver/ts_tablet_manager.h: Line 189: void FailTabletsInDataDir(const string& uuid); nit: std::string in header -- To view, visit http://gerrit.cloudera.org:8080/7029 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib3169deada702c527b70fabf9d2223364f9a9ea6 Gerrit-PatchSet: 24 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
