svn commit: r1393839 - in /hbase/branches/0.89-fb/src: main/java/org/apache/hadoop/hbase/regionserver/HRegion.java main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java test/java/org/apache/hadoop/hbase/thrift/TestThriftServerLegacy.java

[mb...@apache.org]

Author: mbautin
Date: Wed Oct  3 23:59:00 2012
New Revision: 1393839

URL: http://svn.apache.org/viewvc?rev=1393839&view=rev
Log:
[jira] [HBASE-6930] [89-fb] Fix TestThriftServerLegacy: notifyAll should be inside synchronized block, and a null checkAndMutate expected value should be handled correctly

Author: mbautin

Summary:
There were a couple of reasons why TestThriftServerLegacy has been failing recently in the HBase 89-fb branch:
- rHBASEEIGHTNINEFBBRANCH1393468 was calling notifyAll outside a synchronized block
- rHBASEEIGHTNINEFBBRANCH1391219 changed the meaning of a null expected value passed to checkAndMutate but that was not reflected in the Thrift handler

Test Plan: Run TestThriftServerLegacy

Reviewers: kannan, liyintang, kranganathan

Reviewed By: liyintang

Differential Revision: https://reviews.facebook.net/D5841

Modified:
    hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
    hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java
    hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerLegacy.java

Modified: hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java?rev=1393839&r1=1393838&r2=1393839&view=diff
==============================================================================
--- hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java (original)
+++ hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java Wed Oct  3 23:59:00 2012
@@ -2901,8 +2901,8 @@ public class HRegion implements HeapSize
         byte[] row = lockIds.remove(lockid);
         lockedRows.remove(row);
       }
+      lockedRows.notifyAll();
     }
-    lockedRows.notifyAll();
   }
 
   /**

Modified: hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java?rev=1393839&r1=1393838&r2=1393839&view=diff
==============================================================================
--- hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java (original)
+++ hbase/branches/0.89-fb/src/main/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java Wed Oct  3 23:59:00 2012
@@ -1060,6 +1060,11 @@ public class ThriftServerRunner implemen
         }
         byte[][] famAndQfCheck = KeyValue.parseColumn(getBytes(columnCheck));
         byte[] valueCheckBytes = getBytes(valueCheck);
+        if (valueCheck == null) {
+          // This means we are not expecting to see an existing value. An empty array would mean
+          // we are looking for an zero-length existing value.
+          valueCheckBytes = null;
+        }
 
         if (!delete.isEmpty() && !put.isEmpty()) {
           // can't do both, not atomic, not good idea!

Modified: hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerLegacy.java
URL: http://svn.apache.org/viewvc/hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerLegacy.java?rev=1393839&r1=1393838&r2=1393839&view=diff
==============================================================================
--- hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerLegacy.java (original)
+++ hbase/branches/0.89-fb/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerLegacy.java Wed Oct  3 23:59:00 2012
@@ -407,10 +407,11 @@ public class TestThriftServerLegacy exte
 
     // Apply timestamped Mutations to rowA
     long time1 = System.currentTimeMillis();
-    handler.checkAndMutateRowTs(tableAname, rowAname, columnAname, null,
-        getMutations(), time1, null);
+    assertTrue(handler.checkAndMutateRowTs(tableAname, rowAname, columnAname, null,
+        getMutations(), time1, null));
 
     List<TRowResult> res1 = handler.getRow(tableAname, rowAname, null);
+    assertEquals(1, res1.size());
     // Check that all went according to plan
     assertEquals(res1.get(0).columns.get(columnAname).value, valueAname);
     assertEquals(res1.get(0).columns.get(columnBname).value, valueBname);