[jira] [Commented] (HBASE-7377) Clean up TestHBase7051

["Hadoop QA (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HBASE-7377?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13535238#comment-13535238 ] 

Hadoop QA commented on HBASE-7377:
----------------------------------

{color:red}-1 overall{color}.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12561549/HBASE-7377-94.patch
  against trunk revision .

    {color:green}+1 @author{color}.  The patch does not contain any @author tags.

    {color:green}+1 tests included{color}.  The patch appears to include 3 new or modified tests.

    {color:red}-1 patch{color}.  The patch command could not apply the patch.

Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3592//console

This message is automatically generated.
                
> Clean up TestHBase7051
> ----------------------
>
>                 Key: HBASE-7377
>                 URL: https://issues.apache.org/jira/browse/HBASE-7377
>             Project: HBase
>          Issue Type: Improvement
>    Affects Versions: 0.94.3, 0.96.0
>            Reporter: Gregory Chanan
>            Assignee: Gregory Chanan
>            Priority: Minor
>             Fix For: 0.96.0, 0.94.4
>
>         Attachments: HBASE-7377-94.patch
>
>
> TestHBase7051 tests a race condition between checkAndPuts and puts, but is hard to follow and verify correctness on for a few reasons:
> - there are no comments and the steps of the test are numbers
> - there are variables that are read/written on different threads with no synchronization/volatile (e.g. checkAndPutCompleted).  This may be okay depending on the other synchronization, but hard to verify.
> - the worker threads are not checked for exceptions.  This may also be okay if it is not possible for the test to succeed if exceptions are thrown, but hard to verify.
> - The test seems to work, but I'm not sure if it's for the reasons expected.  HBASE-7051 is recreated if the following steps occur:
> 1) Put releases row lock (where we are now)
> 2) CheckAndPut grabs row lock and reads the value prior to the put (10)
> because the MVCC has not advanced
> 3) Put advances MVCC
> On Step 1 the test does the following:
> {code}
> latch.await();
> {code}
> which waits for the checkAndPut to grab the row lock:
> {code}
>       latch.countDown();
>       return super.getLock(lockid, row, waitForLock);
> {code}
> But now we haven't really done anything to guarantee the race we want: the put will try to advance the MVCC and the checkAndPut will try to read and either could win, when we really want the checkAndPut read to win.
> Luckily, the put actually waits before advancing its MVCC, but only because it happens to fall through to the next case in the if:
> {code}
>     if (count == 2) {
>         try {
>           putCompleted = true;
>           super.releaseRowLock(lockId);
>           latch.await();
>         } catch (InterruptedException e) {
>           // TODO Auto-generated catch block
>           e.printStackTrace();
>         }
>       }
>       if (count == 3) {
>         super.releaseRowLock(lockId);
>         try {
>           Thread.sleep(1000);
>         } catch (InterruptedException e) {
>           // TODO Auto-generated catch block
>           e.printStackTrace();
>         }
>         latch.countDown();
>       }
> {code}
> which happens to sleep.  I doubt this is intentional.  Also, there are multiple latch.countDown calls even though the latch is initialized to 1.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira