Mike Percy has posted comments on this change. Change subject: disk failure: coordinate error handling ......................................................................
Patch Set 20: (5 comments) http://gerrit.cloudera.org:8080/#/c/7029/20/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: Line 297: uint16_t* GetUuidIdxForUuid(const std::string& uuid) { I looked through here and I believe this is safe. That said, this API is a bit scary at first glance. What lifetime guarantees do we make about the pointee here? (To me it looks like the ptr is good as long as the DataDirManager has been opened and was not yet destroyed.) We should at least document them. Still, I think it would be a little more intuitive if this returned a bool or a Status and took a uint16_t* out-parameter. http://gerrit.cloudera.org:8080/#/c/7029/20/src/kudu/fs/error_manager.h File src/kudu/fs/error_manager.h: Line 34: typedef Callback<void(const std::string&)> ErrorNotificationCb; doc callback argument Line 57: void RunErrorNotificationCb(const std::string& uuid) const { nit: arguments need docs Line 58: notify_cb_.Run(uuid); It makes me a little nervous not to submit this to a thread pool. We'd have to be careful to release any locks held which may not be easy to do at a low level if we are calling back into a higher level class. Although I guess if we don't need it then "YAGNI" Line 61: void RunErrorNotificationCb(const DataDir* dir) const { Can we remove this one? -- 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: 20 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
