HBase
  1. HBase
  2. HBASE-4528

The put operation can release the rowlock before sync-ing the Hlog

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.94.0
    • Component/s: regionserver
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Adds early-lock-release ability so we can do the WAL sync outside of the row lock. Extends MemStore/RWCC to support rollback.

      Description

      This allows for better throughput when there are hot rows. A single row update improves from 100 puts/sec/server to 5000 puts/sec/server.

      1. 4528-trunk-v9.txt
        36 kB
        Ted Yu
      2. 4528-trunk.txt
        42 kB
        Ted Yu
      3. HBASE-4528-Trunk-FINAL.patch
        38 kB
        Jonathan Gray
      4. appendNoSyncPut8.txt
        36 kB
        dhruba borthakur
      5. appendNoSyncPut7.txt
        36 kB
        dhruba borthakur
      6. appendNoSyncPut6.txt
        33 kB
        dhruba borthakur
      7. appendNoSyncPut5.txt
        33 kB
        dhruba borthakur
      8. appendNoSync5.txt
        33 kB
        dhruba borthakur
      9. appendNoSyncPut4.txt
        30 kB
        dhruba borthakur
      10. appendNoSyncPut3.txt
        26 kB
        dhruba borthakur
      11. appendNoSyncPut2.txt
        18 kB
        dhruba borthakur
      12. appendNoSyncPut1.txt
        17 kB
        dhruba borthakur

        Activity

        Hide
        Hudson added a comment -

        Integrated in HBase-0.94-security-on-Hadoop-23 #7 (See https://builds.apache.org/job/HBase-0.94-security-on-Hadoop-23/7/)
        HBASE-6321 ReplicationSource dies reading the peer's id
        HBASE-6647 [performance regression] appendNoSync/HBASE-4528 doesn't
        take deferred log flush into account (Revision 1379290)

        Result = FAILURE
        jdcryans :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
        Show
        Hudson added a comment - Integrated in HBase-0.94-security-on-Hadoop-23 #7 (See https://builds.apache.org/job/HBase-0.94-security-on-Hadoop-23/7/ ) HBASE-6321 ReplicationSource dies reading the peer's id HBASE-6647 [performance regression] appendNoSync/ HBASE-4528 doesn't take deferred log flush into account (Revision 1379290) Result = FAILURE jdcryans : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94-security #51 (See https://builds.apache.org/job/HBase-0.94-security/51/)
        HBASE-6321 ReplicationSource dies reading the peer's id
        HBASE-6647 [performance regression] appendNoSync/HBASE-4528 doesn't
        take deferred log flush into account (Revision 1379290)

        Result = FAILURE
        jdcryans :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
        Show
        Hudson added a comment - Integrated in HBase-0.94-security #51 (See https://builds.apache.org/job/HBase-0.94-security/51/ ) HBASE-6321 ReplicationSource dies reading the peer's id HBASE-6647 [performance regression] appendNoSync/ HBASE-4528 doesn't take deferred log flush into account (Revision 1379290) Result = FAILURE jdcryans : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
        Hide
        Hudson added a comment -

        Integrated in HBase-0.94 #445 (See https://builds.apache.org/job/HBase-0.94/445/)
        HBASE-6321 ReplicationSource dies reading the peer's id
        HBASE-6647 [performance regression] appendNoSync/HBASE-4528 doesn't
        take deferred log flush into account (Revision 1379290)

        Result = FAILURE
        jdcryans :
        Files :

        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java
        • /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
        Show
        Hudson added a comment - Integrated in HBase-0.94 #445 (See https://builds.apache.org/job/HBase-0.94/445/ ) HBASE-6321 ReplicationSource dies reading the peer's id HBASE-6647 [performance regression] appendNoSync/ HBASE-4528 doesn't take deferred log flush into account (Revision 1379290) Result = FAILURE jdcryans : Files : /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeper.java /hbase/branches/0.94/src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
        Hide
        Lars Hofhansl added a comment -

        -1 on backporting. 0.94 will be branched in less than a month.
        There are many, many more performance enhancement in 0.94, we cannot backport them all and neither should we.

        Show
        Lars Hofhansl added a comment - -1 on backporting. 0.94 will be branched in less than a month. There are many, many more performance enhancement in 0.94, we cannot backport them all and neither should we.
        Hide
        Jonathan Gray added a comment -

        @Mubarek, since it's a performance optimization and new feature, it's not going to be committed into the 90/92 branches. That being said, this patch could be backported if someone wanted to use it on a 92 branch (90 might be significantly more difficult, not sure).

        Show
        Jonathan Gray added a comment - @Mubarek, since it's a performance optimization and new feature, it's not going to be committed into the 90/92 branches. That being said, this patch could be backported if someone wanted to use it on a 92 branch (90 might be significantly more difficult, not sure).
        Hide
        Mubarak Seyed added a comment -

        Is there any plan on backporting to 92.1 and/or 0.90.7?

        Show
        Mubarak Seyed added a comment - Is there any plan on backporting to 92.1 and/or 0.90.7?
        Hide
        Hudson added a comment -

        Integrated in HBase-TRUNK #2380 (See https://builds.apache.org/job/HBase-TRUNK/2380/)
        HBASE-4528 The put operation can release the rowlock before sync-ing the Hlog (dhruba via jgray)

        jgray :
        Files :

        • /hbase/trunk/CHANGES.txt
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java
        • /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java
        • /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java
        Show
        Hudson added a comment - Integrated in HBase-TRUNK #2380 (See https://builds.apache.org/job/HBase-TRUNK/2380/ ) HBASE-4528 The put operation can release the rowlock before sync-ing the Hlog (dhruba via jgray) jgray : Files : /hbase/trunk/CHANGES.txt /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java /hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java /hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java
        Hide
        Lars Hofhansl added a comment -

        yeah! This is awesome.

        Show
        Lars Hofhansl added a comment - yeah! This is awesome.
        Hide
        Jonathan Gray added a comment -

        Committed to trunk. Thanks Dhruba for the awesome work, Thanks Ted and others for all the good reviews.

        Show
        Jonathan Gray added a comment - Committed to trunk. Thanks Dhruba for the awesome work, Thanks Ted and others for all the good reviews.
        Hide
        Ted Yu added a comment -

        I ran the 4 failed tests reported by HadoopQA on MacBook and they all passed.
        More importantly https://builds.apache.org/job/PreCommit-HBASE-Build/87//testReport/org.apache.hadoop.hbase.master/TestDistributedLogSplitting/testOrphanLogCreation/ doesn't show NPE.

        I think 4528-trunk-v9.txt should be good to go.

        Show
        Ted Yu added a comment - I ran the 4 failed tests reported by HadoopQA on MacBook and they all passed. More importantly https://builds.apache.org/job/PreCommit-HBASE-Build/87//testReport/org.apache.hadoop.hbase.master/TestDistributedLogSplitting/testOrphanLogCreation/ doesn't show NPE. I think 4528-trunk-v9.txt should be good to go.
        Hide
        Ted Yu added a comment -

        Remove extraneous changes for pom.xml

        Show
        Ted Yu added a comment - Remove extraneous changes for pom.xml
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12501323/4528-trunk-v9.txt
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 18 new or modified tests.

        -1 javadoc. The javadoc tool appears to have generated -167 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.coprocessor.TestMasterObserver
        org.apache.hadoop.hbase.master.TestDistributedLogSplitting
        org.apache.hadoop.hbase.coprocessor.TestCoprocessorEndpoint
        org.apache.hadoop.hbase.coprocessor.TestRegionServerCoprocessorExceptionWithAbort

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/87//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/87//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/87//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12501323/4528-trunk-v9.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 18 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -167 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.coprocessor.TestMasterObserver org.apache.hadoop.hbase.master.TestDistributedLogSplitting org.apache.hadoop.hbase.coprocessor.TestCoprocessorEndpoint org.apache.hadoop.hbase.coprocessor.TestRegionServerCoprocessorExceptionWithAbort Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/87//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/87//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/87//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        Run test suite without changes to HLogSplitter.java

        Show
        Ted Yu added a comment - Run test suite without changes to HLogSplitter.java
        Hide
        Ted Yu added a comment -

        All the hadoop slots on Apache Jenkins are up.
        I think we should attach the final patch for TRUNK and resubmit for PreCommit build.

        Show
        Ted Yu added a comment - All the hadoop slots on Apache Jenkins are up. I think we should attach the final patch for TRUNK and resubmit for PreCommit build.
        Hide
        stack added a comment -

        @Ted So you are +1?

        Show
        stack added a comment - @Ted So you are +1?
        Hide
        Ted Yu added a comment -

        Using TRUNK, I wasn't able to reproduce test failure for TestDistributedLogSplitting:

          622  ~/runtest.sh 3 TestDistributedLogSplitting#testWorkerAbort
          623  ~/runtest.sh 3 TestDistributedLogSplitting#testWorkerAbort
          624  ~/runtest.sh 3 TestDistributedLogSplitting
        

        The changes to HLogSplitter.java can be omitted at time of integration.

        Show
        Ted Yu added a comment - Using TRUNK, I wasn't able to reproduce test failure for TestDistributedLogSplitting: 622 ~/runtest.sh 3 TestDistributedLogSplitting#testWorkerAbort 623 ~/runtest.sh 3 TestDistributedLogSplitting#testWorkerAbort 624 ~/runtest.sh 3 TestDistributedLogSplitting The changes to HLogSplitter.java can be omitted at time of integration.
        Hide
        Ted Yu added a comment -

        @Dhruba:
        Can you take a look at 4528-trunk.txt w.r.t. my comment @ 27/Oct/11 22:09 ?

        With 4528-trunk.txt and the patch from HBASE-4692, we should be able to verify whether TestDistributedLogSplitting passes.

        Thanks

        Show
        Ted Yu added a comment - @Dhruba: Can you take a look at 4528-trunk.txt w.r.t. my comment @ 27/Oct/11 22:09 ? With 4528-trunk.txt and the patch from HBASE-4692 , we should be able to verify whether TestDistributedLogSplitting passes. Thanks
        Hide
        dhruba borthakur added a comment -

        Hi Ted, do you want me to make any changes to the patch? I am not clear on what the "next steps" are.

        Show
        dhruba borthakur added a comment - Hi Ted, do you want me to make any changes to the patch? I am not clear on what the "next steps" are.
        Hide
        Ted Yu added a comment - - edited

        From src/hdfs/org/apache/hadoop/hdfs/DFSClient.java:

            private void closeThreads() throws IOException {
              try {
                streamer.close();
                streamer.join();
                
        

        Besides closeInternal(), closeThreads() may be called from sync().
        Looks like closeThreads() should be made re-entrant (by adding streamer = null; assignment after streamer.join() call).
        So ignoring NPE shouldn't be a problem.

        TRUNK build is broken at the moment.
        I will try to reproduce the NPE after TRUNK becomes stable.

        Show
        Ted Yu added a comment - - edited From src/hdfs/org/apache/hadoop/hdfs/DFSClient.java: private void closeThreads() throws IOException { try { streamer.close(); streamer.join(); Besides closeInternal(), closeThreads() may be called from sync(). Looks like closeThreads() should be made re-entrant (by adding streamer = null; assignment after streamer.join() call). So ignoring NPE shouldn't be a problem. TRUNK build is broken at the moment. I will try to reproduce the NPE after TRUNK becomes stable.
        Hide
        Ted Yu added a comment -

        I didn't keep the whole output file for the failed test. I need to look at 0.20.205 source code for better understanding of the above NPE.
        The additional log should be changed to WARN.

        Show
        Ted Yu added a comment - I didn't keep the whole output file for the failed test. I need to look at 0.20.205 source code for better understanding of the above NPE. The additional log should be changed to WARN.
        Hide
        Jonathan Gray added a comment -

        Is it safe to ignore this close? Should it be a WARN not DEBUG? I'm a little confused why this is happening in the test. Is the FS being closed before this finishes or what?

        Show
        Jonathan Gray added a comment - Is it safe to ignore this close? Should it be a WARN not DEBUG? I'm a little confused why this is happening in the test. Is the FS being closed before this finishes or what?
        Hide
        Ted Yu added a comment -

        Run test suite on PreCommit.

        Show
        Ted Yu added a comment - Run test suite on PreCommit.
        Hide
        Ted Yu added a comment -

        Patch incorporating suggested fix for HLogSplitter.java

        Show
        Ted Yu added a comment - Patch incorporating suggested fix for HLogSplitter.java
        Hide
        Ted Yu added a comment -

        Found the following in output file for a failed test:

        2011-10-27 14:25:15,878 DEBUG [10.246.204.28,52533,1319750687794.splitLogManagerTimeoutMonitor] master.SplitLogManager$TimeoutMonitor(829): total tasks = 1 unassigned = 1
        2011-10-27 14:25:16,126 INFO  [SplitLogWorker-10.246.204.28,52535,1319750687853] regionserver.SplitLogWorker(308): worker 10.246.204.28,52535,1319750687853 done with task /hbase/splitlog/hdfs%3A%2F%2Flocalhost%3A52522%2Fuser%2Fzhihyu%2F.logs%2F10.246.204.28%2C52535%2C1319750687853%2F10.246.204.28%252C52535%252C1319750687853.1319750690240 in 5049ms
        2011-10-27 14:25:16,126 ERROR [SplitLogWorker-10.246.204.28,52535,1319750687853] regionserver.SplitLogWorker(169): unexpected error
        java.lang.NullPointerException
          at org.apache.hadoop.hdfs.DFSClient$DFSOutputStream.closeThreads(DFSClient.java:3648)
          at org.apache.hadoop.hdfs.DFSClient$DFSOutputStream.closeInternal(DFSClient.java:3691)
          at org.apache.hadoop.hdfs.DFSClient$DFSOutputStream.close(DFSClient.java:3626)
          at org.apache.hadoop.fs.FSDataOutputStream$PositionCache.close(FSDataOutputStream.java:61)
          at org.apache.hadoop.fs.FSDataOutputStream.close(FSDataOutputStream.java:86)
          at org.apache.hadoop.io.SequenceFile$Writer.close(SequenceFile.java:966)
          at org.apache.hadoop.hbase.regionserver.wal.SequenceFileLogWriter.close(SequenceFileLogWriter.java:177)
          at org.apache.hadoop.hbase.regionserver.wal.HLogSplitter.splitLogFileToTemp(HLogSplitter.java:458)
          at org.apache.hadoop.hbase.regionserver.wal.HLogSplitter.splitLogFileToTemp(HLogSplitter.java:352)
          at org.apache.hadoop.hbase.regionserver.SplitLogWorker$1.exec(SplitLogWorker.java:113)
          at org.apache.hadoop.hbase.regionserver.SplitLogWorker.grabTask(SplitLogWorker.java:266)
          at org.apache.hadoop.hbase.regionserver.SplitLogWorker.taskLoop(SplitLogWorker.java:197)
          at org.apache.hadoop.hbase.regionserver.SplitLogWorker.run(SplitLogWorker.java:165)
          at java.lang.Thread.run(Thread.java:680)
        2011-10-27 14:25:16,126 INFO  [SplitLogWorker-10.246.204.28,52535,1319750687853] regionserver.SplitLogWorker(171): SplitLogWorker 10.246.204.28,52535,1319750687853 exiting
        2011-10-27 14:25:16,878 DEBUG [10.246.204.28,52533,1319750687794.splitLogManagerTimeoutMonitor] master.SplitLogManager$TimeoutMonitor(829): total tasks = 1 unassigned = 1
        

        I don't think NPE out of DFSClient.java for closing is critical.
        The following patch allowed me to loop through TestDistributedLogSplitting on MacBook 9 times without hitting test failure.

        Index: src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
        ===================================================================
        --- src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java	(revision 1190003)
        +++ src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java	(working copy)
        @@ -455,7 +455,11 @@
                 }
                 n++;
                 WriterAndPath wap = (WriterAndPath)o;
        -        wap.w.close();
        +        try {
        +          wap.w.close();
        +        } catch (Exception e) {
        +          LOG.debug("splitLogFileToTemp closing SequenceFileLogWriter", e);
        +        }
                 LOG.debug("Closed " + wap.p);
               }
               String msg = ("processed " + editsCount + " edits across " + n + " regions" +
        
        Show
        Ted Yu added a comment - Found the following in output file for a failed test: 2011-10-27 14:25:15,878 DEBUG [10.246.204.28,52533,1319750687794.splitLogManagerTimeoutMonitor] master.SplitLogManager$TimeoutMonitor(829): total tasks = 1 unassigned = 1 2011-10-27 14:25:16,126 INFO [SplitLogWorker-10.246.204.28,52535,1319750687853] regionserver.SplitLogWorker(308): worker 10.246.204.28,52535,1319750687853 done with task /hbase/splitlog/hdfs%3A%2F%2Flocalhost%3A52522%2Fuser%2Fzhihyu%2F.logs%2F10.246.204.28%2C52535%2C1319750687853%2F10.246.204.28%252C52535%252C1319750687853.1319750690240 in 5049ms 2011-10-27 14:25:16,126 ERROR [SplitLogWorker-10.246.204.28,52535,1319750687853] regionserver.SplitLogWorker(169): unexpected error java.lang.NullPointerException at org.apache.hadoop.hdfs.DFSClient$DFSOutputStream.closeThreads(DFSClient.java:3648) at org.apache.hadoop.hdfs.DFSClient$DFSOutputStream.closeInternal(DFSClient.java:3691) at org.apache.hadoop.hdfs.DFSClient$DFSOutputStream.close(DFSClient.java:3626) at org.apache.hadoop.fs.FSDataOutputStream$PositionCache.close(FSDataOutputStream.java:61) at org.apache.hadoop.fs.FSDataOutputStream.close(FSDataOutputStream.java:86) at org.apache.hadoop.io.SequenceFile$Writer.close(SequenceFile.java:966) at org.apache.hadoop.hbase.regionserver.wal.SequenceFileLogWriter.close(SequenceFileLogWriter.java:177) at org.apache.hadoop.hbase.regionserver.wal.HLogSplitter.splitLogFileToTemp(HLogSplitter.java:458) at org.apache.hadoop.hbase.regionserver.wal.HLogSplitter.splitLogFileToTemp(HLogSplitter.java:352) at org.apache.hadoop.hbase.regionserver.SplitLogWorker$1.exec(SplitLogWorker.java:113) at org.apache.hadoop.hbase.regionserver.SplitLogWorker.grabTask(SplitLogWorker.java:266) at org.apache.hadoop.hbase.regionserver.SplitLogWorker.taskLoop(SplitLogWorker.java:197) at org.apache.hadoop.hbase.regionserver.SplitLogWorker.run(SplitLogWorker.java:165) at java.lang. Thread .run( Thread .java:680) 2011-10-27 14:25:16,126 INFO [SplitLogWorker-10.246.204.28,52535,1319750687853] regionserver.SplitLogWorker(171): SplitLogWorker 10.246.204.28,52535,1319750687853 exiting 2011-10-27 14:25:16,878 DEBUG [10.246.204.28,52533,1319750687794.splitLogManagerTimeoutMonitor] master.SplitLogManager$TimeoutMonitor(829): total tasks = 1 unassigned = 1 I don't think NPE out of DFSClient.java for closing is critical. The following patch allowed me to loop through TestDistributedLogSplitting on MacBook 9 times without hitting test failure. Index: src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java =================================================================== --- src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java (revision 1190003) +++ src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java (working copy) @@ -455,7 +455,11 @@ } n++; WriterAndPath wap = (WriterAndPath)o; - wap.w.close(); + try { + wap.w.close(); + } catch (Exception e) { + LOG.debug( "splitLogFileToTemp closing SequenceFileLogWriter" , e); + } LOG.debug( "Closed " + wap.p); } String msg = ( "processed " + editsCount + " edits across " + n + " regions" +
        Hide
        Jonathan Gray added a comment -

        +1 on adding the log line Ted. Will do.

        I will try to spend time looking at the unit test tonight.

        Show
        Jonathan Gray added a comment - +1 on adding the log line Ted. Will do. I will try to spend time looking at the unit test tonight.
        Hide
        Ted Yu added a comment -

        I found no log statements in any of the rollback methods.
        I think at least we should add one LOG statement at the beginning of rollbackMemstore(), such as:

            LOG.debug("rollbackMemstore start:" + start + " end:" + end);
        
        Show
        Ted Yu added a comment - I found no log statements in any of the rollback methods. I think at least we should add one LOG statement at the beginning of rollbackMemstore(), such as: LOG.debug( "rollbackMemstore start:" + start + " end:" + end);
        Hide
        Ted Yu added a comment -

        The following test failure seems to be reproducible based on HBASE-4528-Trunk-FINAL.patch:

        Failed tests:   testWorkerAbort(org.apache.hadoop.hbase.master.TestDistributedLogSplitting): expected:<1> but was:<0>
        
        Show
        Ted Yu added a comment - The following test failure seems to be reproducible based on HBASE-4528 -Trunk-FINAL.patch: Failed tests: testWorkerAbort(org.apache.hadoop.hbase.master.TestDistributedLogSplitting): expected:<1> but was:<0>
        Hide
        Ted Yu added a comment -

        I am fine with the current form of this feature.
        Depending on how often memstoreRollback is executed in production, we can formulate the next step with another JIRA.

        Thanks for the nice job done, Dhruba.

        Show
        Ted Yu added a comment - I am fine with the current form of this feature. Depending on how often memstoreRollback is executed in production, we can formulate the next step with another JIRA. Thanks for the nice job done, Dhruba.
        Hide
        dhruba borthakur added a comment -

        TestDistibutedLogSplitting fails without this patch too. The other tests pass in my setup. I am rerunnign test again.

        Ted: the TimeRangeTracker seems to me to be a heuristic, it can for sure tell if the key does not belong in the file. The fact that we are not rolling back timeRangeTracker for a memstoreRollback means that there might be a case where we will look for this key in this store file although the key does not belong to this store file, but that should be ok.

        Show
        dhruba borthakur added a comment - TestDistibutedLogSplitting fails without this patch too. The other tests pass in my setup. I am rerunnign test again. Ted: the TimeRangeTracker seems to me to be a heuristic, it can for sure tell if the key does not belong in the file. The fact that we are not rolling back timeRangeTracker for a memstoreRollback means that there might be a case where we will look for this key in this store file although the key does not belong to this store file, but that should be ok.
        Hide
        stack added a comment -

        Do these tests fail w/o this patch?

        org.apache.hadoop.hbase.master.TestDistributedLogSplitting
        org.apache.hadoop.hbase.coprocessor.TestRegionServerCoprocessorExceptionWithAbort
        org.apache.hadoop.hbase.coprocessor.TestMasterObserver
        org.apache.hadoop.hbase.master.TestMasterFailover
        

        I see that TestMasterObserver has been failing recently.... but TestMasterFailover and TestDistributedLogSplitting?

        I"m looking at TestMasterObserver here. When I run it singly, it passes – the f'er

        Show
        stack added a comment - Do these tests fail w/o this patch? org.apache.hadoop.hbase.master.TestDistributedLogSplitting org.apache.hadoop.hbase.coprocessor.TestRegionServerCoprocessorExceptionWithAbort org.apache.hadoop.hbase.coprocessor.TestMasterObserver org.apache.hadoop.hbase.master.TestMasterFailover I see that TestMasterObserver has been failing recently.... but TestMasterFailover and TestDistributedLogSplitting? I"m looking at TestMasterObserver here. When I run it singly, it passes – the f'er
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12500956/HBASE-4528-Trunk-FINAL.patch
        against trunk revision .

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 13 new or modified tests.

        -1 javadoc. The javadoc tool appears to have generated -167 warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed these unit tests:
        org.apache.hadoop.hbase.master.TestDistributedLogSplitting
        org.apache.hadoop.hbase.coprocessor.TestRegionServerCoprocessorExceptionWithAbort
        org.apache.hadoop.hbase.coprocessor.TestMasterObserver
        org.apache.hadoop.hbase.master.TestMasterFailover

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/74//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/74//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/74//console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12500956/HBASE-4528-Trunk-FINAL.patch against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 13 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -167 warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests: org.apache.hadoop.hbase.master.TestDistributedLogSplitting org.apache.hadoop.hbase.coprocessor.TestRegionServerCoprocessorExceptionWithAbort org.apache.hadoop.hbase.coprocessor.TestMasterObserver org.apache.hadoop.hbase.master.TestMasterFailover Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/74//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/74//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/74//console This message is automatically generated.
        Hide
        Ted Yu added a comment -

        I think TimeRangeTracker should be updated during rollback.
        See my comment @ 23/Oct/11 10:45

        Show
        Ted Yu added a comment - I think TimeRangeTracker should be updated during rollback. See my comment @ 23/Oct/11 10:45
        Hide
        Jonathan Gray added a comment -

        Sorry Ted, I'm not clear on what exactly you're pointing out. Is something broken there?

        Show
        Jonathan Gray added a comment - Sorry Ted, I'm not clear on what exactly you're pointing out. Is something broken there?
        Hide
        Ted Yu added a comment -

        What about TimeRangeTracker ?

        Show
        Ted Yu added a comment - What about TimeRangeTracker ?
        Hide
        Jonathan Gray added a comment -

        Patch being committed. This is dhruba's appendNoSyncPut8.txt rebased on trunk. One small change was needed in TestParallelPut in order to compile.

        Show
        Jonathan Gray added a comment - Patch being committed. This is dhruba's appendNoSyncPut8.txt rebased on trunk. One small change was needed in TestParallelPut in order to compile.
        Hide
        Ted Yu added a comment -

        From StoreFile.Reader.passesTimerangeFilter():

            public boolean shouldSeek(Scan scan, final SortedSet<byte[]> columns) {
              return (passesTimerangeFilter(scan) && passesBloomFilter(scan, columns));
            }
        
            /**
             * Check if this storeFile may contain keys within the TimeRange
             * @param scan
             * @return False if it definitely does not exist in this StoreFile
             */
            private boolean passesTimerangeFilter(Scan scan) {
        
        Show
        Ted Yu added a comment - From StoreFile.Reader.passesTimerangeFilter(): public boolean shouldSeek(Scan scan, final SortedSet< byte []> columns) { return (passesTimerangeFilter(scan) && passesBloomFilter(scan, columns)); } /** * Check if this storeFile may contain keys within the TimeRange * @param scan * @ return False if it definitely does not exist in this StoreFile */ private boolean passesTimerangeFilter(Scan scan) {
        Hide
        dhruba borthakur added a comment -

        All unit tests (except DistributedLogSplitting) passes with this patch

        Show
        dhruba borthakur added a comment - All unit tests (except DistributedLogSplitting) passes with this patch
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/
        -----------------------------------------------------------

        (Updated 2011-10-22 15:55:20.687122)

        Review request for hbase.

        Changes
        -------

        Fixed two typos pointed out by Ted.

        TimeRangeTracker is not updated because it is a heuristic that records the (min, max) of the timestamps of the kvs in the memstore. We cannot update it during the rollback because we do not know what the next min or max is without scanning all the existing keys in the memstore. From what I understand, the timeRangeTracker is essentially a heuristic, so it is ok to not update it in the rare case of a rollback.

        Summary
        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        This addresses bug HBASE-4528.
        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs (updated)


        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1187667
        /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1187667
        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1187667
        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1187667
        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1187667
        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1187667
        /src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 1187667
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1187667
        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1187667

        Diff: https://reviews.apache.org/r/2141/diff

        Testing
        -------

        I ran TestLogRolling over and over again, about 50 times, not failed a single time.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-22 15:55:20.687122) Review request for hbase. Changes ------- Fixed two typos pointed out by Ted. TimeRangeTracker is not updated because it is a heuristic that records the (min, max) of the timestamps of the kvs in the memstore. We cannot update it during the rollback because we do not know what the next min or max is without scanning all the existing keys in the memstore. From what I understand, the timeRangeTracker is essentially a heuristic, so it is ok to not update it in the rare case of a rollback. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs (updated) /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 1187667 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1187667 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1187667 Diff: https://reviews.apache.org/r/2141/diff Testing ------- I ran TestLogRolling over and over again, about 50 times, not failed a single time. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2776
        -----------------------------------------------------------

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment6227>

        Should read 'Finished waiting for RWCC, ...'

        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
        <https://reviews.apache.org/r/2141/#comment6225>

        Should read 'Remove a key'

        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
        <https://reviews.apache.org/r/2141/#comment6226>

        Either put 'It is ok ...' on the next line or remove asterisk before 'It'.

        More importantly, I think timeRangeTracker should be updated because the write path changes timeRangeTracker and kvset at the same time.

        • Ted

        On 2011-10-22 07:53:42, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-22 07:53:42)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1187667

        /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1187667

        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1187667

        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1187667

        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1187667

        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1187667

        /src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 1187667

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1187667

        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1187667

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        I ran TestLogRolling over and over again, about 50 times, not failed a single time.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2776 ----------------------------------------------------------- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment6227 > Should read 'Finished waiting for RWCC, ...' /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java < https://reviews.apache.org/r/2141/#comment6225 > Should read 'Remove a key' /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java < https://reviews.apache.org/r/2141/#comment6226 > Either put 'It is ok ...' on the next line or remove asterisk before 'It'. More importantly, I think timeRangeTracker should be updated because the write path changes timeRangeTracker and kvset at the same time. Ted On 2011-10-22 07:53:42, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-22 07:53:42) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 1187667 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1187667 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1187667 Diff: https://reviews.apache.org/r/2141/diff Testing ------- I ran TestLogRolling over and over again, about 50 times, not failed a single time. Thanks, Dhruba
        Hide
        ramkrishna.s.vasudevan added a comment -

        Thanks Dhruba for addressing the comments. Syncing will happen in 2 places, correctness is not a problem but sync operation will happen more than once for the same data.
        +1 for patch.

        Show
        ramkrishna.s.vasudevan added a comment - Thanks Dhruba for addressing the comments. Syncing will happen in 2 places, correctness is not a problem but sync operation will happen more than once for the same data. +1 for patch.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/
        -----------------------------------------------------------

        (Updated 2011-10-22 07:53:42.308465)

        Review request for hbase.

        Changes
        -------

        Addressed feedback comments from RamKrishna and Jonathan. Fixed unit test to not assert erroneously. Enhanced Memstore.rollback() to rollback keys from memstore and snapshot.

        Summary
        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        This addresses bug HBASE-4528.
        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs (updated)


        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1187667
        /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1187667
        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1187667
        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1187667
        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1187667
        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1187667
        /src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 1187667
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1187667
        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1187667

        Diff: https://reviews.apache.org/r/2141/diff

        Testing
        -------

        I ran TestLogRolling over and over again, about 50 times, not failed a single time.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-22 07:53:42.308465) Review request for hbase. Changes ------- Addressed feedback comments from RamKrishna and Jonathan. Fixed unit test to not assert erroneously. Enhanced Memstore.rollback() to rollback keys from memstore and snapshot. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs (updated) /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1187667 /src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 1187667 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1187667 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1187667 Diff: https://reviews.apache.org/r/2141/diff Testing ------- I ran TestLogRolling over and over again, about 50 times, not failed a single time. Thanks, Dhruba
        Hide
        dhruba borthakur added a comment -

        Addressed feedback comments from RamKrishna and Jonathan. Fixed unit test to not assert erroneously. Enhanced Memstore.rollback() to rollback keys from memstore and snapshot.

        Show
        dhruba borthakur added a comment - Addressed feedback comments from RamKrishna and Jonathan. Fixed unit test to not assert erroneously. Enhanced Memstore.rollback() to rollback keys from memstore and snapshot.
        Hide
        dhruba borthakur added a comment -

        Hi Ramkrisham I agree that the sync can occur from two places: from the rpc handler thread or the background LogSyncer thread. That should not impact correctness, isn't it true?

        I will upload a new patch that addresses your comments about this patch in reviewboard.

        Show
        dhruba borthakur added a comment - Hi Ramkrisham I agree that the sync can occur from two places: from the rpc handler thread or the background LogSyncer thread. That should not impact correctness, isn't it true? I will upload a new patch that addresses your comments about this patch in reviewboard.
        Hide
        ramkrishna.s.vasudevan added a comment -

        @Dhruba
        I found one more thing using the testcase TestParallelPut.java. If the LogSyncer thread starts running first time the unflushedEntries will be 0 and syncedTillHere will also be 0.
        Now first put will make this unFlushedEntries to be 1. Now the LogSyncer thread will check for the condition

           try {
                    if (unflushedEntries.get() <= syncedTillHere) {
                      Thread.sleep(this.optionalFlushInterval);
                    }
                    sync();
                  }
        

        now there is a chance that just after the append is completed to HLog before the lock gets released the entry may be synced because of LogFlusher Thread. Is it ok ? Pls correct me if am wrong. I think it should not be a problem but syncing will happen in 2 different places.

        Show
        ramkrishna.s.vasudevan added a comment - @Dhruba I found one more thing using the testcase TestParallelPut.java. If the LogSyncer thread starts running first time the unflushedEntries will be 0 and syncedTillHere will also be 0. Now first put will make this unFlushedEntries to be 1. Now the LogSyncer thread will check for the condition try { if (unflushedEntries.get() <= syncedTillHere) { Thread .sleep( this .optionalFlushInterval); } sync(); } now there is a chance that just after the append is completed to HLog before the lock gets released the entry may be synced because of LogFlusher Thread. Is it ok ? Pls correct me if am wrong. I think it should not be a problem but syncing will happen in 2 different places.
        Hide
        stack added a comment -

        @Dhruba What about Rams' postulate above? Good stuff.

        Show
        stack added a comment - @Dhruba What about Rams' postulate above? Good stuff.
        Hide
        Ted Yu added a comment -

        Minor comment, line 1184:

           LOG.debug("Finished snapshotting, commencing flushing stores");
        

        should read:

           LOG.debug("Finished waiting for rwcc, commencing flushing stores");
        
        Show
        Ted Yu added a comment - Minor comment, line 1184: LOG.debug( "Finished snapshotting, commencing flushing stores" ); should read: LOG.debug( "Finished waiting for rwcc, commencing flushing stores" );
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2652
        -----------------------------------------------------------

        Ship it!

        +1 for commit to trunk.

        Great work, Dhruba. Really nice comments explaining the changes too.

        A few minor nits, mostly docs and stuff. My major issue is around a better test of some of the potential race conditions. Let's file a follow-up JIRA to deal with that. I'm good with committing now as this is only going to trunk and there's plenty of time to continue making improvements and better tests. We'll be brining this to our 92 branch so it'll get cluster testing as well.

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5963>

        it's not before we commit, it's before we even begin iterating memstore / writing the storefiles, right? (a distinction we discussed yesterday)

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5964>

        nice fix

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5965>

        this is an awesome comment

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5966>

        This is not really Write to WAL? This is just building the WALEdit?

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5967>

        good

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5968>

        nit, whitespace

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5969>

        add something like ", or null to create and complete an rwcc transaction within the call?

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5970>

        the description seems a little vague. this is specifically for exceptions during log syncing when using early-lock-release? or does this / will this potentially cover other failures? also, some whitespace in this method.

        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
        <https://reviews.apache.org/r/2141/#comment5974>

        TODO: optimize (not a big deal because this is rarely used but you can use tailMaps/iterators and such to make this more efficient, especially if the set of input KVs are sorted)

        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
        <https://reviews.apache.org/r/2141/#comment5975>

        rollbacks only happen on the MemStore? never the snapshot? are there race conditions here?

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
        <https://reviews.apache.org/r/2141/#comment5976>

        should we be flushing concurrently? and doing other nasty things like mocking an exception out of the sync and verifying the rollback? i think a majority of the change is easy enough to verify through code analysis, and i think it's fine to commit this change as-is, but i'm worried about some kind of race between snapshotting, waiting for the read point to advance, an hlog exception, and rollback of the memstore. need to ensure rwcc still moves forward, need to ensure edits don't make it to hlog / get replayed, if post-snapshot there are new edits going to memstore and a new hlog, that the right thing happens w.r.t. that hlog sync either succeeding or failing again.

        i'm cool with filing a follow-up JIRA about making this test more thorough.

        • Jonathan

        On 2011-10-18 07:04:47, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-18 07:04:47)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1185500

        /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1185500

        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1185500

        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1185500

        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1185500

        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1185500

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1185500

        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1185500

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        I ran TestLogRolling over and over again, about 50 times, not failed a single time.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2652 ----------------------------------------------------------- Ship it! +1 for commit to trunk. Great work, Dhruba. Really nice comments explaining the changes too. A few minor nits, mostly docs and stuff. My major issue is around a better test of some of the potential race conditions. Let's file a follow-up JIRA to deal with that. I'm good with committing now as this is only going to trunk and there's plenty of time to continue making improvements and better tests. We'll be brining this to our 92 branch so it'll get cluster testing as well. /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5963 > it's not before we commit, it's before we even begin iterating memstore / writing the storefiles, right? (a distinction we discussed yesterday) /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5964 > nice fix /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5965 > this is an awesome comment /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5966 > This is not really Write to WAL? This is just building the WALEdit? /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5967 > good /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5968 > nit, whitespace /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5969 > add something like ", or null to create and complete an rwcc transaction within the call? /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5970 > the description seems a little vague. this is specifically for exceptions during log syncing when using early-lock-release? or does this / will this potentially cover other failures? also, some whitespace in this method. /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java < https://reviews.apache.org/r/2141/#comment5974 > TODO: optimize (not a big deal because this is rarely used but you can use tailMaps/iterators and such to make this more efficient, especially if the set of input KVs are sorted) /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java < https://reviews.apache.org/r/2141/#comment5975 > rollbacks only happen on the MemStore? never the snapshot? are there race conditions here? /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java < https://reviews.apache.org/r/2141/#comment5976 > should we be flushing concurrently? and doing other nasty things like mocking an exception out of the sync and verifying the rollback? i think a majority of the change is easy enough to verify through code analysis, and i think it's fine to commit this change as-is, but i'm worried about some kind of race between snapshotting, waiting for the read point to advance, an hlog exception, and rollback of the memstore. need to ensure rwcc still moves forward, need to ensure edits don't make it to hlog / get replayed, if post-snapshot there are new edits going to memstore and a new hlog, that the right thing happens w.r.t. that hlog sync either succeeding or failing again. i'm cool with filing a follow-up JIRA about making this test more thorough. Jonathan On 2011-10-18 07:04:47, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-18 07:04:47) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1185500 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1185500 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1185500 Diff: https://reviews.apache.org/r/2141/diff Testing ------- I ran TestLogRolling over and over again, about 50 times, not failed a single time. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2655
        -----------------------------------------------------------

        Ship it!

        +1

        This version looks good to me, assuming all tests pass.

        • Gary

        On 2011-10-18 07:04:47, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-18 07:04:47)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1185500

        /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1185500

        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1185500

        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1185500

        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1185500

        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1185500

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1185500

        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1185500

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        I ran TestLogRolling over and over again, about 50 times, not failed a single time.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2655 ----------------------------------------------------------- Ship it! +1 This version looks good to me, assuming all tests pass. Gary On 2011-10-18 07:04:47, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-18 07:04:47) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1185500 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1185500 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1185500 Diff: https://reviews.apache.org/r/2141/diff Testing ------- I ran TestLogRolling over and over again, about 50 times, not failed a single time. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2646
        -----------------------------------------------------------

        Now as per the current logic if there are 2 syncs happening parallely
        If there are 2 thread trying to sync
        Thread A has txid 1 and thread B has txid 2.
        Now if first B completes sync then the syncedTillHere will be 2 and later if A completes sync he will update as 1.
        So now LogSyncer will see this condition and he will again try to sync

        if (unflushedEntries.get() <= syncedTillHere) {
        Because now unflushedEntries will be 2. This is not a bug may be an additional sync call.

        I may be wrong. You can ignore if this comment is not valid. Thanks Dhruba.

        Minor comments
        ==============
        After the first testPut() gets executed the very first sync as part of testParallelPuts() will not get invoked as already the this.syncedTillHere is 1.
        assertEquals(0, ret.length); -> This should be assertEquals(1, ret.length); as we get some return value here.

        • ramkrishna

        On 2011-10-18 07:04:47, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-18 07:04:47)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1185500

        /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1185500

        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1185500

        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1185500

        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1185500

        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1185500

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1185500

        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1185500

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        I ran TestLogRolling over and over again, about 50 times, not failed a single time.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2646 ----------------------------------------------------------- Now as per the current logic if there are 2 syncs happening parallely If there are 2 thread trying to sync Thread A has txid 1 and thread B has txid 2. Now if first B completes sync then the syncedTillHere will be 2 and later if A completes sync he will update as 1. So now LogSyncer will see this condition and he will again try to sync if (unflushedEntries.get() <= syncedTillHere) { Because now unflushedEntries will be 2. This is not a bug may be an additional sync call. I may be wrong. You can ignore if this comment is not valid. Thanks Dhruba. Minor comments ============== After the first testPut() gets executed the very first sync as part of testParallelPuts() will not get invoked as already the this.syncedTillHere is 1. assertEquals(0, ret.length); -> This should be assertEquals(1, ret.length); as we get some return value here. ramkrishna On 2011-10-18 07:04:47, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-18 07:04:47) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1185500 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1185500 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1185500 Diff: https://reviews.apache.org/r/2141/diff Testing ------- I ran TestLogRolling over and over again, about 50 times, not failed a single time. Thanks, Dhruba
        Hide
        Ted Yu added a comment -

        +1 if tests pass.

        Show
        Ted Yu added a comment - +1 if tests pass.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/
        -----------------------------------------------------------

        (Updated 2011-10-18 07:04:47.525284)

        Review request for hbase.

        Changes
        -------

        There is no reason to reacquire the rowlock in HRegion.rollabackMemstore.

        Summary
        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        This addresses bug HBASE-4528.
        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs (updated)


        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1185500
        /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1185500
        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1185500
        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1185500
        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1185500
        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1185500
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1185500
        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1185500

        Diff: https://reviews.apache.org/r/2141/diff

        Testing
        -------

        I ran TestLogRolling over and over again, about 50 times, not failed a single time.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-18 07:04:47.525284) Review request for hbase. Changes ------- There is no reason to reacquire the rowlock in HRegion.rollabackMemstore. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs (updated) /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1185500 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1185500 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1185500 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1185500 Diff: https://reviews.apache.org/r/2141/diff Testing ------- I ran TestLogRolling over and over again, about 50 times, not failed a single time. Thanks, Dhruba
        Hide
        dhruba borthakur added a comment -

        After some discussion with Ted and JGray, we decided that the rowlock is not necessary for HRegion.rollbackMemstore. This is the version of the patch that should satisfy all parties. Please review.

        Show
        dhruba borthakur added a comment - After some discussion with Ted and JGray, we decided that the rowlock is not necessary for HRegion.rollbackMemstore. This is the version of the patch that should satisfy all parties. Please review.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-10-17 08:46:53, ramkrishna vasudevan wrote:

        > @Dhruba

        > Any figures to know the improvement in write performance?

        I had updated the performance numbers in the JIRA comments. Please let me know if they are adequate.

        • Dhruba

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2617
        -----------------------------------------------------------

        On 2011-10-17 04:39:55, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-17 04:39:55)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1184991

        /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1184991

        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1184991

        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1184991

        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1184991

        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1184991

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1184991

        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1184991

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        I ran TestLogRolling over and over again, about 50 times, not failed a single time.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-10-17 08:46:53, ramkrishna vasudevan wrote: > @Dhruba > Any figures to know the improvement in write performance? I had updated the performance numbers in the JIRA comments. Please let me know if they are adequate. Dhruba ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2617 ----------------------------------------------------------- On 2011-10-17 04:39:55, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-17 04:39:55) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1184991 /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1184991 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1184991 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1184991 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1184991 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1184991 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1184991 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1184991 Diff: https://reviews.apache.org/r/2141/diff Testing ------- I ran TestLogRolling over and over again, about 50 times, not failed a single time. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2617
        -----------------------------------------------------------

        @Dhruba
        Any figures to know the improvement in write performance?

        • ramkrishna

        On 2011-10-17 04:39:55, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-17 04:39:55)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1184991

        /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1184991

        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1184991

        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1184991

        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1184991

        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1184991

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1184991

        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1184991

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        I ran TestLogRolling over and over again, about 50 times, not failed a single time.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2617 ----------------------------------------------------------- @Dhruba Any figures to know the improvement in write performance? ramkrishna On 2011-10-17 04:39:55, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-17 04:39:55) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1184991 /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1184991 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1184991 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1184991 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1184991 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1184991 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1184991 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1184991 Diff: https://reviews.apache.org/r/2141/diff Testing ------- I ran TestLogRolling over and over again, about 50 times, not failed a single time. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/
        -----------------------------------------------------------

        (Updated 2011-10-17 04:39:55.174101)

        Review request for hbase.

        Changes
        -------

        Fixed typos.

        Summary
        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        This addresses bug HBASE-4528.
        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs (updated)


        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1184991
        /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1184991
        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1184991
        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1184991
        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1184991
        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1184991
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1184991
        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1184991

        Diff: https://reviews.apache.org/r/2141/diff

        Testing
        -------

        I ran TestLogRolling over and over again, about 50 times, not failed a single time.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-17 04:39:55.174101) Review request for hbase. Changes ------- Fixed typos. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs (updated) /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1184991 /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1184991 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1184991 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1184991 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1184991 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1184991 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1184991 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1184991 Diff: https://reviews.apache.org/r/2141/diff Testing ------- I ran TestLogRolling over and over again, about 50 times, not failed a single time. Thanks, Dhruba
        Hide
        dhruba borthakur added a comment -

        Fixed typos.

        Performance numbers return on hbase-92 with a variant of hdfs 0.20.

        vanilla hdfs : 1200 put/sec (no patch),
        5000 puts/sec (with patch)
        synconsync hdfs : 80 put/sec (no patch)

        The synconsync-version-of-hdfs is an internal version of hdfs that makes the datanode issue a sync() on the corresponding ext3 block file for every invocation of DFSClient.sync(). This ensures that a hbase transaction is really,really on disk before the put rpc returns to the client.

        Show
        dhruba borthakur added a comment - Fixed typos. Performance numbers return on hbase-92 with a variant of hdfs 0.20. vanilla hdfs : 1200 put/sec (no patch), 5000 puts/sec (with patch) synconsync hdfs : 80 put/sec (no patch) The synconsync-version-of-hdfs is an internal version of hdfs that makes the datanode issue a sync() on the corresponding ext3 block file for every invocation of DFSClient.sync(). This ensures that a hbase transaction is really,really on disk before the put rpc returns to the client.
        Hide
        jiraposter@reviews.apache.org added a comment -

        On 2011-10-15 11:55:54, Ted Yu wrote:

        > We're closer.

        > Thanks for the perseverance, Dhruba.

        I will post another version of this patch with some typos corrected.

        On 2011-10-15 11:55:54, Ted Yu wrote:

        > /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1884

        > <https://reviews.apache.org/r/2141/diff/5/?file=50446#file50446line1884>

        >

        > We know that w != null here, so w.getWriteNumber() should be passed to rollbackMemstore().

        There is no need to explicitly pass in w.getWriteNumber(). All the keys that are hanging off the familyMaps variable have their memstoreTS set appropriately. These were set in the call to applyFamilyMapToMemstore(). This memstoreTS will be used in the rollback methods to ensure that only keys in the memstore that also have a matching memstoreTS value are removed.

        On 2011-10-15 11:55:54, Ted Yu wrote:

        > /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 2195

        > <https://reviews.apache.org/r/2141/diff/5/?file=50446#file50446line2195>

        >

        > We should have memstoreTS parameter here.

        No need to pass in memstoreTS. The kvs hanging off the parameter 'familyMaps' already have the memstoreTS that was used to insert these keys in the memstore.

        On 2011-10-15 11:55:54, Ted Yu wrote:

        > /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 2228

        > <https://reviews.apache.org/r/2141/diff/5/?file=50446#file50446line2228>

        >

        > I think this should be in a finally block corresponding to the try at line 2205.

        I do not think a finally block is needed. If the getlock itself threw an exception, then there is no reason to do a releaseLock. Nothing else in this code section can throw an exception.

        • Dhruba

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2611
        -----------------------------------------------------------

        On 2011-10-15 07:32:28, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-15 07:32:28)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1183585

        /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1183585

        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1183585

        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1183585

        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1183585

        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1183585

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1183585

        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1183585

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        I ran TestLogRolling over and over again, about 50 times, not failed a single time.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - On 2011-10-15 11:55:54, Ted Yu wrote: > We're closer. > Thanks for the perseverance, Dhruba. I will post another version of this patch with some typos corrected. On 2011-10-15 11:55:54, Ted Yu wrote: > /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 1884 > < https://reviews.apache.org/r/2141/diff/5/?file=50446#file50446line1884 > > > We know that w != null here, so w.getWriteNumber() should be passed to rollbackMemstore(). There is no need to explicitly pass in w.getWriteNumber(). All the keys that are hanging off the familyMaps variable have their memstoreTS set appropriately. These were set in the call to applyFamilyMapToMemstore(). This memstoreTS will be used in the rollback methods to ensure that only keys in the memstore that also have a matching memstoreTS value are removed. On 2011-10-15 11:55:54, Ted Yu wrote: > /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 2195 > < https://reviews.apache.org/r/2141/diff/5/?file=50446#file50446line2195 > > > We should have memstoreTS parameter here. No need to pass in memstoreTS. The kvs hanging off the parameter 'familyMaps' already have the memstoreTS that was used to insert these keys in the memstore. On 2011-10-15 11:55:54, Ted Yu wrote: > /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java, line 2228 > < https://reviews.apache.org/r/2141/diff/5/?file=50446#file50446line2228 > > > I think this should be in a finally block corresponding to the try at line 2205. I do not think a finally block is needed. If the getlock itself threw an exception, then there is no reason to do a releaseLock. Nothing else in this code section can throw an exception. Dhruba ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2611 ----------------------------------------------------------- On 2011-10-15 07:32:28, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-15 07:32:28) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1183585 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1183585 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1183585 Diff: https://reviews.apache.org/r/2141/diff Testing ------- I ran TestLogRolling over and over again, about 50 times, not failed a single time. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2611
        -----------------------------------------------------------

        We're closer.
        Thanks for the perseverance, Dhruba.

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5900>

        We know that w != null here, so w.getWriteNumber() should be passed to rollbackMemstore().

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5901>

        We should have memstoreTS parameter here.

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5903>

        Typo: succeeded

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5904>

        Interesting, for next round of discussions.
        I think we should take more preventive action here.

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5898>

        Should read 'keyvals from memstore whose timestamp matches'

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5902>

        our memstoreTS should be checked against that of kv's.

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5899>

        I think this should be in a finally block corresponding to the try at line 2205.

        • Ted

        On 2011-10-15 07:32:28, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-15 07:32:28)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1183585

        /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1183585

        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1183585

        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1183585

        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1183585

        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1183585

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1183585

        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1183585

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        I ran TestLogRolling over and over again, about 50 times, not failed a single time.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2611 ----------------------------------------------------------- We're closer. Thanks for the perseverance, Dhruba. /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5900 > We know that w != null here, so w.getWriteNumber() should be passed to rollbackMemstore(). /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5901 > We should have memstoreTS parameter here. /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5903 > Typo: succeeded /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5904 > Interesting, for next round of discussions. I think we should take more preventive action here. /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5898 > Should read 'keyvals from memstore whose timestamp matches' /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5902 > our memstoreTS should be checked against that of kv's. /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5899 > I think this should be in a finally block corresponding to the try at line 2205. Ted On 2011-10-15 07:32:28, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-15 07:32:28) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1183585 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1183585 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1183585 Diff: https://reviews.apache.org/r/2141/diff Testing ------- I ran TestLogRolling over and over again, about 50 times, not failed a single time. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/
        -----------------------------------------------------------

        (Updated 2011-10-15 07:32:28.011678)

        Review request for hbase.

        Summary
        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        This addresses bug HBASE-4528.
        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs


        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1183585
        /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1183585
        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1183585
        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1183585
        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1183585
        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1183585
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1183585
        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1183585

        Diff: https://reviews.apache.org/r/2141/diff

        Testing (updated)
        -------

        I ran TestLogRolling over and over again, about 50 times, not failed a single time.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-15 07:32:28.011678) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1183585 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1183585 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1183585 Diff: https://reviews.apache.org/r/2141/diff Testing (updated) ------- I ran TestLogRolling over and over again, about 50 times, not failed a single time. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/
        -----------------------------------------------------------

        (Updated 2011-10-15 07:31:28.693947)

        Review request for hbase.

        Summary (updated)
        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        This addresses bug HBASE-4528.
        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs


        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1183585
        /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1183585
        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1183585
        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1183585
        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1183585
        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1183585
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1183585
        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1183585

        Diff: https://reviews.apache.org/r/2141/diff

        Testing
        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-15 07:31:28.693947) Review request for hbase. Summary (updated) ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1183585 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1183585 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1183585 Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/
        -----------------------------------------------------------

        (Updated 2011-10-15 07:31:06.389199)

        Review request for hbase.

        Changes
        -------

        Addressed Kannans, ted and Gary review comments. Changed name of method to rollbackMemstore. And the rollback method now compare memstoreTS before deleting the key.

        Summary
        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.
        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs (updated)


        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1183585
        /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1183585
        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1183585
        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1183585
        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1183585
        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1183585
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1183585
        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1183585

        Diff: https://reviews.apache.org/r/2141/diff

        Testing
        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-15 07:31:06.389199) Review request for hbase. Changes ------- Addressed Kannans, ted and Gary review comments. Changed name of method to rollbackMemstore. And the rollback method now compare memstoreTS before deleting the key. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs (updated) /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueSkipListSet.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1183585 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1183585 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1183585 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1183585 Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        dhruba borthakur added a comment -

        Addressed Kannans, ted and Gary review comments. Changed name of method to rollbackMemstore. And the rollback method now compare memstoreTS before deleting the key.

        Show
        dhruba borthakur added a comment - Addressed Kannans, ted and Gary review comments. Changed name of method to rollbackMemstore. And the rollback method now compare memstoreTS before deleting the key.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2488
        -----------------------------------------------------------

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5629>

        Isn't the memstore "snapshot" taken here (in flusher.prepare())? And if so, what prevents new transactions from sneaking into the memstore between line 1144 (where you take the rwcc point) and this one?

        Maybe lines 1143-1144 be moved inside the writeLock().lock()?

        • Kannan

        On 2011-10-08 07:50:25, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-08 07:50:25)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1180314

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1180314

        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1180314

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2488 ----------------------------------------------------------- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5629 > Isn't the memstore "snapshot" taken here (in flusher.prepare())? And if so, what prevents new transactions from sneaking into the memstore between line 1144 (where you take the rwcc point) and this one? Maybe lines 1143-1144 be moved inside the writeLock().lock()? Kannan On 2011-10-08 07:50:25, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-08 07:50:25) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1180314 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1180314 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1180314 Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2481
        -----------------------------------------------------------

        This version looks good to me, with the exception of the need for a memstoreTS check when removing the memstore entries.

        I did a couple runs of TestLogRolling with this version and it passes.

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5622>

        Agree with Ted that we should check memstoreTS against w.getWriteNumber() when removing, since this isn't a part of KeyValue.equals().

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5617>

        I think this line is still needed to ensure the WriteEntry is removed from the RWCC write queue in the case of a rollback.

        • Gary

        On 2011-10-08 07:50:25, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-08 07:50:25)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1180314

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1180314

        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1180314

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2481 ----------------------------------------------------------- This version looks good to me, with the exception of the need for a memstoreTS check when removing the memstore entries. I did a couple runs of TestLogRolling with this version and it passes. /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5622 > Agree with Ted that we should check memstoreTS against w.getWriteNumber() when removing, since this isn't a part of KeyValue.equals(). /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5617 > I think this line is still needed to ensure the WriteEntry is removed from the RWCC write queue in the case of a rollback. Gary On 2011-10-08 07:50:25, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-08 07:50:25) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1180314 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1180314 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1180314 Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2467
        -----------------------------------------------------------

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5575>

        If walSyncSuccessful is true, rwcc.completeMemstoreInsert(w) on line 1852 would have been called.
        If walSyncSuccessful is false, removeKeysFromMemstore() on line 1878 rolls back the changes to memstore.
        Looks like this line is no longer needed.

        • Ted

        On 2011-10-08 07:50:25, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-08 07:50:25)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1180314

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1180314

        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1180314

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2467 ----------------------------------------------------------- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5575 > If walSyncSuccessful is true, rwcc.completeMemstoreInsert(w) on line 1852 would have been called. If walSyncSuccessful is false, removeKeysFromMemstore() on line 1878 rolls back the changes to memstore. Looks like this line is no longer needed. Ted On 2011-10-08 07:50:25, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-08 07:50:25) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1180314 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1180314 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1180314 Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        Ted Yu added a comment -

        Minor suggestion for the new MemStore.remove() method:
        Since it is called only in case of error recovery, I feel a better name maybe rollback().

        Show
        Ted Yu added a comment - Minor suggestion for the new MemStore.remove() method: Since it is called only in case of error recovery, I feel a better name maybe rollback().
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2464
        -----------------------------------------------------------

        +1

        • ramkrishna

        On 2011-10-08 07:50:25, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-08 07:50:25)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1180314

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1180314

        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1180314

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2464 ----------------------------------------------------------- +1 ramkrishna On 2011-10-08 07:50:25, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-08 07:50:25) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1180314 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1180314 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1180314 Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2463
        -----------------------------------------------------------

        Nice work.
        See comments below which aim to solve the case Dhruba raised @ 08/Oct/11 08:01

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5573>

        We should pass w.getWriteNumber() to this method and use it to verify the memstoreTS of kv's matches the writeNumber.
        w wouldn't be null in this case since step 8 shouldn't have been executed yet.

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5569>

        This line should be moved into an else block for the if block above.

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5572>

        We should verify that kv.getMemstoreTS() matches w.getWriteNumber() before removing from store.

        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
        <https://reviews.apache.org/r/2141/#comment5570>

        Should be 'remove a KeyValue'

        • Ted

        On 2011-10-08 07:50:25, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-08 07:50:25)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1180314

        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1180314

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1180314

        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1180314

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2463 ----------------------------------------------------------- Nice work. See comments below which aim to solve the case Dhruba raised @ 08/Oct/11 08:01 /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5573 > We should pass w.getWriteNumber() to this method and use it to verify the memstoreTS of kv's matches the writeNumber. w wouldn't be null in this case since step 8 shouldn't have been executed yet. /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5569 > This line should be moved into an else block for the if block above. /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5572 > We should verify that kv.getMemstoreTS() matches w.getWriteNumber() before removing from store. /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java < https://reviews.apache.org/r/2141/#comment5570 > Should be 'remove a KeyValue' Ted On 2011-10-08 07:50:25, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-08 07:50:25) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1180314 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1180314 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1180314 Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        dhruba borthakur added a comment -

        It appears to me that deleting kvs from the memstore (as part of the transaction rollback) should be ok. It is done without the rowlock because the kvs are not yet visible to scanners because the rwcc is not yet advanced.

        One doubtful case is when a thread A is trying to insert kv via a Put call. Thread A inserted into memstore but has failed to sync to wal. Now suppose another thread B has concurrently inserted exactly the same kv into the memstore successfully and committed the transaction by syncing to wal successfully. Now thread A tries to rollback its failed transaction and removes the kv from the memstore. Is this scenario possible? In that case, should the rollback code in Thread A delete the kv from the memstore only if its kv.memstoreTS matches its own?

        Show
        dhruba borthakur added a comment - It appears to me that deleting kvs from the memstore (as part of the transaction rollback) should be ok. It is done without the rowlock because the kvs are not yet visible to scanners because the rwcc is not yet advanced. One doubtful case is when a thread A is trying to insert kv via a Put call. Thread A inserted into memstore but has failed to sync to wal. Now suppose another thread B has concurrently inserted exactly the same kv into the memstore successfully and committed the transaction by syncing to wal successfully. Now thread A tries to rollback its failed transaction and removes the kv from the memstore. Is this scenario possible? In that case, should the rollback code in Thread A delete the kv from the memstore only if its kv.memstoreTS matches its own?
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/
        -----------------------------------------------------------

        (Updated 2011-10-08 07:50:25.069515)

        Review request for hbase.

        Changes
        -------

        The HLog.sync() does not throw an exception if it encounters a HDFS error. Instead it triggers a logroll as usual. If the put code encounter an error while syncing to hdfs, then it rollbacks the change to the memstore and throws an exception to the client.

        Summary
        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.
        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs (updated)


        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1180314
        /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1180314
        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1180314
        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1180314
        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1180314
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1180314
        /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1180314

        Diff: https://reviews.apache.org/r/2141/diff

        Testing
        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-08 07:50:25.069515) Review request for hbase. Changes ------- The HLog.sync() does not throw an exception if it encounters a HDFS error. Instead it triggers a logroll as usual. If the put code encounter an error while syncing to hdfs, then it rollbacks the change to the memstore and throws an exception to the client. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs (updated) /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1180314 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1180314 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1180314 /src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestLogRolling.java 1180314 Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        dhruba borthakur added a comment -

        Addressed Gary's concern. The HLog.sync() does not throw an exception if it encounters a HDFS error. Instead it triggers a logroll as usual.

        If the put code encounter an error while syncing to hdfs, then it rollbacks the change to the memstore and throws an exception to the client.

        Kannan: I will rerun my performance benchmark and report results.

        Show
        dhruba borthakur added a comment - Addressed Gary's concern. The HLog.sync() does not throw an exception if it encounters a HDFS error. Instead it triggers a logroll as usual. If the put code encounter an error while syncing to hdfs, then it rollbacks the change to the memstore and throws an exception to the client. Kannan: I will rerun my performance benchmark and report results.
        Hide
        Kannan Muthukkaruppan added a comment -

        Dhruba: You wrote: <<<A single row update improves from 100 puts/sec/server to 5000 puts/sec/server.>>>.

        Can you reconfirm the above? On hbase-89 based test, I am seeing "single row" update do ~1000 puts/sec/server.

        Show
        Kannan Muthukkaruppan added a comment - Dhruba: You wrote: <<<A single row update improves from 100 puts/sec/server to 5000 puts/sec/server.>>>. Can you reconfirm the above? On hbase-89 based test, I am seeing "single row" update do ~1000 puts/sec/server.
        Hide
        Jonathan Gray added a comment -

        Dhruba and I just talked about this. I also like the MemStore rollback. It should not be that difficult, just removing the List<KV> that we added.

        Show
        Jonathan Gray added a comment - Dhruba and I just talked about this. I also like the MemStore rollback. It should not be that difficult, just removing the List<KV> that we added.
        Hide
        Ted Yu added a comment -

        I was thinking about the possibility of memstore rollback as well.
        Here're the operations in applyFamilyMapToMemstore() whose effect needs to be rolled back:

                for (KeyValue kv: edits) {
                  kv.setMemstoreTS(w.getWriteNumber());
                  size += store.add(kv);
                }
        
        Show
        Ted Yu added a comment - I was thinking about the possibility of memstore rollback as well. Here're the operations in applyFamilyMapToMemstore() whose effect needs to be rolled back: for (KeyValue kv: edits) { kv.setMemstoreTS(w.getWriteNumber()); size += store.add(kv); }
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2397
        -----------------------------------------------------------

        Overall I see this patch as trading off service resiliency in favor of performance.

        With the current ordering of operations (WAL append and sync prior to memstore insert), we ensure that an error during sync is seen by the client and memstore consistency is maintained. Importantly (at least for my goals), this also allows us to do some reasoning about when it's necessary to abort the region server or when we can take additional actions to try to ride over a transient error. As long as there were no deferred flush edits, we could reason that any error on sync was propagated back to the client as a failure and we did not need to abort yet. This is the direction I've been trying to move with HBASE-4222/4282 and a partial form of it was already in place prior to that.

        I understand why we want to reorder these operations and move the sync outside of the acquired row locks. From this standpoint, since an error on sync leaves the memstore polluted, aborting immediately is the right thing to do. But I don't think it's a desirable behavior. I think it will lead to more complaints from users about observed instability of the system.

        The use-case that motivated HBASE-4222 was performing a rolling restart of all DataNodes in a cluster, with a running, but completely quiescent HBase cluster. In this case, with no data durability at stake, we really should be able to recover. But instead what will happen is a catastrophic failure of RegionServers as each server tries to roll its HLog. The patch in it's current state would regress to this behavior, triggering RS aborts even more quickly than prior to HBASE-4222 (no HLog close would be attempted).

        I would really like to find a way to keep the performance optimization of moving the HLog sync outside of the row locks, while still being able to guarantee memstore consistency in the case of failure, so that we can still reason about whether or not a RS abort is really necessary.

        Speaking naively, is it at all feasible that the RWCC.WriteEntry could track the KeyValues instances it's used to apply to the memstore? And these references could then be used to attempt a memstore rollback on failure? Any other ways that we can maintain memstore consistency here without giving up and aborting?

        /src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java
        <https://reviews.apache.org/r/2141/#comment5501>

        Personally, I think this is a step in the wrong direction. I would like to see us be more resilient in the face of transient HDFS errors, as long as we have sufficient information to reason that we have not compromised correctness.

        • Gary

        On 2011-10-06 08:08:49, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-06 08:08:49)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1179529

        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1179529

        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1179529

        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1179529

        /src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 1179529

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1179529

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2397 ----------------------------------------------------------- Overall I see this patch as trading off service resiliency in favor of performance. With the current ordering of operations (WAL append and sync prior to memstore insert), we ensure that an error during sync is seen by the client and memstore consistency is maintained. Importantly (at least for my goals), this also allows us to do some reasoning about when it's necessary to abort the region server or when we can take additional actions to try to ride over a transient error. As long as there were no deferred flush edits, we could reason that any error on sync was propagated back to the client as a failure and we did not need to abort yet. This is the direction I've been trying to move with HBASE-4222 /4282 and a partial form of it was already in place prior to that. I understand why we want to reorder these operations and move the sync outside of the acquired row locks. From this standpoint, since an error on sync leaves the memstore polluted, aborting immediately is the right thing to do. But I don't think it's a desirable behavior. I think it will lead to more complaints from users about observed instability of the system. The use-case that motivated HBASE-4222 was performing a rolling restart of all DataNodes in a cluster, with a running, but completely quiescent HBase cluster. In this case, with no data durability at stake, we really should be able to recover. But instead what will happen is a catastrophic failure of RegionServers as each server tries to roll its HLog. The patch in it's current state would regress to this behavior, triggering RS aborts even more quickly than prior to HBASE-4222 (no HLog close would be attempted). I would really like to find a way to keep the performance optimization of moving the HLog sync outside of the row locks, while still being able to guarantee memstore consistency in the case of failure, so that we can still reason about whether or not a RS abort is really necessary. Speaking naively, is it at all feasible that the RWCC.WriteEntry could track the KeyValues instances it's used to apply to the memstore? And these references could then be used to attempt a memstore rollback on failure? Any other ways that we can maintain memstore consistency here without giving up and aborting? /src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java < https://reviews.apache.org/r/2141/#comment5501 > Personally, I think this is a step in the wrong direction. I would like to see us be more resilient in the face of transient HDFS errors, as long as we have sufficient information to reason that we have not compromised correctness. Gary On 2011-10-06 08:08:49, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-06 08:08:49) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1179529 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1179529 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1179529 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1179529 /src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 1179529 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1179529 Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2390
        -----------------------------------------------------------

        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java
        <https://reviews.apache.org/r/2141/#comment5491>

        If advanceMemstore() returns true above, can we skip this call ?

        • Ted

        On 2011-10-06 08:08:49, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-06 08:08:49)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1179529

        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1179529

        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1179529

        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1179529

        /src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 1179529

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1179529

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2390 ----------------------------------------------------------- /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java < https://reviews.apache.org/r/2141/#comment5491 > If advanceMemstore() returns true above, can we skip this call ? Ted On 2011-10-06 08:08:49, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-06 08:08:49) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1179529 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1179529 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1179529 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1179529 /src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 1179529 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1179529 Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/
        -----------------------------------------------------------

        (Updated 2011-10-06 08:08:49.288861)

        Review request for hbase.

        Changes
        -------

        1. The flush of memstore waits for current transactions to quiesce before committing the flushed files. This should address the problem pointed out by Kannan.

        2. The Hlog.syncer() does not throw an exception, instead causes the regionserver to exit if it is unable to sync to hdfs. The assumption here is that if hbase is unable to write/sync to hdfs, then the simplest and correct error recovery is to exit. (For example, if the memstore flush fails, the regionserver exits)

        Summary
        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.
        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs (updated)


        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1179529
        /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1179529
        /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1179529
        /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1179529
        /src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 1179529
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1179529

        Diff: https://reviews.apache.org/r/2141/diff

        Testing
        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-06 08:08:49.288861) Review request for hbase. Changes ------- 1. The flush of memstore waits for current transactions to quiesce before committing the flushed files. This should address the problem pointed out by Kannan. 2. The Hlog.syncer() does not throw an exception, instead causes the regionserver to exit if it is unable to sync to hdfs. The assumption here is that if hbase is unable to write/sync to hdfs, then the simplest and correct error recovery is to exit. (For example, if the memstore flush fails, the regionserver exits) Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs (updated) /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1179529 /src/main/java/org/apache/hadoop/hbase/regionserver/ReadWriteConsistencyControl.java 1179529 /src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1179529 /src/main/java/org/apache/hadoop/hbase/regionserver/StoreFlusher.java 1179529 /src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 1179529 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION /src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java 1179529 Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        dhruba borthakur added a comment -

        1. The flush of memstore waits for current transactions to quiesce before committing the flushed files. This should address the problem pointed out by Kannan.

        2. The Hlog.syncer() does not throw an exception, instead causes the regionserver to exit if it is unable to sync to hdfs. The assumption here is that if hbase is unable to write/sync to hdfs, then the simplest and correct error recovery is to exit. (For example, if the memstore flush fails, the regionserver exits)

        Show
        dhruba borthakur added a comment - 1. The flush of memstore waits for current transactions to quiesce before committing the flushed files. This should address the problem pointed out by Kannan. 2. The Hlog.syncer() does not throw an exception, instead causes the regionserver to exit if it is unable to sync to hdfs. The assumption here is that if hbase is unable to write/sync to hdfs, then the simplest and correct error recovery is to exit. (For example, if the memstore flush fails, the regionserver exits)
        Hide
        dhruba borthakur added a comment -

        Hi Ted, if the HLog.sync() throws an exception, then it is not clear whether the transaction made it to the disk or not, but the changes to memstore are already made. So, it won't help to catch the exception from Hlog.sync() and continue; instead it is better to fail completely, do you agree?

        Kannan: thanks for pointing out the problem related to memstore flushes. I had a solution for this deployed internally, will upload a new patch that will include the fix to the problem. Essentially, the flush will wait for all currently running transactions to quiesce before committing the flush.

        Show
        dhruba borthakur added a comment - Hi Ted, if the HLog.sync() throws an exception, then it is not clear whether the transaction made it to the disk or not, but the changes to memstore are already made. So, it won't help to catch the exception from Hlog.sync() and continue; instead it is better to fail completely, do you agree? Kannan: thanks for pointing out the problem related to memstore flushes. I had a solution for this deployed internally, will upload a new patch that will include the fix to the problem. Essentially, the flush will wait for all currently running transactions to quiesce before committing the flush.
        Hide
        Ted Yu added a comment -

        For Dhruba's comment @ 06/Oct/11 04:09, the following call would prevent inconsistency Gary mentioned from happening:

                Runtime.getRuntime().halt(1);
        

        But this is not as amenable as guarding the following in the finally block against possible IOE (from sync()):

        if (w != null) rwcc.completeMemstoreInsert(w);
        
        Show
        Ted Yu added a comment - For Dhruba's comment @ 06/Oct/11 04:09, the following call would prevent inconsistency Gary mentioned from happening: Runtime .getRuntime().halt(1); But this is not as amenable as guarding the following in the finally block against possible IOE (from sync()): if (w != null ) rwcc.completeMemstoreInsert(w);
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2383
        -----------------------------------------------------------

        Along the lines of the concern Gary raises, because of insert into memstore happening before "sync" succeeding, I think another scenario can cause problems too:

        1) say, memstore insert happens to CF1 & CF2.
        2) then, memstore for CF1 gets flushed
        3) region server crashes before the sync is successful and before CF2 was flushed.

        Now, when the region is opened in a new region server, the region will contain a partial transaction (only CF1 changes will survive)-- and cross CF atomicity will be lost.

        • Kannan

        On 2011-10-03 05:54:07, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-03 05:54:07)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178298

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2383 ----------------------------------------------------------- Along the lines of the concern Gary raises, because of insert into memstore happening before "sync" succeeding, I think another scenario can cause problems too: 1) say, memstore insert happens to CF1 & CF2. 2) then, memstore for CF1 gets flushed 3) region server crashes before the sync is successful and before CF2 was flushed. Now, when the region is opened in a new region server, the region will contain a partial transaction (only CF1 changes will survive)-- and cross CF atomicity will be lost. Kannan On 2011-10-03 05:54:07, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-03 05:54:07) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178298 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        dhruba borthakur added a comment -

        I agree that if the HLog.sync() throws an exception (but does not abort the regiomnserver), then we have a serious problem in our hands.

        The existing trunk-code has the problem that if we are able to successfully update the WAL but insertion into the memstore resulted in an exception, then the client will be notified of the failed-update, but actually the transaction could come back to life if wal-recovery occurs.

        One proposal is to change the last few lines of HLog.syncer(long txid) to throw an Runtime exception instead of requesting a log roll.

        } catch (IOException e) {
              LOG.fatal("Could not sync. Earlier was Requesting close of hlog " +
                        " but instead we should bail out completely", e);
              Runtime.exit(1);
            }
        
        

        Do you think that this code is palatable? Do folks usually see this code path being executed in their clusters?

        Show
        dhruba borthakur added a comment - I agree that if the HLog.sync() throws an exception (but does not abort the regiomnserver), then we have a serious problem in our hands. The existing trunk-code has the problem that if we are able to successfully update the WAL but insertion into the memstore resulted in an exception, then the client will be notified of the failed-update, but actually the transaction could come back to life if wal-recovery occurs. One proposal is to change the last few lines of HLog.syncer(long txid) to throw an Runtime exception instead of requesting a log roll. } catch (IOException e) { LOG.fatal( "Could not sync. Earlier was Requesting close of hlog " + " but instead we should bail out completely" , e); Runtime .exit(1); } Do you think that this code is palatable? Do folks usually see this code path being executed in their clusters?
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2380
        -----------------------------------------------------------

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5474>

        We should check this.htableDescriptor.isDeferredLogFlush()

        • Ted

        On 2011-10-03 05:54:07, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-03 05:54:07)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178298

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2380 ----------------------------------------------------------- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5474 > We should check this.htableDescriptor.isDeferredLogFlush() Ted On 2011-10-03 05:54:07, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-03 05:54:07) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178298 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2373
        -----------------------------------------------------------

        Two potential issues that I see:

        #1 - If I'm reading it correctly, this change breaks current behavior for tables where DEFERRED_LOG_FLUSH=true, where HLog.sync() should not be triggered.

        #2 - It also seems like moving the memstore updates prior to the WAL append and sync has the potential to leave around memstore entries that are not present in the WAL if:
        1) the log.sync() call throws an IOException
        2) the memstore entries are made visible via the call to rwcc.completeMemstoreInsert() in the finally block
        3) the client sees the puts as failed due to the IOException
        4) the subsequent roll of the HLog writer triggered by the IOException succeeds – the region server does not abort in this case.

        In this case, wouldn't the client see the update as failed (due to the IOException), while the memstore entries persist? So do the memstore changes need to be tracked and rolled back? Or am I missing something?

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5460>

        Doesn't this break current behavior in respect to tables with deferred log flush set to "true"? Previously no sync was triggered for these tables.

        Also, if the sync call throws an IOException that gets sent back to the client, how do the memstore entries get cleared?

        • Gary

        On 2011-10-03 05:54:07, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-03 05:54:07)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178298

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2373 ----------------------------------------------------------- Two potential issues that I see: #1 - If I'm reading it correctly, this change breaks current behavior for tables where DEFERRED_LOG_FLUSH=true, where HLog.sync() should not be triggered. #2 - It also seems like moving the memstore updates prior to the WAL append and sync has the potential to leave around memstore entries that are not present in the WAL if: 1) the log.sync() call throws an IOException 2) the memstore entries are made visible via the call to rwcc.completeMemstoreInsert() in the finally block 3) the client sees the puts as failed due to the IOException 4) the subsequent roll of the HLog writer triggered by the IOException succeeds – the region server does not abort in this case. In this case, wouldn't the client see the update as failed (due to the IOException), while the memstore entries persist? So do the memstore changes need to be tracked and rolled back? Or am I missing something? /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5460 > Doesn't this break current behavior in respect to tables with deferred log flush set to "true"? Previously no sync was triggered for these tables. Also, if the sync call throws an IOException that gets sent back to the client, how do the memstore entries get cleared? Gary On 2011-10-03 05:54:07, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-03 05:54:07) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178298 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        Ted Yu added a comment -

        See the following in HLog.syncer(long txid) which is called by sync():

            } catch (IOException e) {
              LOG.fatal("Could not sync. Requesting close of hlog", e);
              requestLogRoll();
              throw e;
            }
        
        Show
        Ted Yu added a comment - See the following in HLog.syncer(long txid) which is called by sync(): } catch (IOException e) { LOG.fatal( "Could not sync. Requesting close of hlog" , e); requestLogRoll(); throw e; }
        Hide
        Ted Yu added a comment -

        How about possible IOE from the following call at line 1809:

              txid = this.log.appendNoSync(regionInfo, this.htableDescriptor.getName(),
        
        Show
        Ted Yu added a comment - How about possible IOE from the following call at line 1809: txid = this .log.appendNoSync(regionInfo, this .htableDescriptor.getName(),
        Hide
        dhruba borthakur added a comment -

        from my understanding of the code in HLog.sync(), if it throws an exception then the entire regionserver is supposed to exit, is it not? If that is the case, then is there any point in catching the exception from log.sync()?

        Show
        dhruba borthakur added a comment - from my understanding of the code in HLog.sync(), if it throws an exception then the entire regionserver is supposed to exit, is it not? If that is the case, then is there any point in catching the exception from log.sync()?
        Hide
        Ted Yu added a comment -

        I was referring to possible exception coming out of log.sync() call.

        Show
        Ted Yu added a comment - I was referring to possible exception coming out of log.sync() call.
        Hide
        dhruba borthakur added a comment -

        Hi Ted,

        > Should we guard the rwcc call in the finally block ?

        The method rwcc.completeMemstoreInsert() does not throw an exceptions, so the case you refer to above should not arise, isn't it?

        Show
        dhruba borthakur added a comment - Hi Ted, > Should we guard the rwcc call in the finally block ? The method rwcc.completeMemstoreInsert() does not throw an exceptions, so the case you refer to above should not arise, isn't it?
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2270
        -----------------------------------------------------------

        Ship it!

        • Lars

        On 2011-10-03 05:54:07, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-03 05:54:07)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178298

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2270 ----------------------------------------------------------- Ship it! Lars On 2011-10-03 05:54:07, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-03 05:54:07) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178298 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2263
        -----------------------------------------------------------

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5240>

        What if some exception gets thrown here ?
        Should we guard the rwcc call in the finally block ?

        • Ted

        On 2011-10-03 05:54:07, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-03 05:54:07)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178298

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2263 ----------------------------------------------------------- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5240 > What if some exception gets thrown here ? Should we guard the rwcc call in the finally block ? Ted On 2011-10-03 05:54:07, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-03 05:54:07) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178298 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2262
        -----------------------------------------------------------

        Very Minor comment
        I think the testcase is not using junit3. If it is ok then no problem. Thanks.

        • ramkrishna

        On 2011-10-03 05:54:07, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-03 05:54:07)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178298

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2262 ----------------------------------------------------------- Very Minor comment I think the testcase is not using junit3. If it is ok then no problem. Thanks. ramkrishna On 2011-10-03 05:54:07, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-03 05:54:07) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178298 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2260
        -----------------------------------------------------------

        Ship it!

        One minor comment.

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
        <https://reviews.apache.org/r/2141/#comment5238>

        To achieve greater level of parallelism, you can fold this loop into the loop above and introduce a CountDownLatch so that the Putter's start at the same time.

        See:
        http://programmingexamples.wikidot.com/countdownlatch

        • Ted

        On 2011-10-03 05:54:07, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-03 05:54:07)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178298

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2260 ----------------------------------------------------------- Ship it! One minor comment. /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java < https://reviews.apache.org/r/2141/#comment5238 > To achieve greater level of parallelism, you can fold this loop into the loop above and introduce a CountDownLatch so that the Putter's start at the same time. See: http://programmingexamples.wikidot.com/countdownlatch Ted On 2011-10-03 05:54:07, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-03 05:54:07) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178298 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        dhruba borthakur added a comment -

        All unit tests pass now (expect TestDistributedLogSplitting, TestRollingRestart, TestHTablePool), and these fail on trunk even without my patch.

        Show
        dhruba borthakur added a comment - All unit tests pass now (expect TestDistributedLogSplitting, TestRollingRestart, TestHTablePool), and these fail on trunk even without my patch.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/
        -----------------------------------------------------------

        (Updated 2011-10-03 05:54:07.197305)

        Review request for hbase.

        Changes
        -------

        Incorporated most of Ted's comments and all of Lar's comments.

        I did not change the order of advancing the rwcc first before releasing the rowlock in the finally-clause because this will occur only in some error case, and in that case it might be better to do things in the normal order. Technically, either way should be fine, but if I am missing something please let me know and I can change it too.

        In TestParallelPut, I did not fold the two loops of thread-creation and thread-start. The reason being that I would like more concurrency among the threads, and if I create and start in the same loop then it is likely that by the a thread starts running, the earlier ones would probably be finished or advanced significantly, thus reducing the time when all threads are running concurrently.

        Summary
        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.
        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs (updated)


        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178298
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        Diff: https://reviews.apache.org/r/2141/diff

        Testing
        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-03 05:54:07.197305) Review request for hbase. Changes ------- Incorporated most of Ted's comments and all of Lar's comments. I did not change the order of advancing the rwcc first before releasing the rowlock in the finally-clause because this will occur only in some error case, and in that case it might be better to do things in the normal order. Technically, either way should be fine, but if I am missing something please let me know and I can change it too. In TestParallelPut, I did not fold the two loops of thread-creation and thread-start. The reason being that I would like more concurrency among the threads, and if I create and start in the same loop then it is likely that by the a thread starts running, the earlier ones would probably be finished or advanced significantly, thus reducing the time when all threads are running concurrently. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs (updated) /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178298 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        dhruba borthakur added a comment -

        Incorporated most of Ted's comments and all of Lar's comments.

        I did not change the order of advancing the rwcc first before releasing the rowlock in the finally-clause because this will occur only in some error case, and in that case it might be better to do things in the normal order. Technically, either way should be fine, but if I am missing something please let me know and I can change it too.

        In TestParallelPut, I did not fold the two loops of thread-creation and thread-start. The reason being that I would like more concurrency among the threads, and if I create and start in the same loop then it is likely that by the a thread starts running, the earlier ones would probably be finished or advanced significantly, thus reducing the time when all threads are running concurrently.

        Show
        dhruba borthakur added a comment - Incorporated most of Ted's comments and all of Lar's comments. I did not change the order of advancing the rwcc first before releasing the rowlock in the finally-clause because this will occur only in some error case, and in that case it might be better to do things in the normal order. Technically, either way should be fine, but if I am missing something please let me know and I can change it too. In TestParallelPut, I did not fold the two loops of thread-creation and thread-start. The reason being that I would like more concurrency among the threads, and if I create and start in the same loop then it is likely that by the a thread starts running, the earlier ones would probably be finished or advanced significantly, thus reducing the time when all threads are running concurrently.
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2253
        -----------------------------------------------------------

        Awesome.
        See comments below, I think there is a problem when the operation is marked SUCCESS.

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5232>

        Maybe this should only be set after WAL is synced (see comment below too)?

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5231>

        Is this right?
        retCodeDetails is set to success when writing to the memstore, which means this would never happen (as SUCCESS != NOT_RUN)

        • Lars

        On 2011-10-02 07:16:56, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-02 07:16:56)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178113

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2253 ----------------------------------------------------------- Awesome. See comments below, I think there is a problem when the operation is marked SUCCESS. /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5232 > Maybe this should only be set after WAL is synced (see comment below too)? /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5231 > Is this right? retCodeDetails is set to success when writing to the memstore, which means this would never happen (as SUCCESS != NOT_RUN) Lars On 2011-10-02 07:16:56, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-02 07:16:56) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178113 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/#review2249
        -----------------------------------------------------------

        Nice feature.
        Hopefully there is no surprise in running test suite.

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5215>

        Better remove the whitespace.

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5216>

        Thw should be The

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5217>

        According to the sequence in try block, I think this call should be placed after line 1870.

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5218>

        There should be javadoc for parameter w.

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        <https://reviews.apache.org/r/2141/#comment5219>

        Is localizedRwcc a better name ?

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
        <https://reviews.apache.org/r/2141/#comment5220>

        This is no longer needed.

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
        <https://reviews.apache.org/r/2141/#comment5221>

        Should read "don't spin"

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
        <https://reviews.apache.org/r/2141/#comment5222>

        The word test is redundant.

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
        <https://reviews.apache.org/r/2141/#comment5223>

        I think this loop can be folded into the loop above.

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
        <https://reviews.apache.org/r/2141/#comment5224>

        At least a LOG call should be placed here, for debugging.

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
        <https://reviews.apache.org/r/2141/#comment5225>

        Second part of the sentence should be ' see if the values match'.

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
        <https://reviews.apache.org/r/2141/#comment5226>

        Please remove the space before (.

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
        <https://reviews.apache.org/r/2141/#comment5227>

        Should be initHRegion(, without space.

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
        <https://reviews.apache.org/r/2141/#comment5230>

        This variable should be named row or rowkey.

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
        <https://reviews.apache.org/r/2141/#comment5228>

        Good.

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java
        <https://reviews.apache.org/r/2141/#comment5229>

        Should append ' + " failed"'

        • Ted

        On 2011-10-02 07:16:56, Dhruba Borthakur wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/2141/

        -----------------------------------------------------------

        (Updated 2011-10-02 07:16:56)

        Review request for hbase.

        Summary

        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.

        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs

        -----

        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178113

        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        Diff: https://reviews.apache.org/r/2141/diff

        Testing

        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/#review2249 ----------------------------------------------------------- Nice feature. Hopefully there is no surprise in running test suite. /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5215 > Better remove the whitespace. /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5216 > Thw should be The /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5217 > According to the sequence in try block, I think this call should be placed after line 1870. /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5218 > There should be javadoc for parameter w. /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java < https://reviews.apache.org/r/2141/#comment5219 > Is localizedRwcc a better name ? /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java < https://reviews.apache.org/r/2141/#comment5220 > This is no longer needed. /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java < https://reviews.apache.org/r/2141/#comment5221 > Should read "don't spin" /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java < https://reviews.apache.org/r/2141/#comment5222 > The word test is redundant. /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java < https://reviews.apache.org/r/2141/#comment5223 > I think this loop can be folded into the loop above. /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java < https://reviews.apache.org/r/2141/#comment5224 > At least a LOG call should be placed here, for debugging. /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java < https://reviews.apache.org/r/2141/#comment5225 > Second part of the sentence should be ' see if the values match'. /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java < https://reviews.apache.org/r/2141/#comment5226 > Please remove the space before (. /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java < https://reviews.apache.org/r/2141/#comment5227 > Should be initHRegion(, without space. /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java < https://reviews.apache.org/r/2141/#comment5230 > This variable should be named row or rowkey. /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java < https://reviews.apache.org/r/2141/#comment5228 > Good. /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java < https://reviews.apache.org/r/2141/#comment5229 > Should append ' + " failed"' Ted On 2011-10-02 07:16:56, Dhruba Borthakur wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- (Updated 2011-10-02 07:16:56) Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs ----- /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178113 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/2141/
        -----------------------------------------------------------

        Review request for hbase.

        Summary
        -------

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        This addresses bug HBASE-4528.
        https://issues.apache.org/jira/browse/HBASE-4528

        Diffs


        /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178113
        /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION

        Diff: https://reviews.apache.org/r/2141/diff

        Testing
        -------

        Not yet run the full suite of unit tests.

        Thanks,

        Dhruba

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2141/ ----------------------------------------------------------- Review request for hbase. Summary ------- The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful. This addresses bug HBASE-4528 . https://issues.apache.org/jira/browse/HBASE-4528 Diffs /src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1178113 /src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java PRE-CREATION Diff: https://reviews.apache.org/r/2141/diff Testing ------- Not yet run the full suite of unit tests. Thanks, Dhruba
        Hide
        dhruba borthakur added a comment -
        Show
        dhruba borthakur added a comment - Review board: https://reviews.apache.org/r/2141/
        Hide
        dhruba borthakur added a comment -

        The changes the multiPut operation so that the sync to the wal occurs outside the rowlock.

        This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated.

        I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

        Show
        dhruba borthakur added a comment - The changes the multiPut operation so that the sync to the wal occurs outside the rowlock. This enhancement is done only to HRegion.mut(Put[]) because this is the only method that gets invoked from an application. The HRegion.put(Put) is used only by unit tests and should possibly be deprecated. I have attached a unit test. I have not yet run all unit tests, but early feedback on this patch will be very helpful.

          People

          • Assignee:
            dhruba borthakur
            Reporter:
            dhruba borthakur
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development