Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8395 )
Change subject: error_manager: synchronize/serialize handling ...................................................................... Patch Set 11: (2 comments) looks good, just a couple nits http://gerrit.cloudera.org:8080/#/c/8395/9/src/kudu/fs/error_manager-test.cc File src/kudu/fs/error_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/8395/9/src/kudu/fs/error_manager-test.cc@92 PS9, Line 92: template <int I> > It's an ErrorNotificationCb and needs this signature. Will add a comment. Why not just "curry" it, which means embed the parameter at the bind site? So instead of this: Bind(&FsErrorManagerTest::SleepAndWriteFirstEmptyCb<ErrorHandlerType::TABLET>, this); write: Bind(&FsErrorManagerTest::SleepAndWriteFirstEmptyCb, this, ErrorHandlerType::TABLET); and the resulting callback, if the signature was SleepAndWriteFirstEmptyCb(ErrorHandlerType type, const string& s) will now be a callback that only takes const string& s. It basically amounts to the same thing but omits the unnecessary templating in this case. http://gerrit.cloudera.org:8080/#/c/8395/11/src/kudu/fs/error_manager-test.cc File src/kudu/fs/error_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/8395/11/src/kudu/fs/error_manager-test.cc@184 PS11, Line 184: thread( since you are using emplace_back, you can now omit the thread( part also -- To view, visit http://gerrit.cloudera.org:8080/8395 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie61c408a0b4424f933f40a31147568c2f906be0e Gerrit-Change-Number: 8395 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 21 Nov 2017 20:49:38 +0000 Gerrit-HasComments: Yes
