[HBASE-7377] Clean up TestHBase7051

• Type: Improvement
• Status: Closed
• Priority: Minor
• Resolution: Fixed
• Affects Version/s: 0.94.3, 0.95.2
• Fix Version/s: 0.94.4, 0.95.0
• Assignee: Gregory Chanan
• Reporter: Gregory Chanan
• Created: 18/Dec/12 02:14
• Updated: 26/Feb/13 08:22
• Resolved: 18/Dec/12 21:34

Description

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:

latch.await();

which waits for the checkAndPut to grab the row lock:

      latch.countDown();
      return super.getLock(lockid, row, waitForLock);

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:

    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();
      }

which happens to sleep. I doubt this is intentional. Also, there are multiple latch.countDown calls even though the latch is initialized to 1.