You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/06/13 19:01:47 UTC
[kudu-CR] [raft consensus-itest] CHECK OK --> ASSERT OK where possible
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7172
Change subject: [raft_consensus-itest] CHECK_OK --> ASSERT_OK where possible
......................................................................
[raft_consensus-itest] CHECK_OK --> ASSERT_OK where possible
This patch introduces a micro clean-up on the raft_consensus-itest: use
ASSERT_OK() instead of CHECK_OK() where possible.
For tests, it's more idiomatic to use ASSERT_OK() than CHECK_OK().
Change-Id: I1bf7e5743a93619dd08db2519e2f5d9aeef24d86
---
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
2 files changed, 66 insertions(+), 56 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/72/7172/1
--
To view, visit http://gerrit.cloudera.org:8080/7172
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1bf7e5743a93619dd08db2519e2f5d9aeef24d86
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [raft consensus-itest] CHECK OK --> ASSERT OK where possible
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has abandoned this change.
Change subject: [raft_consensus-itest] CHECK_OK --> ASSERT_OK where possible
......................................................................
Abandoned
not needed
--
To view, visit http://gerrit.cloudera.org:8080/7172
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: I1bf7e5743a93619dd08db2519e2f5d9aeef24d86
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [raft consensus-itest] CHECK OK --> ASSERT OK where possible
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin 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.
>
The updated functions/methods are called only from the main thread, AFAIK.
> 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).
By my understanding, using CHECK_OK() does not make much sense anywhere in test code except for the cases where {ASSERT,EXPECT}_OK() don't work or where it's necessary to collect the stack trace. Otherwise, why would we want to abort the test and skip running the rest of the test scenarios?
>
> 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?
Yes, I checked the callsites of the updated functions. That's why I added 'where possible' in the description.
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [raft consensus-itest] CHECK OK --> ASSERT OK where possible
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: [raft_consensus-itest] CHECK_OK --> ASSERT_OK where possible
......................................................................
Patch Set 1:
I didn't carefully review this whole patch, I just skimmed it but I'll chime in since I was chatting with Alexey about it over lunch. I'd say in general I'm supportive of these kind of changes with the following caveats / criteria in mind:
1. Typically anything at the top level of a test that is doing a CHECK can be safely replaced with an ASSERT. The Thread stuff is subjective because if starting a thread fails then there is some system issue, most likely unrelated to the test, but still I don't really see harm in changing those to ASSERT_OK().
2. For test utility functions, if I am writing something to be reused by many tests I will typically return a Status since it's a very intuitive interface in the context of the Kudu codebase. See cluster_itest_util.h for an example of that. But if I am writing helper functions for use only the current test suite I usually prefer a void return with ASSERT inside the function and NO_FATALS at the call site because it provides a form of stack trace at failure time and cuts down on debugging when a test fails (especially flaky failures).
I won't much get into non-main thread use of ASSERT (which doesn't work) since I don't think there is any disagreement there (I didn't audit this patch for that personally).
--
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [raft consensus-itest] CHECK OK --> ASSERT OK where possible
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No