HBase
  1. HBase
  2. HBASE-10252

Don't write back to WAL/memstore when Increment amount is zero (mostly for query rather than update intention)

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.98.0, 0.99.0
    • Component/s: regionserver
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      When user calls Increment by providing amount=0, we don't write the original value to WAL or memstore : adding 0 yields a 'new' value just with the same value as the original one.

      1. user provides 0 amount for query rather than for update, this fix is ok; this intention is the most possible case;
      2. user provides 0 amount for an update, this fix is also ok : no need to touch back-end value if that value isn't changed;
      3. either case we both return correct value, and keep subsequent query results correct : if the 0 amount Increment is the first update, the query is the same for retrieving a 0 value or retrieving nothing;

      1. HBASE-10252-trunk-v1.patch
        8 kB
        Honghua Feng
      2. HBASE-10252-trunk-v0.patch
        5 kB
        Honghua Feng

        Activity

        Hide
        Honghua Feng added a comment -

        patch for trunk is provided

        Show
        Honghua Feng added a comment - patch for trunk is provided
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12620750/HBASE-10252-trunk-v0.patch
        against trunk revision .
        ATTACHMENT ID: 12620750

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no new tests are needed for this patch.
        Also please list what manual steps were performed to verify this patch.

        +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop1.1. The patch compiles against the hadoop 1.1 profile.

        +1 javadoc. The javadoc tool did not generate any 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 lineLengths. The patch does not introduce lines longer than 100

        -1 site. The patch appears to cause mvn site goal to fail.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//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/12620750/HBASE-10252-trunk-v0.patch against trunk revision . ATTACHMENT ID: 12620750 +1 @author . The patch does not contain any @author tags. -1 tests included . The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop1.1 . The patch compiles against the hadoop 1.1 profile. +1 javadoc . The javadoc tool did not generate any 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 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8297//console This message is automatically generated.
        Hide
        Honghua Feng added a comment -

        attach new patch with unit-test verifying that noWriteBack takes effect when increment amount is 0. the new unit-test is added to TestDurability to leverage the helper function of HLog entry counting

        Show
        Honghua Feng added a comment - attach new patch with unit-test verifying that noWriteBack takes effect when increment amount is 0. the new unit-test is added to TestDurability to leverage the helper function of HLog entry counting
        Hide
        Honghua Feng added a comment -

        additional words:
        1. no production code to v0 patch;
        2. unit-test verifies: a) original Increment semantic doesn't change; b) noWriteBack takes effect when increment amount is 0 (either single or all)

        ping Andrew Purtell

        Show
        Honghua Feng added a comment - additional words: 1. no production code to v0 patch; 2. unit-test verifies: a) original Increment semantic doesn't change; b) noWriteBack takes effect when increment amount is 0 (either single or all) ping Andrew Purtell
        Hide
        Honghua Feng added a comment -

        correct one statement of above comment:

        1. no production code to v0 patch;

        ==> I meant: new v1 patch only contains a new accoording unit-test, without production code change compared with v0 patch;

        btw: it's a surprise that no unit-test for increment originally, neither for HRegion.increment nor for HTable.increment from client side.

        Show
        Honghua Feng added a comment - correct one statement of above comment: 1. no production code to v0 patch; ==> I meant: new v1 patch only contains a new accoording unit-test, without production code change compared with v0 patch; btw: it's a surprise that no unit-test for increment originally, neither for HRegion.increment nor for HTable.increment from client side.
        Hide
        Honghua Feng added a comment -

        also thanks Ted Yu for the prompt response and review,

        Show
        Honghua Feng added a comment - also thanks Ted Yu for the prompt response and review,
        Hide
        Honghua Feng added a comment -

        btw: to my knowledge, this improvement can be applied to all branches, maybe can't be applied directly (some branches remain incrementColumnValue. etc)

        Show
        Honghua Feng added a comment - btw: to my knowledge, this improvement can be applied to all branches, maybe can't be applied directly (some branches remain incrementColumnValue. etc)
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12620838/HBASE-10252-trunk-v1.patch
        against trunk revision .
        ATTACHMENT ID: 12620838

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

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

        +1 hadoop1.0. The patch compiles against the hadoop 1.0 profile.

        +1 hadoop1.1. The patch compiles against the hadoop 1.1 profile.

        +1 javadoc. The javadoc tool did not generate any 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 lineLengths. The patch does not introduce lines longer than 100

        -1 site. The patch appears to cause mvn site goal to fail.

        +1 core tests. The patch passed unit tests in .

        Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//testReport/
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html
        Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
        Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//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/12620838/HBASE-10252-trunk-v1.patch against trunk revision . ATTACHMENT ID: 12620838 +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop1.0 . The patch compiles against the hadoop 1.0 profile. +1 hadoop1.1 . The patch compiles against the hadoop 1.1 profile. +1 javadoc . The javadoc tool did not generate any 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 lineLengths . The patch does not introduce lines longer than 100 -1 site . The patch appears to cause mvn site goal to fail. +1 core tests . The patch passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-prefix-tree.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-client.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-protocol.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-thrift.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/8303//console This message is automatically generated.
        Hide
        Andrew Purtell added a comment -

        +1 on patch v1 for trunk and 0.98, thanks for the new unit test

        Show
        Andrew Purtell added a comment - +1 on patch v1 for trunk and 0.98, thanks for the new unit test
        Hide
        Ted Yu added a comment -

        Integrated to 0.98 and trunk.

        Thanks for the patch, Honghua.

        Thanks for the review, Andy.

        Show
        Ted Yu added a comment - Integrated to 0.98 and trunk. Thanks for the patch, Honghua. Thanks for the review, Andy.
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-0.98 #45 (See https://builds.apache.org/job/HBase-0.98/45/)
        HBASE-10252 Don't write back to WAL/memstore when Increment amount is zero (tedyu: rev 1554312)

        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestDurability.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-0.98 #45 (See https://builds.apache.org/job/HBase-0.98/45/ ) HBASE-10252 Don't write back to WAL/memstore when Increment amount is zero (tedyu: rev 1554312) /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestDurability.java
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-0.98-on-Hadoop-1.1 #42 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/42/)
        HBASE-10252 Don't write back to WAL/memstore when Increment amount is zero (tedyu: rev 1554312)

        • /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        • /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestDurability.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-0.98-on-Hadoop-1.1 #42 (See https://builds.apache.org/job/HBase-0.98-on-Hadoop-1.1/42/ ) HBASE-10252 Don't write back to WAL/memstore when Increment amount is zero (tedyu: rev 1554312) /hbase/branches/0.98/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/branches/0.98/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestDurability.java
        Hide
        Honghua Feng added a comment -
        Show
        Honghua Feng added a comment - thanks Ted Yu and Andrew Purtell
        Hide
        Hudson added a comment -

        FAILURE: Integrated in HBase-TRUNK-on-Hadoop-1.1 #29 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-1.1/29/)
        HBASE-10252 Don't write back to WAL/memstore when Increment amount is zero (tedyu: rev 1554313)

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestDurability.java
        Show
        Hudson added a comment - FAILURE: Integrated in HBase-TRUNK-on-Hadoop-1.1 #29 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-1.1/29/ ) HBASE-10252 Don't write back to WAL/memstore when Increment amount is zero (tedyu: rev 1554313) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestDurability.java
        Hide
        Hudson added a comment -

        SUCCESS: Integrated in HBase-TRUNK #4770 (See https://builds.apache.org/job/HBase-TRUNK/4770/)
        HBASE-10252 Don't write back to WAL/memstore when Increment amount is zero (tedyu: rev 1554313)

        • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
        • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestDurability.java
        Show
        Hudson added a comment - SUCCESS: Integrated in HBase-TRUNK #4770 (See https://builds.apache.org/job/HBase-TRUNK/4770/ ) HBASE-10252 Don't write back to WAL/memstore when Increment amount is zero (tedyu: rev 1554313) /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestDurability.java
        Hide
        stack added a comment -

        So, this patch breaks an asynchbase test (see the below – thanks to Benoit Sigoure for help debugging). The test presumes that even though the increment value is 0, if the cell does not exist yet, then the cell is created (with a value of 0). That is how it worked in 0.96 and previous.

        Honghua Feng My guess is that you did not intend to remove this behavior? If that is the case, I'll make a small patch in a new issue to restore cell creation though the value is zero. Thanks boss.

        21:28:57.922 [main] ERROR org.hbase.async.test.TestIntegration - Test failed: incrementCoalescingWithZeroSumAmount
        java.lang.AssertionError: List was expected to contain 1 items but was found to contain 0: []
        	at org.hbase.async.test.TestIntegration.assertSizeIs(TestIntegration.java:851) [build/:na]
        	at org.hbase.async.test.TestIntegration.incrementCoalescingWithZeroSumAmount(TestIntegration.java:595) [build/:na]
        	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:1.7.0_45]
        	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57) ~[na:1.7.0_45]
        	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:1.7.0_45]
        	at java.lang.reflect.Method.invoke(Method.java:606) ~[na:1.7.0_45]
        	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47) ~[junit-4.11.jar:na]
        	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) ~[junit-4.11.jar:na]
        	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44) ~[junit-4.11.jar:na]
        	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) ~[junit-4.11.jar:na]
        	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) ~[junit-4.11.jar:na]
        	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27) ~[junit-4.11.jar:na]
        	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271) [junit-4.11.jar:na]
        	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70) [junit-4.11.jar:na]
        	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50) [junit-4.11.jar:na]
        	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238) [junit-4.11.jar:na]
        	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63) [junit-4.11.jar:na]
        	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236) [junit-4.11.jar:na]
        	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53) [junit-4.11.jar:na]
        	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229) [junit-4.11.jar:na]
        	at org.junit.runners.ParentRunner.run(ParentRunner.java:309) [junit-4.11.jar:na]
        	at org.junit.runner.JUnitCore.run(JUnitCore.java:160) [junit-4.11.jar:na]
        	at org.junit.runner.JUnitCore.run(JUnitCore.java:138) [junit-4.11.jar:na]
        	at org.hbase.async.test.TestIntegration.main(TestIntegration.java:133) [build/:na]
        
        Show
        stack added a comment - So, this patch breaks an asynchbase test (see the below – thanks to Benoit Sigoure for help debugging). The test presumes that even though the increment value is 0, if the cell does not exist yet, then the cell is created (with a value of 0). That is how it worked in 0.96 and previous. Honghua Feng My guess is that you did not intend to remove this behavior? If that is the case, I'll make a small patch in a new issue to restore cell creation though the value is zero. Thanks boss. 21:28:57.922 [main] ERROR org.hbase.async.test.TestIntegration - Test failed: incrementCoalescingWithZeroSumAmount java.lang.AssertionError: List was expected to contain 1 items but was found to contain 0: [] at org.hbase.async.test.TestIntegration.assertSizeIs(TestIntegration.java:851) [build/:na] at org.hbase.async.test.TestIntegration.incrementCoalescingWithZeroSumAmount(TestIntegration.java:595) [build/:na] at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:1.7.0_45] at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57) ~[na:1.7.0_45] at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:1.7.0_45] at java.lang.reflect.Method.invoke(Method.java:606) ~[na:1.7.0_45] at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47) ~[junit-4.11.jar:na] at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) ~[junit-4.11.jar:na] at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44) ~[junit-4.11.jar:na] at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) ~[junit-4.11.jar:na] at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) ~[junit-4.11.jar:na] at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27) ~[junit-4.11.jar:na] at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271) [junit-4.11.jar:na] at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70) [junit-4.11.jar:na] at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50) [junit-4.11.jar:na] at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238) [junit-4.11.jar:na] at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63) [junit-4.11.jar:na] at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236) [junit-4.11.jar:na] at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53) [junit-4.11.jar:na] at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229) [junit-4.11.jar:na] at org.junit.runners.ParentRunner.run(ParentRunner.java:309) [junit-4.11.jar:na] at org.junit.runner.JUnitCore.run(JUnitCore.java:160) [junit-4.11.jar:na] at org.junit.runner.JUnitCore.run(JUnitCore.java:138) [junit-4.11.jar:na] at org.hbase.async.test.TestIntegration.main(TestIntegration.java:133) [build/:na]
        Hide
        Honghua Feng added a comment -

        The test presumes that even though the increment value is 0, if the cell does not exist yet, then the cell is created (with a value of 0). That is how it worked in 0.96 and previous. My guess is that you did not intend to remove this behavior? If that is the case, I'll make a small patch in a new issue to restore cell creation though the value is zero. Thanks boss.

        This patch is to save the write to wal/memstore when the increment value is 0, so 'if the increment value is 0, and if the cell does not exist yet, then the cell won't be created' is a natural resultant behavior of this patch...
        I'm not familiar with asynchbase itself, but wonder why it cares about whether a cell exists after an increment operation? seems what really matters from a 'client' perspective is whether the value it reads back is correct after some increment is performed (such as a read can confirm that the value it reads back is 0 immediately after a first increment with value=0, if that's the case, it's deemed correct, it shouldn't care about whether that cell exists or not. The value is 0 for increment under both scenarios 1) non-existing cell and 2) an existing cell with value=0). if my understanding is correct, seems the according asynchbase test,rather than the hbase code,should be corrected here.

        Show
        Honghua Feng added a comment - The test presumes that even though the increment value is 0, if the cell does not exist yet, then the cell is created (with a value of 0). That is how it worked in 0.96 and previous. My guess is that you did not intend to remove this behavior? If that is the case, I'll make a small patch in a new issue to restore cell creation though the value is zero. Thanks boss. This patch is to save the write to wal/memstore when the increment value is 0, so 'if the increment value is 0, and if the cell does not exist yet, then the cell won't be created' is a natural resultant behavior of this patch... I'm not familiar with asynchbase itself, but wonder why it cares about whether a cell exists after an increment operation? seems what really matters from a 'client' perspective is whether the value it reads back is correct after some increment is performed (such as a read can confirm that the value it reads back is 0 immediately after a first increment with value=0, if that's the case, it's deemed correct, it shouldn't care about whether that cell exists or not. The value is 0 for increment under both scenarios 1) non-existing cell and 2) an existing cell with value=0). if my understanding is correct, seems the according asynchbase test,rather than the hbase code,should be corrected here.
        Hide
        Honghua Feng added a comment -

        as above explanation, whether a cell exists is not part of the semantic of increment API, so the test against it shouldn't impose such assumption/check as well

        you certainly can make a small patch to make the failed test pass by creating a new cell with 0 value when the cell is non-existing and the increment value is 0, but that can make the code with additional special cases/handling, and the special case added this time is not required from the API semantic perspective

        your opinion?

        Show
        Honghua Feng added a comment - as above explanation, whether a cell exists is not part of the semantic of increment API, so the test against it shouldn't impose such assumption/check as well you certainly can make a small patch to make the failed test pass by creating a new cell with 0 value when the cell is non-existing and the increment value is 0, but that can make the code with additional special cases/handling, and the special case added this time is not required from the API semantic perspective your opinion?
        Hide
        stack added a comment -

        Good pushback Honghua Feng If amount is 0 and we still have to test existence, then we lose the improvement you have put up here. Let me chat w/ asynchbasers... This is a change in semantic from previous versions and while case can be made for behavior being either the old or the new, lets doc how it is now and move forward; let me do that and write a unit test to underscore our new expectation.

        Show
        stack added a comment - Good pushback Honghua Feng If amount is 0 and we still have to test existence, then we lose the improvement you have put up here. Let me chat w/ asynchbasers... This is a change in semantic from previous versions and while case can be made for behavior being either the old or the new, lets doc how it is now and move forward; let me do that and write a unit test to underscore our new expectation.
        Hide
        Andrew Purtell added a comment -

        Sounds good Stack

        Show
        Andrew Purtell added a comment - Sounds good Stack
        Hide
        stack added a comment -

        I opened https://github.com/OpenTSDB/asynchbase/issues/78 to change asynchbase test.

        Show
        stack added a comment - I opened https://github.com/OpenTSDB/asynchbase/issues/78 to change asynchbase test.
        Hide
        Honghua Feng added a comment -

        Sounds good stack

        Show
        Honghua Feng added a comment - Sounds good stack

          People

          • Assignee:
            Honghua Feng
            Reporter:
            Honghua Feng
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development