Todd Lipcon has posted comments on this change. Change subject: [raft_consensus-itest] CHECK_OK --> ASSERT_OK where possible ......................................................................
Patch Set 1: I'm not sure about this change. The downside of ASSERT_OK is that it is not threadsafe, and some of these functions may be called from non-main threads. Additionally, I think it's useful in a test to separate the pieces of behavior that the test is really focusing on (where we use ASSERT) vs the things that it assumes "just work" such as pieces of test infrastructure or ancillary utility code (where CHECK is reasonable). The second point is somewhat subjective, though, and not written down anywhere, so willing to be convinced otherwise. But the first is a concern. Did you check whether all of these call sites are only used from the main thread? -- To view, visit http://gerrit.cloudera.org:8080/7172 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1bf7e5743a93619dd08db2519e2f5d9aeef24d86 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: No
