Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8395 )
Change subject: error_manager: synchronize/serialize handling ...................................................................... Patch Set 9: (7 comments) http://gerrit.cloudera.org:8080/#/c/8395/9/src/kudu/fs/data_dirs-test.cc File src/kudu/fs/data_dirs-test.cc: http://gerrit.cloudera.org:8080/#/c/8395/9/src/kudu/fs/data_dirs-test.cc@398 PS9, Line 398: env_, test_roots_, DataDirManagerOptions(), &dd_manager_)); nit: spurious changes in this file 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@64 PS9, Line 64: std::stringstream out; : std::copy(test_vec_.begin(), test_vec_.end(), : std::ostream_iterator<int>(out, " ")); : return out.str(); nit: this would be easier to read as: return JoinInts(test_vec_, " "); http://gerrit.cloudera.org:8080/#/c/8395/9/src/kudu/fs/error_manager-test.cc@92 PS9, Line 92: template <int I> nit: why not just pass i as the first parameter to the method? http://gerrit.cloudera.org:8080/#/c/8395/9/src/kudu/fs/error_manager-test.cc@100 PS9, Line 100: test_vec_[first_available] = I; Using DFAKE_MUTEX and DFAKE_SCOPED_LOCK would be a nice verification mechanism for the thread safety here http://gerrit.cloudera.org:8080/#/c/8395/9/src/kudu/fs/error_manager-test.cc@116 PS9, Line 116: vector<int> test_vec_; Deserves a comment explaining what it is http://gerrit.cloudera.org:8080/#/c/8395/9/src/kudu/fs/error_manager-test.cc@122 PS9, Line 122: TEST_F(FsErrorManagerTest, TestBasicRegistration) { nit: could use a test comment http://gerrit.cloudera.org:8080/#/c/8395/9/src/kudu/fs/error_manager-test.cc@177 PS9, Line 177: = thread nit: you can delete this part of the assignment and just instantiate t with a constructor call -- 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: 9 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 21 Nov 2017 07:28:29 +0000 Gerrit-HasComments: Yes
