[
https://issues.apache.org/jira/browse/HBASE-5150?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13182738#comment-13182738
]
jirapos...@reviews.apache.org commented on HBASE-5150:
------------------------------------------------------
bq. On 2012-01-09 03:01:16, Prakash Khemani wrote:
bq. >
src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java,
line 316
bq. > <https://reviews.apache.org/r/3433/diff/2/?file=67170#file67170line316>
bq. >
bq. > There is no reason to wait for 3 seconds over here
bq.
bq. Jimmy Xiang wrote:
bq. If everything works as expected, we won't have to wait here, since
those two fail() calls won't be called. I'd like to give it some time to make
sure the second splitLogDistributed() is really blocked there.
bq.
bq. Prakash Khemani wrote:
bq. If the test is working as expecte then there will be a 3sec wait here.
This I still feel is unnecessary.
bq.
bq. If the second splitLogDistributed() isn't really blocked and
result.get(3000, millisecs) returns early then there is nothing in the test to
catch that case.
bq.
bq. I prefer the original thread based test rather than the executor based.
bq.
bq. Also there used to be a assertTrue(t.isIntruppted()) at the end of the
test. It's possible I missed checking it in. That was the test to make sure
that the split-logging thread had blocked and was actually interrupted. You
could also add a counter to test that case.
The point of the issue here is that the fail() and assertTrue() in the original
thread based test will throw some errors and terminate that thread, but it
doesn't fail the test. Only if you throw such errors in the main thread, it
will fail the test. With the executor based change, the result.get() call will
get those errors and fail the test.
As to the 3sec wait, it is a tough call. There are two threads: the main
thread and the log splitting thread. If the log splitting thread fails fast, by
the original way, we just know it is failed
(waitForCounter(tot_mgr_wait_for_zk_delete, 0, 1, 10000)) but we don't know why.
Can we change the wait time from 3sec to 1sec? If we don't wait at all, those
two fail()s may be better to change to some error logging messages. Otherwise,
we don't know the cause at all. We just know
tot_mgr_wait_for_zk_delete is not changed as expected.
bq. On 2012-01-09 03:01:16, Prakash Khemani wrote:
bq. > src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java,
line 481
bq. > <https://reviews.apache.org/r/3433/diff/2/?file=67171#file67171line481>
bq. >
bq. > The test should fail if there is an exception.
bq.
bq. Jimmy Xiang wrote:
bq. That fail() doesn't fail the test. If there is an exception, those
waitForCounter() will actually fail the test.
bq.
bq. Prakash Khemani wrote:
bq. fail() over here serves 3 purposes (1) It documents that we are not
expecting any exceptions in the thread and if there is one then the test should
fail. (2) if there is an exception then test fails quicker with the fail()
statement (3) if we don't put a fail() here then we will ignore any exception
thrown by the thread. the counter logic might not be strong enough to flag all
exceptions.
I understand the purposes. But it doesn't achieve some of them, for example,
as I mentioned earlier, it won't fail the test. There are two choices. (1) We
can put the message in some log. (2) Using executor to get the error in the
main thread. But that needs a wait in the main thread. There is already a
warning message "splitLogDistributed failed", should we also say the test is
failed too?
- Jimmy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3433/#review4259
-----------------------------------------------------------
On 2012-01-09 17:34:47, Jimmy Xiang wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/3433/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-01-09 17:34:47)
bq.
bq.
bq. Review request for hbase, Todd Lipcon, Ted Yu, Michael Stack, and Prakash
Khemani.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Cleaned up the tests introduced in HBASE-5081, added some documentation.
bq.
bq.
bq. This addresses bug HBASE-5150.
bq. https://issues.apache.org/jira/browse/HBASE-5150
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
0ef0e33
bq.
src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java
b0487f1
bq. src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java
0974b56
bq.
bq. Diff: https://reviews.apache.org/r/3433/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Affected tests: TestDistributedLogSplitting and TestSplitLogManager, are
passed
bq.
bq.
bq. Thanks,
bq.
bq. Jimmy
bq.
bq.
> Fail in a thread may not fail a test, clean up log splitting test
> -----------------------------------------------------------------
>
> Key: HBASE-5150
> URL: https://issues.apache.org/jira/browse/HBASE-5150
> Project: HBase
> Issue Type: Test
> Affects Versions: 0.94.0
> Reporter: Jimmy Xiang
> Assignee: Jimmy Xiang
> Priority: Minor
> Attachments: hbase-5150.txt
>
>
> This is to clean up some tests for HBASE-5081. The Assert.fail method in a
> separate thread will terminate the thread, but may not fail the test.
> We can use callable, so that we can get the error in getting the result.
> Some documentation to explain the test will be helpful too.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
