HBase
  1. HBase
  2. HBASE-4583

Integrate RWCC with Append and Increment operations

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.95.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      Hide
      This issue fixes MVCC issues with Increment and Append. To retain the current performance characteristics, VERSIONS should be set to 1 on column families with columns to be incremented/appended-to.
      If VERSIONS is > 1 historical versions are kept for timerange queries, but Increment/Appends will be slower due to changes accumulating the memstore leading to frequent flushes.
      Show
      This issue fixes MVCC issues with Increment and Append. To retain the current performance characteristics, VERSIONS should be set to 1 on column families with columns to be incremented/appended-to. If VERSIONS is > 1 historical versions are kept for timerange queries, but Increment/Appends will be slower due to changes accumulating the memstore leading to frequent flushes.

      Description

      Currently Increment and Append operations do not work with RWCC and hence a client could see the results of multiple such operation mixed in the same Get/Scan.
      The semantics might be a bit more interesting here as upsert adds and removes to and from the memstore.

      1. 4584-0.94-v1.txt
        23 kB
        Varun Sharma
      2. 4583-mixed-v4.txt
        19 kB
        Lars Hofhansl
      3. 4583-mixed-v2.txt
        19 kB
        Lars Hofhansl
      4. 4583-mixed.txt
        18 kB
        Lars Hofhansl
      5. 4583-trunk-less-radical-v6.txt
        18 kB
        Lars Hofhansl
      6. 4583-trunk-less-radical-v5.txt
        29 kB
        Lars Hofhansl
      7. 4583-trunk-less-radical-v4.txt
        29 kB
        Lars Hofhansl
      8. 4583-trunk-less-radical-v3.txt
        15 kB
        Lars Hofhansl
      9. 4583-trunk-less-radical-v2.txt
        14 kB
        Lars Hofhansl
      10. 4583-trunk-less-radical.txt
        12 kB
        Lars Hofhansl
      11. 4583-trunk-v3.txt
        27 kB
        Lars Hofhansl
      12. 4583-trunk-radical_v2.txt
        24 kB
        Lars Hofhansl
      13. 4583-trunk-radical.txt
        21 kB
        Lars Hofhansl
      14. 4583-v4.txt
        24 kB
        Lars Hofhansl
      15. 4583-v3.txt
        24 kB
        Lars Hofhansl
      16. 4583-v2.txt
        15 kB
        Lars Hofhansl
      17. 4583.txt
        15 kB
        Lars Hofhansl

        Issue Links

          Activity

          Hide
          Jonathan Gray added a comment -

          We likely won't be able to do in-place modifications or direct KV removal from MemStore. A simple way would be to also introduce a delete marker that removes the previous value, but the marker will have the rwcc of the new edit, so you'll have the right consistency.

          This will lead to a build up of unnecessary KVs in the MemStore. Periodically cleaning that up would be possible but unnecessarily complex I think.

          Another option would be to remove the previous KVs after you roll rwcc forward and release the row lock, before dropping the region-level lock. Should definitely be possible. Will obviously require a remangling of upsert but it's kinda dirty anyways.

          Show
          Jonathan Gray added a comment - We likely won't be able to do in-place modifications or direct KV removal from MemStore. A simple way would be to also introduce a delete marker that removes the previous value, but the marker will have the rwcc of the new edit, so you'll have the right consistency. This will lead to a build up of unnecessary KVs in the MemStore. Periodically cleaning that up would be possible but unnecessarily complex I think. Another option would be to remove the previous KVs after you roll rwcc forward and release the row lock, before dropping the region-level lock. Should definitely be possible. Will obviously require a remangling of upsert but it's kinda dirty anyways.
          Hide
          Lars Hofhansl added a comment -

          Another option would be to remove the previous KVs after you roll rwcc forward and release the row lock, before dropping the region-level lock.

          That should work. I'll take a crack at it.

          Show
          Lars Hofhansl added a comment - Another option would be to remove the previous KVs after you roll rwcc forward and release the row lock, before dropping the region-level lock. That should work. I'll take a crack at it.
          Hide
          Lars Hofhansl added a comment -

          Looking at the code. There are some other weird things:
          1. HRegion.applyFamilyMapToMemstore calls Store.add for each KV, which in turns calls Memstore.add. At each iteration both the Store and the Memstore take and release the read lock. Should be more efficient to pass a set of only take the locks once (at the expense of slightly reduce lock granularity).
          2. Both HRegion.increment and HRegion.append (which is modeled after increment) update the memstore first, and then write the WAL. If the WAL sync fails there would be uncommitted state in the memstore. Fixing this should either incur a performance penalty (need to sync the WAL with the update.readLock held, or require memstore rollback (like we do for put now).

          I think these two should have separate jiras.

          Show
          Lars Hofhansl added a comment - Looking at the code. There are some other weird things: 1. HRegion.applyFamilyMapToMemstore calls Store.add for each KV, which in turns calls Memstore.add. At each iteration both the Store and the Memstore take and release the read lock. Should be more efficient to pass a set of only take the locks once (at the expense of slightly reduce lock granularity). 2. Both HRegion.increment and HRegion.append (which is modeled after increment) update the memstore first, and then write the WAL. If the WAL sync fails there would be uncommitted state in the memstore. Fixing this should either incur a performance penalty (need to sync the WAL with the update.readLock held, or require memstore rollback (like we do for put now). I think these two should have separate jiras.
          Hide
          Ted Yu added a comment -

          Let's log an issue for #2 above.

          Show
          Ted Yu added a comment - Let's log an issue for #2 above.
          Hide
          Lars Hofhansl added a comment - - edited

          Here's a sketch of a patch.
          May need some clean up but should work.

          Basically the KVs of Append and Increment are applied to memstore through a call to the "standard" applyFamilyMapToMemstore(...); then duplicates are removed after the rwcc is moved forward.
          Still a lot of boilerplate shared between Append and Increment.

          I think the dup removal does not need to hold the regions updatesLock.readLock (just the lock.readLock), but I am not entirely sure.

          Comments/suggestions/flames/praise are welcome.

          Show
          Lars Hofhansl added a comment - - edited Here's a sketch of a patch. May need some clean up but should work. Basically the KVs of Append and Increment are applied to memstore through a call to the "standard" applyFamilyMapToMemstore(...); then duplicates are removed after the rwcc is moved forward. Still a lot of boilerplate shared between Append and Increment. I think the dup removal does not need to hold the regions updatesLock.readLock (just the lock.readLock), but I am not entirely sure. Comments/suggestions/flames/praise are welcome.
          Hide
          Lars Hofhansl added a comment - - edited

          Also changes some calls to take "? extends Collection" rather than List, so that I can use guava's MultiMap. If all callers would be changed the "? extends" could be removed.

          Show
          Lars Hofhansl added a comment - - edited Also changes some calls to take "? extends Collection" rather than List, so that I can use guava's MultiMap. If all callers would be changed the "? extends" could be removed.
          Hide
          Ted Yu added a comment -

          Publishing on reviewboard would make reviewing much easier.

          Patch looks good overall.

          A few minor comments.
          newFamilyMap.asMap() is called more than once in append() and increment(). We can use a variable to hold its return and reuse the variable in place of the second call.
          Line 3948 can be omitted:

                 flush = isFlushSize(size);
          

          because we have the same call at line 3964.
          Same argument can be made for the call at line 3824.

          In MemStore.java:

             * For each KeyValue if the keyValue did already exist, with a
          

          Should read 'if the KeyValue does already ...'

          This comment in Store.java can be refined:

             * qualifier exists in MemStore with a memstoreTS < the passed KV, it will be removed.
          

          because we only pass keyvalues to this.memstore.removeDups().

          Show
          Ted Yu added a comment - Publishing on reviewboard would make reviewing much easier. Patch looks good overall. A few minor comments. newFamilyMap.asMap() is called more than once in append() and increment(). We can use a variable to hold its return and reuse the variable in place of the second call. Line 3948 can be omitted: flush = isFlushSize(size); because we have the same call at line 3964. Same argument can be made for the call at line 3824. In MemStore.java: * For each KeyValue if the keyValue did already exist, with a Should read 'if the KeyValue does already ...' This comment in Store.java can be refined: * qualifier exists in MemStore with a memstoreTS < the passed KV, it will be removed. because we only pass keyvalues to this.memstore.removeDups().
          Hide
          Ted Yu added a comment -

          Still a lot of boilerplate shared between Append and Increment.

          I think we should take this opportunity to reduce duplicate code.

          Show
          Ted Yu added a comment - Still a lot of boilerplate shared between Append and Increment. I think we should take this opportunity to reduce duplicate code.
          Hide
          Lars Hofhansl added a comment -

          The reason why I am calling isFlushSize again is because the duplicate removal might fail and that should not impact the operation. So I call it first in the required part and then again in the part that might fail. If the 2nd part fails, the first value should be used.

          Agree on the boiler plate removal.

          Show
          Lars Hofhansl added a comment - The reason why I am calling isFlushSize again is because the duplicate removal might fail and that should not impact the operation. So I call it first in the required part and then again in the part that might fail. If the 2nd part fails, the first value should be used. Agree on the boiler plate removal.
          Hide
          Ted Yu added a comment -

          @Lars:
          removeDupsInMemstore() isn't declared to throw exception. So I didn't expect failure in that part of the code.
          If runtime exception comes out of it, would requestFlush() still be executed ?

          Show
          Ted Yu added a comment - @Lars: removeDupsInMemstore() isn't declared to throw exception. So I didn't expect failure in that part of the code. If runtime exception comes out of it, would requestFlush() still be executed ?
          Hide
          Lars Hofhansl added a comment -
          Show
          Lars Hofhansl added a comment - New patch on rb: https://reviews.apache.org/r/2633/
          Hide
          Lars Hofhansl added a comment -

          Some amount of boilerplate is necessary, because of the locking requirements (i.e. cannot have an upsert method in HRegion, since the setup and the first part of upsert need to have the same locks held).
          Could have a boilerplate method and pass in a command object, but that would not necessarily make it more readable.

          Show
          Lars Hofhansl added a comment - Some amount of boilerplate is necessary, because of the locking requirements (i.e. cannot have an upsert method in HRegion, since the setup and the first part of upsert need to have the same locks held). Could have a boilerplate method and pass in a command object, but that would not necessarily make it more readable.
          Hide
          Ted Yu added a comment -

          and then when the rowlock and the updatesLock.readLock are released, but after the rwcc is forwarded, the old rows are removed from the memstore.

          I think the above description would be more readable if expressed this way:

          after the rwcc is forwarded and, the rowlock and the updatesLock.readLock are released, the old rows are removed from the memstore.

          Correct me if the above is wrong.

          Show
          Ted Yu added a comment - and then when the rowlock and the updatesLock.readLock are released, but after the rwcc is forwarded, the old rows are removed from the memstore. I think the above description would be more readable if expressed this way: after the rwcc is forwarded and, the rowlock and the updatesLock.readLock are released, the old rows are removed from the memstore. Correct me if the above is wrong.
          Hide
          Lars Hofhansl added a comment -

          Sure, that is more to the point.

          Show
          Lars Hofhansl added a comment - Sure, that is more to the point.
          Hide
          Hadoop QA added a comment -

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

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

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

          -1 javadoc. The javadoc tool appears to have generated -166 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.master.TestMasterFailover
          org.apache.hadoop.hbase.master.TestDistributedLogSplitting

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/106//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/106//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/106//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/12501538/4583-v2.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -166 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.master.TestMasterFailover org.apache.hadoop.hbase.master.TestDistributedLogSplitting Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/106//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/106//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/106//console This message is automatically generated.
          Hide
          Lars Hofhansl added a comment -

          Much cleaned up patch. Unifies the atomicUpsert (the called must acquire the locks, atomicUpsert releases them).

          Also includes rollback logic in case the sync of the WAL fails.

          Show
          Lars Hofhansl added a comment - Much cleaned up patch. Unifies the atomicUpsert (the called must acquire the locks, atomicUpsert releases them). Also includes rollback logic in case the sync of the WAL fails.
          Hide
          Lars Hofhansl added a comment -

          Latest patch (also on RB, but needs to be attached so that I can trigger a patch build).

          Show
          Lars Hofhansl added a comment - Latest patch (also on RB, but needs to be attached so that I can trigger a patch build).
          Hide
          Hadoop QA added a comment -

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

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

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

          -1 javadoc. The javadoc tool appears to have generated -166 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.master.TestDistributedLogSplitting
          org.apache.hadoop.hbase.master.TestMasterFailover

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/107//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/107//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/107//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/12501555/4583-v3.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -166 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.master.TestDistributedLogSplitting org.apache.hadoop.hbase.master.TestMasterFailover Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/107//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/107//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/107//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

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

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

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

          -1 javadoc. The javadoc tool appears to have generated -166 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.master.TestMasterFailover
          org.apache.hadoop.hbase.master.TestDistributedLogSplitting
          org.apache.hadoop.hbase.regionserver.TestSplitLogWorker

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/108//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/108//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/108//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/12501559/4583-v4.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 javadoc. The javadoc tool appears to have generated -166 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.master.TestMasterFailover org.apache.hadoop.hbase.master.TestDistributedLogSplitting org.apache.hadoop.hbase.regionserver.TestSplitLogWorker Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/108//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/108//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/108//console This message is automatically generated.
          Show
          Ted Yu added a comment - I don't see 'Too many open files' in https://builds.apache.org/job/PreCommit-HBASE-Build/108//testReport/org.apache.hadoop.hbase.regionserver/TestSplitLogWorker/testAcquireTaskAtStartup/
          Hide
          Lars Hofhansl added a comment -

          TestSplitLogWorker passes locally with patch applied.

          Show
          Lars Hofhansl added a comment - TestSplitLogWorker passes locally with patch applied.
          Hide
          Lars Hofhansl added a comment -

          As discussed on rb (thank Prakash and Jon), this is a bit more complicated than expected. Because append and increment are not idempotent we have to produce serializable schedules
          (when we start with a value of 1 and add 1 by two separate increment operations, the end result has to be 3, regardless of how the two increment operations are interleaved).

          There are various ways to produce serializable schedules (pessimistic locking, optimistic locking with rechecking of pre conditions, snapshot isolation, etc), all which will probably mean worse performance for both append and increment.

          As said above the current implementation sync's the WAL after the memstore is updated and the new values are visible to other threads, and after the locks are released. A failure to sync the WAL will leave uncommitted state in the memstore, in addition other threads can be see uncommitted state.

          We can only safely make the changes available to other threads after (1) the WAL is committed (for these atomic, non-idempotent operations at least), at the same time we need to (2) retain the row lock until the rwcc is forwarded (otherwise two increment operations could come in and both see the same start value, leading to a non serializable schedule)... So my current patch is no good (it leads to problem (2))

          (1) and (2) together mean that the WAL needs to be sync'ed with the row lock held (which would be quite a performance degradation).

          Now, what we could do is use rwcc to make the changes to the CFs atomic, and still sync the WAL after all the locks are released (as we do now). With this compromise everything would be correct unless the sync'ing of WAL fails (the theme used for puts to release locks early, then forward rwcc, and do a rollback of the applied values if WAL sync fails, does not work here).

          Show
          Lars Hofhansl added a comment - As discussed on rb (thank Prakash and Jon), this is a bit more complicated than expected. Because append and increment are not idempotent we have to produce serializable schedules (when we start with a value of 1 and add 1 by two separate increment operations, the end result has to be 3, regardless of how the two increment operations are interleaved). There are various ways to produce serializable schedules (pessimistic locking, optimistic locking with rechecking of pre conditions, snapshot isolation, etc), all which will probably mean worse performance for both append and increment. As said above the current implementation sync's the WAL after the memstore is updated and the new values are visible to other threads, and after the locks are released. A failure to sync the WAL will leave uncommitted state in the memstore, in addition other threads can be see uncommitted state. We can only safely make the changes available to other threads after (1) the WAL is committed (for these atomic, non-idempotent operations at least), at the same time we need to (2) retain the row lock until the rwcc is forwarded (otherwise two increment operations could come in and both see the same start value, leading to a non serializable schedule)... So my current patch is no good (it leads to problem (2)) (1) and (2) together mean that the WAL needs to be sync'ed with the row lock held (which would be quite a performance degradation). Now, what we could do is use rwcc to make the changes to the CFs atomic, and still sync the WAL after all the locks are released (as we do now). With this compromise everything would be correct unless the sync'ing of WAL fails (the theme used for puts to release locks early, then forward rwcc, and do a rollback of the applied values if WAL sync fails, does not work here).
          Hide
          stack added a comment -

          There are various ways to produce serializable schedules (pessimistic locking, optimistic locking with rechecking of pre conditions, snapshot isolation, etc), all which will probably mean worse performance for both append and increment.

          Shouldn't we do it anyways (though big yuck on your list above – it makes my brain hurt just thinking on it. Can you imagine rechecking pre-conditions and then replaying the failed transaction.. how much fun that'll be to code up!)?

          Shouldn't we be correct first and then performant?

          As said above the current implementation sync's the WAL after the memstore is updated and the new values are visible to other threads, and after the locks are released.

          Sounds broke to me; sounds like big compromise for sake of better perf. Should we open new issue on this?

          (1) and (2) together mean that the WAL needs to be sync'ed with the row lock held (which would be quite a performance degradation).

          Shouldn't we ship with this config. with options to run hbase otherwise (memstore put then sync, etc.)

          Now, what we could do is use rwcc to make the changes to the CFs atomic, and still sync the WAL after all the locks are released (as we do now). With this compromise everything would be correct unless the sync'ing of WAL fails

          Sounds broke still?

          Thanks for the write up and for digging in here fellas.

          Show
          stack added a comment - There are various ways to produce serializable schedules (pessimistic locking, optimistic locking with rechecking of pre conditions, snapshot isolation, etc), all which will probably mean worse performance for both append and increment. Shouldn't we do it anyways (though big yuck on your list above – it makes my brain hurt just thinking on it. Can you imagine rechecking pre-conditions and then replaying the failed transaction.. how much fun that'll be to code up!)? Shouldn't we be correct first and then performant? As said above the current implementation sync's the WAL after the memstore is updated and the new values are visible to other threads, and after the locks are released. Sounds broke to me; sounds like big compromise for sake of better perf. Should we open new issue on this? (1) and (2) together mean that the WAL needs to be sync'ed with the row lock held (which would be quite a performance degradation). Shouldn't we ship with this config. with options to run hbase otherwise (memstore put then sync, etc.) Now, what we could do is use rwcc to make the changes to the CFs atomic, and still sync the WAL after all the locks are released (as we do now). With this compromise everything would be correct unless the sync'ing of WAL fails Sounds broke still? Thanks for the write up and for digging in here fellas.
          Hide
          Lars Hofhansl added a comment -

          You make a good point. If people want performance they'd pass false as wreToWal. Otherwise they will get correct and "slow" behavior.

          Show
          Lars Hofhansl added a comment - You make a good point. If people want performance they'd pass false as wreToWal. Otherwise they will get correct and "slow" behavior.
          Hide
          Lars Hofhansl added a comment -

          After thinking about this for a bit I am going to follow a different approach:
          1. get locks
          2. setup rwcc
          3. sync WAL (of requested)
          4. update memstore (add KVS)
          5. forward rwcc
          6. release locks
          7. remove old dup KVs from memstore (pure optimization)

          This is guaranteed to produce serializable schedules (but the WAL is sync'ed with lock held). Somebody who's not interested in absolute correctness, could pass false as writeToWal or enable deferredFlush.
          Since the expensive part is sync'ing the WAL I do not see any ways to make this better.

          Will have a new patch later tonight.

          Show
          Lars Hofhansl added a comment - After thinking about this for a bit I am going to follow a different approach: 1. get locks 2. setup rwcc 3. sync WAL (of requested) 4. update memstore (add KVS) 5. forward rwcc 6. release locks 7. remove old dup KVs from memstore (pure optimization) This is guaranteed to produce serializable schedules (but the WAL is sync'ed with lock held). Somebody who's not interested in absolute correctness, could pass false as writeToWal or enable deferredFlush. Since the expensive part is sync'ing the WAL I do not see any ways to make this better. Will have a new patch later tonight.
          Hide
          Lars Hofhansl added a comment -

          Oops. Hit "add" too early.
          6. Release row lock
          7. remove old dup KVs from memstore (pure optimization)
          8. release region lock

          Show
          Lars Hofhansl added a comment - Oops. Hit "add" too early. 6. Release row lock 7. remove old dup KVs from memstore (pure optimization) 8. release region lock
          Hide
          stack added a comment -

          +1

          If a correct path in place, we might spend time trying to make it faster rather than speedup the by-pass.

          Show
          stack added a comment - +1 If a correct path in place, we might spend time trying to make it faster rather than speedup the by-pass.
          Hide
          Lars Hofhansl added a comment -

          I have a new patch that does the above.
          I wrote a multithreaded stress test and I found the following:

          1. I get incorrect behavior if I remove duplicates after the rowlock is released. (doing that with the rowlock held solved the problem).

          2. Even though the end result is correct, if I have the threads do GET operations I see that they do not always get all columns for a row. More interestingly I find sometimes that even KVs with a different timestamps and different memstoreTS show up in the same Get. (note that no flushing happens during the tests, so this has nothing to do with store files not carrying a memstoreTS).

          I thought at the least the Memstore was consistent (i.e. scanners only see columns when rwcc is rolled forward, but then they would see all columns affected by that writepoint).

          Show
          Lars Hofhansl added a comment - I have a new patch that does the above. I wrote a multithreaded stress test and I found the following: 1. I get incorrect behavior if I remove duplicates after the rowlock is released. (doing that with the rowlock held solved the problem). 2. Even though the end result is correct, if I have the threads do GET operations I see that they do not always get all columns for a row. More interestingly I find sometimes that even KVs with a different timestamps and different memstoreTS show up in the same Get. (note that no flushing happens during the tests, so this has nothing to do with store files not carrying a memstoreTS). I thought at the least the Memstore was consistent (i.e. scanners only see columns when rwcc is rolled forward, but then they would see all columns affected by that writepoint).
          Hide
          Lars Hofhansl added a comment -

          The problem is that if the changes are not visible immediately there might be old scanners still scanning the old KVs in the memstore and hence these cannot be removed.

          Increment/Append/ICV currently work because all changes are visible to all scanners, because only then can we safely remove the duplicates.

          I now think that this:

          Another option would be to remove the previous KVs after you roll rwcc forward and release the row lock, before dropping the region-level lock

          Actually does not work.

          In order to make this work we would need to know the lowest readpoint currently used by any other (read) transaction and only remove duplicate KVs before that readpoint.
          Since we do not have that information and readers also do not take out any locks, I don't see any way of making this work.

          Show
          Lars Hofhansl added a comment - The problem is that if the changes are not visible immediately there might be old scanners still scanning the old KVs in the memstore and hence these cannot be removed. Increment/Append/ICV currently work because all changes are visible to all scanners, because only then can we safely remove the duplicates. I now think that this: Another option would be to remove the previous KVs after you roll rwcc forward and release the row lock, before dropping the region-level lock Actually does not work. In order to make this work we would need to know the lowest readpoint currently used by any other (read) transaction and only remove duplicate KVs before that readpoint. Since we do not have that information and readers also do not take out any locks, I don't see any way of making this work.
          Hide
          stack added a comment -

          Nice work Lars.

          Increment/Append/ICV currently work because all changes are visible to all scanners, because only then can we safely remove the duplicates.

          This is 'cheating', right?

          Since we do not have that information and readers also do not take out any locks, I don't see any way of making this work.

          Downside to leaving the duplicates in place would make for an explosion in versions?

          Show
          stack added a comment - Nice work Lars. Increment/Append/ICV currently work because all changes are visible to all scanners, because only then can we safely remove the duplicates. This is 'cheating', right? Since we do not have that information and readers also do not take out any locks, I don't see any way of making this work. Downside to leaving the duplicates in place would make for an explosion in versions?
          Hide
          Lars Hofhansl added a comment -

          Using memstoreTS of 0 is not necessarily cheating. Depends on how we define the semantics.
          Not removing the old versions leads to an explosion of versions indeed, i also found hard to define behavior if two increments happen in the same ms.
          The question is also: do we need increments/ appends consistent across cfs?
          The real problem is that we're making values available before the wal was written.
          (Typed on a phone)

          Show
          Lars Hofhansl added a comment - Using memstoreTS of 0 is not necessarily cheating. Depends on how we define the semantics. Not removing the old versions leads to an explosion of versions indeed, i also found hard to define behavior if two increments happen in the same ms. The question is also: do we need increments/ appends consistent across cfs? The real problem is that we're making values available before the wal was written. (Typed on a phone)
          Hide
          Lars Hofhansl added a comment -

          (on a computer now)

          So to be more specific... The problem is that we cannot remove duplicate KVs unless we can guarantee that no scanners still operation on these KVs; the only way (currently) to guarantee this is to set the memstoreTS of the changed (new) KVs to 0 and hence making them available to all scanners immediately.
          That on the other hand means that we cannot delay the visibility of any CFs until after all CFs are updated.
          If we do not remove duplicate KVs the memstore will explode (and I also have to investigate the strange behavior I saw with atomic operations with identical timestamps).

          We can still, however, at least update and sync the WAL first and then modify the memstore.

          Show
          Lars Hofhansl added a comment - (on a computer now) So to be more specific... The problem is that we cannot remove duplicate KVs unless we can guarantee that no scanners still operation on these KVs; the only way (currently) to guarantee this is to set the memstoreTS of the changed (new) KVs to 0 and hence making them available to all scanners immediately. That on the other hand means that we cannot delay the visibility of any CFs until after all CFs are updated. If we do not remove duplicate KVs the memstore will explode (and I also have to investigate the strange behavior I saw with atomic operations with identical timestamps). We can still, however, at least update and sync the WAL first and then modify the memstore.
          Hide
          Ted Yu added a comment -

          We can still, however, at least update and sync the WAL first and then modify the memstore.

          Sounds fine to me.

          Consistency across multiple column families can be handled in HBASE-2856.

          Show
          Ted Yu added a comment - We can still, however, at least update and sync the WAL first and then modify the memstore. Sounds fine to me. Consistency across multiple column families can be handled in HBASE-2856 .
          Hide
          Lars Hofhansl added a comment -

          I don't think that HBASE-2856 will address the issues I raised above (which is caused by deleting KVs from the memstore).

          Updating the WAL first, then applying the changes to the memstore, and making them visible immediately, all while holding the rowlock is a relatively little change. It'll make Increment/Append/ICV behave correctly in the face of failing WAL sync's, but will also significantly slow them down (in terms of throughput, but not latency).

          I will factor some common bits out (similar to my initial patch), so that if we want to tackle this in the future (for example by maintaining the lowest current readpoint in use, or something), there'll be just one place to change it.

          If one want performance, one would set writeToWal to false; but maybe these three ops should get another flag to allow for delayed log flush (which would be the current behavior).

          Lastly, we can just document that the Append and Increment operations are convenience methods for grouping many updates into a single roundtrip (not to achieve consistency between CFs).

          Show
          Lars Hofhansl added a comment - I don't think that HBASE-2856 will address the issues I raised above (which is caused by deleting KVs from the memstore). Updating the WAL first, then applying the changes to the memstore, and making them visible immediately, all while holding the rowlock is a relatively little change. It'll make Increment/Append/ICV behave correctly in the face of failing WAL sync's, but will also significantly slow them down (in terms of throughput, but not latency). I will factor some common bits out (similar to my initial patch), so that if we want to tackle this in the future (for example by maintaining the lowest current readpoint in use, or something), there'll be just one place to change it. If one want performance, one would set writeToWal to false; but maybe these three ops should get another flag to allow for delayed log flush (which would be the current behavior). Lastly, we can just document that the Append and Increment operations are convenience methods for grouping many updates into a single roundtrip (not to achieve consistency between CFs).
          Hide
          Lars Hofhansl added a comment -

          I think I might be able to use HRegion.getSmallestReadPoint() for this (introduced by HBASE-2856).

          We cannot unconditionally remove cells from the memstore as we do now, but we can remove all those that are before the region's smallestReadPoint, as we know that no scanner can see those anyway.

          Show
          Lars Hofhansl added a comment - I think I might be able to use HRegion.getSmallestReadPoint() for this (introduced by HBASE-2856 ). We cannot unconditionally remove cells from the memstore as we do now, but we can remove all those that are before the region's smallestReadPoint, as we know that no scanner can see those anyway.
          Hide
          Lars Hofhansl added a comment -

          Ah no, I'd need the equivalent of getLargestReadPoint() and then could remove KVs never than that, might be too expensive to do anyway, though, as all active scanners need to be traversed.

          Show
          Lars Hofhansl added a comment - Ah no, I'd need the equivalent of getLargestReadPoint() and then could remove KVs never than that, might be too expensive to do anyway, though, as all active scanners need to be traversed.
          Hide
          Ted Yu added a comment -

          then could remove KVs never than that

          I guess the above should read 'then could remove KVs newer than that'

          Show
          Ted Yu added a comment - then could remove KVs never than that I guess the above should read 'then could remove KVs newer than that'
          Hide
          Lars Hofhansl added a comment -

          Yep. Thinking about it, though, that wouldn't be very good, because any scanner scanning any row would prevent the older from being cleaned from the memstore.

          Show
          Lars Hofhansl added a comment - Yep. Thinking about it, though, that wouldn't be very good, because any scanner scanning any row would prevent the older from being cleaned from the memstore.
          Hide
          Lars Hofhansl added a comment -

          There currently is no good solution for this.

          Show
          Lars Hofhansl added a comment - There currently is no good solution for this.
          Hide
          Lars Hofhansl added a comment -

          See discussion on HBASE-7051.
          I think if we wait for prior MVCC transactions to finish we can make this correct without setting the memstoreTS to 0

          Show
          Lars Hofhansl added a comment - See discussion on HBASE-7051 . I think if we wait for prior MVCC transactions to finish we can make this correct without setting the memstoreTS to 0
          Hide
          Lars Hofhansl added a comment - - edited

          Here's a pretty radical change!

          It removes Memstore.upsert completely along with all callers and all tests that were testing that functionality.

          Note that while upsert is no longer needed for correctness (it used to set the memstoreTS to 0 so that other increments/appends would be guaranteed to see the previous values), it was still a potential performance improvement because stale KVs were removed immediately.

          However, this actually used to violate the HBase VERSIONS contract.
          TestAtomicOperation.testIncrementMultiThreads is not measurably slower on my machine.

          With this patch Increment and Append are no longer special w.r.t. MVCC.

          Show
          Lars Hofhansl added a comment - - edited Here's a pretty radical change! It removes Memstore.upsert completely along with all callers and all tests that were testing that functionality. Note that while upsert is no longer needed for correctness (it used to set the memstoreTS to 0 so that other increments/appends would be guaranteed to see the previous values), it was still a potential performance improvement because stale KVs were removed immediately. However, this actually used to violate the HBase VERSIONS contract. TestAtomicOperation.testIncrementMultiThreads is not measurably slower on my machine. With this patch Increment and Append are no longer special w.r.t. MVCC.
          Hide
          Lars Hofhansl added a comment -

          Let's get a HadoopQA run.

          Show
          Lars Hofhansl added a comment - Let's get a HadoopQA run.
          Hide
          Lars Hofhansl added a comment -

          Not that Memstore.upsert could be made to honor MVCC by passing in the regions smallest readPoint of any scanner and only deleting KVs older than that - but that would still break the HBase contract that everything in HBase is versioned.

          Show
          Lars Hofhansl added a comment - Not that Memstore.upsert could be made to honor MVCC by passing in the regions smallest readPoint of any scanner and only deleting KVs older than that - but that would still break the HBase contract that everything in HBase is versioned.
          Hide
          Ted Yu added a comment -

          Looking at PreCommit build #3166, looks like there was compilation error against Hadoop 2.0

          Show
          Ted Yu added a comment - Looking at PreCommit build #3166, looks like there was compilation error against Hadoop 2.0
          Hide
          Lars Hofhansl added a comment -

          Hmm... The log output there is quite useless.

          Show
          Lars Hofhansl added a comment - Hmm... The log output there is quite useless.
          Hide
          Lars Hofhansl added a comment -

          Somehow the changes to TestMemstore did not make it into the patch. Should compile now.

          Show
          Lars Hofhansl added a comment - Somehow the changes to TestMemstore did not make it into the patch. Should compile now.
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

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

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

          -1 findbugs. The patch appears to introduce 3 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 passed unit tests in .

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3167//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3167//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3167//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3167//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3167//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3167//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3167//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/12551105/4583-trunk-radical_v2.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 7 new or modified tests. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 85 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 3 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 passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3167//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3167//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3167//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3167//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3167//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3167//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3167//console This message is automatically generated.
          Hide
          Lars Hofhansl added a comment -

          Correctness wise this looks pretty good and it does remove a lot of goofy code.

          Please have a careful look, this will impact Increment/Append performance as each increment will no longer remove the old KVs from the memstore (if it is still found in the memstore).

          But it will make it correct w.r.t. version and TTL handling and remove any special status from Increment and Append.

          Show
          Lars Hofhansl added a comment - Correctness wise this looks pretty good and it does remove a lot of goofy code. Please have a careful look, this will impact Increment/Append performance as each increment will no longer remove the old KVs from the memstore (if it is still found in the memstore). But it will make it correct w.r.t. version and TTL handling and remove any special status from Increment and Append.
          Hide
          Lars Hofhansl added a comment -

          This makes it even more like all other other operation: Use the existing applyFamilyMapToMemstore method to make the actual changes to the memstore.

          Show
          Lars Hofhansl added a comment - This makes it even more like all other other operation: Use the existing applyFamilyMapToMemstore method to make the actual changes to the memstore.
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

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

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

          -1 findbugs. The patch appears to introduce 3 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 passed unit tests in .

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3168//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3168//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3168//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3168//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3168//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3168//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3168//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/12551123/4583-trunk-v3.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 7 new or modified tests. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 85 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 3 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 passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3168//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3168//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3168//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3168//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3168//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3168//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3168//console This message is automatically generated.
          Hide
          Gregory Chanan added a comment -

          Patch looks good, +1.

          I don't think these need to be covered in this JIRA, but some things to consider:

          • Should we do the same for checkAndPut?
          • Ideally we would have some performance testing in this area. Like a YCSB job that does appends/increments in a uniform distribution. This may be significantly slower now?
          • Also some unit (correctness) testing would be good.
          Show
          Gregory Chanan added a comment - Patch looks good, +1. I don't think these need to be covered in this JIRA, but some things to consider: Should we do the same for checkAndPut? Ideally we would have some performance testing in this area. Like a YCSB job that does appends/increments in a uniform distribution. This may be significantly slower now? Also some unit (correctness) testing would be good.
          Hide
          Lars Hofhansl added a comment - - edited

          Yes, I think the same can work for checkAndXXX... In fact maybe now we can unify all of those. They all have 3 steps:

          1. precondition/get old values
          2. internal logic
          3. update values

          It might be possible to extract that boilerplate and pass in an implementation of an "AtomicOperation" interface or something.

          In terms of performance... It will be slower. It's no longer doing upserts (i.e. removing old KVs still found in the memstore).
          Personally I do not understand why Increment would be special here. All other operations work by creating new versions and they all could be sped up if we would remove stale versions from the memstore immediately. Why are Puts not doing upserts? And why would we care less about the history of an increment column?
          I think if we wanted an upsert type functionality it should be an option for every operation (except Delete, I guess). We'd declare we do not care about the history and then the operation could go through existing versions of KVs for which it can prove that no scanner is using them (by using the region's smallest readpoint).

          The only performance test I did was to run TestAtomicOperation#testIncrementMultiThreads, which is about 30% slower (6.5s vs 5s before). This is in part due to the MVCC enforcement and in part due to the fact the memstore will fill up sooner.

          Edit: Spelling

          Show
          Lars Hofhansl added a comment - - edited Yes, I think the same can work for checkAndXXX... In fact maybe now we can unify all of those. They all have 3 steps: precondition/get old values internal logic update values It might be possible to extract that boilerplate and pass in an implementation of an "AtomicOperation" interface or something. In terms of performance... It will be slower. It's no longer doing upserts (i.e. removing old KVs still found in the memstore). Personally I do not understand why Increment would be special here. All other operations work by creating new versions and they all could be sped up if we would remove stale versions from the memstore immediately. Why are Puts not doing upserts? And why would we care less about the history of an increment column? I think if we wanted an upsert type functionality it should be an option for every operation (except Delete, I guess). We'd declare we do not care about the history and then the operation could go through existing versions of KVs for which it can prove that no scanner is using them (by using the region's smallest readpoint). The only performance test I did was to run TestAtomicOperation#testIncrementMultiThreads, which is about 30% slower (6.5s vs 5s before). This is in part due to the MVCC enforcement and in part due to the fact the memstore will fill up sooner. Edit: Spelling
          Hide
          Lars Hofhansl added a comment -

          If I only add the MVCC waiting part to Increment I see about 10% decrease in performance.

          Show
          Lars Hofhansl added a comment - If I only add the MVCC waiting part to Increment I see about 10% decrease in performance.
          Hide
          Lars Hofhansl added a comment -

          Hmm... And if I set maxversions to 1 in the Region used by TestAtomicOperation#testIncrementMultiThreads I see a 15% decrease in performance only.
          (There's high variance in these number, though).

          Show
          Lars Hofhansl added a comment - Hmm... And if I set maxversions to 1 in the Region used by TestAtomicOperation#testIncrementMultiThreads I see a 15% decrease in performance only. (There's high variance in these number, though).
          Hide
          Lars Hofhansl added a comment -

          So the main question is: Keep the upsert functionality for Increment and make it MVCC aware? Or say that Increment is not special here and everything in HBase is versioned?

          Show
          Lars Hofhansl added a comment - So the main question is: Keep the upsert functionality for Increment and make it MVCC aware? Or say that Increment is not special here and everything in HBase is versioned?
          Hide
          Jean-Daniel Cryans added a comment -

          Why are Puts not doing upserts? And why would we care less about the history of an increment column?

          Most of the Increment workloads I know about are usually trying to push a lot more ops/sec than puts, which in turn generates both MemStore and HLog garbage. The way we currently upsert in the MemStore is saving us from flushing hundreds of GBs of crap than then needs to be compacted.

          As to if we should do upsert for Puts... good question, I guess we could permit that.

          Show
          Jean-Daniel Cryans added a comment - Why are Puts not doing upserts? And why would we care less about the history of an increment column? Most of the Increment workloads I know about are usually trying to push a lot more ops/sec than puts, which in turn generates both MemStore and HLog garbage. The way we currently upsert in the MemStore is saving us from flushing hundreds of GBs of crap than then needs to be compacted. As to if we should do upsert for Puts... good question, I guess we could permit that.
          Hide
          Gregory Chanan added a comment -

          I agree with your comments about making a common code path for the read/update operations and in there we could decide if we want to do upserts each time.

          The upsert argument is complicated because it's really a question of the read workload as much as the write workload, but we have to decide at write-time w/o much information (at least now as we don't collect statistics or anything). Off the top of my head I could see upserts being more useful for increments than puts, since increments are only holding numerical values (so old version x is probably just the current version minus the usual increment amount times x). Puts might have information you'd actually want to look at (job titles, times of last submitted help-desk tickets,etc). This is very hand-wavy though.

          So, I think I'd just keep the upsert functionality for now and make it MVCC aware. We can revisit if/when we unify the implementations.

          Show
          Gregory Chanan added a comment - I agree with your comments about making a common code path for the read/update operations and in there we could decide if we want to do upserts each time. The upsert argument is complicated because it's really a question of the read workload as much as the write workload, but we have to decide at write-time w/o much information (at least now as we don't collect statistics or anything). Off the top of my head I could see upserts being more useful for increments than puts, since increments are only holding numerical values (so old version x is probably just the current version minus the usual increment amount times x). Puts might have information you'd actually want to look at (job titles, times of last submitted help-desk tickets,etc). This is very hand-wavy though. So, I think I'd just keep the upsert functionality for now and make it MVCC aware. We can revisit if/when we unify the implementations.
          Hide
          Lars Hofhansl added a comment -

          Yeah... You guys are right.

          I was enjoying all that removed code.

          Show
          Lars Hofhansl added a comment - Yeah... You guys are right. I was enjoying all that removed code.
          Hide
          Lars Hofhansl added a comment -

          Trying a patch right now with upsert honoring MVCC. I get extreme lock contention right now (test that took 5s takes over 10m now)... Not sure what's going on, yet.

          Show
          Lars Hofhansl added a comment - Trying a patch right now with upsert honoring MVCC. I get extreme lock contention right now (test that took 5s takes over 10m now)... Not sure what's going on, yet.
          Hide
          Lars Hofhansl added a comment -

          Here's a less radical patch
          TestAtomicOperation also tests concurrency with Gets (which fails without the rest of this patch).
          The change in performance is within the noise. I suppose an unclosed scanner could cause a pathological situation where upsert wouldn't be able to remove older KVs, because it cannot prove that they are not accessed by a current scanner. When enforcing MVCC this is unavoidable, though.

          So I this patch is good.

          Show
          Lars Hofhansl added a comment - Here's a less radical patch TestAtomicOperation also tests concurrency with Gets (which fails without the rest of this patch). The change in performance is within the noise. I suppose an unclosed scanner could cause a pathological situation where upsert wouldn't be able to remove older KVs, because it cannot prove that they are not accessed by a current scanner. When enforcing MVCC this is unavoidable, though. So I this patch is good.
          Hide
          Lars Hofhansl added a comment -

          Jean-Daniel Cryans

          The way we currently upsert in the MemStore is saving us from flushing hundreds of GBs of crap than then needs to be compacted.

          That is actually not entirely true. With HBASE-4241 it would flush the store more frequently, but during flush older versions of KVs are ignored (i.e. not flushed at all, and thus they do not need to be compacted later). That's the reason why the "radical" patch is not 100x slower, but just 15% (with VERSIONS set to 1).

          Please have a look at the "less radical" patch.
          Because of HBASE-4241 I still prefer the "radical" version (trunk-v3). With many increments for a single row the search in the memstore for upsert might even become counter productive.

          Anyway... I'm fine with committing either of the two patches here. Both should be correct in terms of MVCC behavior.

          Show
          Lars Hofhansl added a comment - Jean-Daniel Cryans The way we currently upsert in the MemStore is saving us from flushing hundreds of GBs of crap than then needs to be compacted. That is actually not entirely true. With HBASE-4241 it would flush the store more frequently, but during flush older versions of KVs are ignored (i.e. not flushed at all, and thus they do not need to be compacted later). That's the reason why the "radical" patch is not 100x slower, but just 15% (with VERSIONS set to 1). Please have a look at the "less radical" patch. Because of HBASE-4241 I still prefer the "radical" version (trunk-v3). With many increments for a single row the search in the memstore for upsert might even become counter productive. Anyway... I'm fine with committing either of the two patches here. Both should be correct in terms of MVCC behavior.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12551310/4583-trunk-less-radical.txt
          against trunk revision .

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

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

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

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

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

          -1 findbugs. The patch appears to introduce 4 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.TestRegionServerCoprocessorExceptionWithAbort

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3179//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3179//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3179//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3179//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3179//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3179//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3179//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/12551310/4583-trunk-less-radical.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 85 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 4 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.TestRegionServerCoprocessorExceptionWithAbort Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3179//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3179//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3179//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3179//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3179//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3179//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3179//console This message is automatically generated.
          Hide
          ramkrishna.s.vasudevan added a comment -

          We'd declare we do not care about the history and then the operation could go through existing versions of KVs

          This could be done.
          +1 on doing the same for checkAndPut also.

          Show
          ramkrishna.s.vasudevan added a comment - We'd declare we do not care about the history and then the operation could go through existing versions of KVs This could be done. +1 on doing the same for checkAndPut also.
          Hide
          Lars Hofhansl added a comment -

          Cleaned up version of the less radical patch.
          Fixed some bugs in the existing upsert code on the way.

          This is the one I would like to commit soon.

          Show
          Lars Hofhansl added a comment - Cleaned up version of the less radical patch. Fixed some bugs in the existing upsert code on the way. This is the one I would like to commit soon.
          Hide
          Ted Yu added a comment -

          Nice effort, Lars.

          For 4583-trunk-less-radical.txt:

          +   * @return The smallest mvcc readPoint across all the scanners in this
          +   * region. Writes newer than this readPoint, are guaranteed not to be seen
          +   * by any current scanner.
          +   */
          +  public long getLargestReadPoint() {
          

          The first line of javadoc doesn't seem to match name of method - smallest in javadoc vs. largest in method name.
          For MemStore.java:

          +   * This now only used by tests.
          

          Insert an 'is' between This and now.

              * @param kvs
              * @return change in memstore size
              */
          -  public long upsert(Iterable<KeyValue> kvs) {
          +  public long upsert(Iterable<KeyValue> kvs, long readpoint) {
          

          Please add javadoc for new parameter readPoint. Same with upsert().

          +        if (cur.getType() == KeyValue.Type.Put.getCode() && cur.getMemstoreTS() < readpoint - 1) {
          

          Why do we need to subtract one from readPoint ? Considering less than is used already.

          Show
          Ted Yu added a comment - Nice effort, Lars. For 4583-trunk-less-radical.txt: + * @ return The smallest mvcc readPoint across all the scanners in this + * region. Writes newer than this readPoint, are guaranteed not to be seen + * by any current scanner. + */ + public long getLargestReadPoint() { The first line of javadoc doesn't seem to match name of method - smallest in javadoc vs. largest in method name. For MemStore.java: + * This now only used by tests. Insert an 'is' between This and now. * @param kvs * @ return change in memstore size */ - public long upsert(Iterable<KeyValue> kvs) { + public long upsert(Iterable<KeyValue> kvs, long readpoint) { Please add javadoc for new parameter readPoint. Same with upsert(). + if (cur.getType() == KeyValue.Type.Put.getCode() && cur.getMemstoreTS() < readpoint - 1) { Why do we need to subtract one from readPoint ? Considering less than is used already.
          Hide
          Lars Hofhansl added a comment -

          Hey Ted... Thanks. You looked at an older patch
          Please see 4583-trunk-less-radical-v2.txt

          Show
          Lars Hofhansl added a comment - Hey Ted... Thanks. You looked at an older patch Please see 4583-trunk-less-radical-v2.txt
          Hide
          Lars Hofhansl added a comment -

          I guess v2 and you comment were posted at exactly the same time

          Show
          Lars Hofhansl added a comment - I guess v2 and you comment were posted at exactly the same time
          Hide
          Ted Yu added a comment -

          I did look at 4583-trunk-less-radical-v2.txt
          Can you enlighten me on my last question above ?

          Show
          Ted Yu added a comment - I did look at 4583-trunk-less-radical-v2.txt Can you enlighten me on my last question above ?
          Hide
          Gregory Chanan added a comment -

          Taking a look now.

          What made the performance within the noise now when it was so bad before?

          Show
          Gregory Chanan added a comment - Taking a look now. What made the performance within the noise now when it was so bad before?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12551392/4583-trunk-less-radical-v2.txt
          against trunk revision .

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

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

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

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

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

          -1 findbugs. The patch appears to introduce 3 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.io.hfile.TestForceCacheImportantBlocks

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3181//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3181//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3181//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3181//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3181//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3181//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3181//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/12551392/4583-trunk-less-radical-v2.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 85 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 3 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.io.hfile.TestForceCacheImportantBlocks Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3181//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3181//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3181//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3181//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3181//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3181//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3181//console This message is automatically generated.
          Hide
          Gregory Chanan added a comment -

          New test for increment looks good. Should we have a similar one for append as well?

          I also have the same question as Ted about "readpoint -1".

                 // create or update (upsert) a new KeyValue with
                 // 'now' and a 0 memstoreTS == immediately visible
                 return upsert(Arrays.asList(
          -          new KeyValue(row, family, qualifier, now, Bytes.toBytes(newValue)))
          +          new KeyValue(row, family, qualifier, now, Bytes.toBytes(newValue))), 1L
          

          it looks from here like we will never remove anything via updateColumnValue, because we would need cur.getMemstoreTS() < readpoint - 1 => cur.getMemstoreTS() < 1-1 => cur.getMemstoreTS() < 0? Why do we pass 1 in the above?

          I also notice in append/increment we call completeMemstoreInsert before we sync to the WAL, while other calls like put do it after. Is it safe to do it before? Why doesn't put do it that way, then?

          Show
          Gregory Chanan added a comment - New test for increment looks good. Should we have a similar one for append as well? I also have the same question as Ted about "readpoint -1". // create or update (upsert) a new KeyValue with // 'now' and a 0 memstoreTS == immediately visible return upsert(Arrays.asList( - new KeyValue(row, family, qualifier, now, Bytes.toBytes(newValue))) + new KeyValue(row, family, qualifier, now, Bytes.toBytes(newValue))), 1L it looks from here like we will never remove anything via updateColumnValue, because we would need cur.getMemstoreTS() < readpoint - 1 => cur.getMemstoreTS() < 1-1 => cur.getMemstoreTS() < 0? Why do we pass 1 in the above? I also notice in append/increment we call completeMemstoreInsert before we sync to the WAL, while other calls like put do it after. Is it safe to do it before? Why doesn't put do it that way, then?
          Hide
          Gregory Chanan added a comment -

          Thinking about completeMemstoreInsert again, it seems wrong to have it before we sync to the WAL. Otherwise, a read could see the new data, a crash could occur before the WAL sync finishes, and recovery would lose the data.

          Show
          Gregory Chanan added a comment - Thinking about completeMemstoreInsert again, it seems wrong to have it before we sync to the WAL. Otherwise, a read could see the new data, a crash could occur before the WAL sync finishes, and recovery would lose the data.
          Hide
          Lars Hofhansl added a comment -

          @Ted: As for readpoint - 1. Now that you ask, I found this through long debugging. I am not actually sure I can explain that right now.

          @Gregory: My initial code had a bug where it did not remove the old values at all, and hence it not only exploded the memstore, it also churned through all KVs every single time to see if some of them can be removed.

          Show
          Lars Hofhansl added a comment - @Ted: As for readpoint - 1. Now that you ask, I found this through long debugging. I am not actually sure I can explain that right now. @Gregory: My initial code had a bug where it did not remove the old values at all, and hence it not only exploded the memstore, it also churned through all KVs every single time to see if some of them can be removed.
          Hide
          Jean-Daniel Cryans added a comment -

          Patch looks good although I'm not too familiar with that part of the code.

          I tested it locally and it doesn't really seem to have a big impact on performance (3 threads pounding 1 row with ICVs).

          I'm not sure about putting this into 0.94 tho, we're already pretty late in the point releases for changes like this one.

          Show
          Jean-Daniel Cryans added a comment - Patch looks good although I'm not too familiar with that part of the code. I tested it locally and it doesn't really seem to have a big impact on performance (3 threads pounding 1 row with ICVs). I'm not sure about putting this into 0.94 tho, we're already pretty late in the point releases for changes like this one.
          Hide
          Lars Hofhansl added a comment -

          I was checking out why readpoint - 1 is needed. Turns out the patch is not entirely correct.

          The new KV is newer than the current readpoint (by definition, because it is written from a new transaction). A scanner running at the current readpoint, thus, will not see the new KV, but rather the older one. That means that we can only remove older KVs if there is a newer one that is still older than the current readpoint (which means all new scanner will see that one).
          readpoint - 1 fixed it for my test case, but it is not generally correct.

          Show
          Lars Hofhansl added a comment - I was checking out why readpoint - 1 is needed. Turns out the patch is not entirely correct. The new KV is newer than the current readpoint (by definition, because it is written from a new transaction). A scanner running at the current readpoint, thus, will not see the new KV, but rather the older one. That means that we can only remove older KVs if there is a newer one that is still older than the current readpoint (which means all new scanner will see that one). readpoint - 1 fixed it for my test case, but it is not generally correct.
          Hide
          Lars Hofhansl added a comment -

          This patch is doing the right thing (TM).
          A KV is removable if it is not the newest one older than that smallest readpoint.

          I.e. no current scanner sees that KV.

          Show
          Lars Hofhansl added a comment - This patch is doing the right thing (TM). A KV is removable if it is not the newest one older than that smallest readpoint. I.e. no current scanner sees that KV.
          Hide
          Lars Hofhansl added a comment -

          @Gregory: updateColumnValue is only used in tests... It should just be removed, but there're tests using it still.
          And you're right the memstore transaction should not be completed before the WAL is sync'ed.

          Update coming.

          Show
          Lars Hofhansl added a comment - @Gregory: updateColumnValue is only used in tests... It should just be removed, but there're tests using it still. And you're right the memstore transaction should not be completed before the WAL is sync'ed. Update coming.
          Hide
          Lars Hofhansl added a comment -

          Does completeMemstoreInsert after the WAL sync. (good catch Gregory).
          Unfortunately the only way to do this without more restructuring is another try/catch wrapper.
          As for updateColumnValue... All the test there make the call directly, there is no region to derive a readpoint from. No not actually deleting anything is the best approach.

          Show
          Lars Hofhansl added a comment - Does completeMemstoreInsert after the WAL sync. (good catch Gregory). Unfortunately the only way to do this without more restructuring is another try/catch wrapper. As for updateColumnValue... All the test there make the call directly, there is no region to derive a readpoint from. No not actually deleting anything is the best approach.
          Hide
          Gregory Chanan added a comment -

          append looks like it calls completeMemstoreInsert twice (once before sync, once after)

          Show
          Gregory Chanan added a comment - append looks like it calls completeMemstoreInsert twice (once before sync, once after)
          Hide
          Lars Hofhansl added a comment -

          Ah crap... I'm tired today. Fixed.

          Show
          Lars Hofhansl added a comment - Ah crap... I'm tired today. Fixed.
          Hide
          Ted Yu added a comment -

          Putting the patch on review board, I was able to understand the changes better
          completeMemstoreInsert() doesn't throw exception:

            public void completeMemstoreInsert(WriteEntry e) {
          

          I wonder if the last mvcc.completeMemstoreInsert(w) call can be latched onto (before) closeRegionOperation():

          +        mvcc.completeMemstoreInsert(w);        
                 }
          -      if (writeToWAL) {
          -        syncOrDefer(txid); // sync the transaction log outside the rowlock
          -      }
               } finally {
                 closeRegionOperation();
          

          This way there is no need to add one more try / catch block.
          There are also some white spaces introduced in patch v5.

          Show
          Ted Yu added a comment - Putting the patch on review board, I was able to understand the changes better completeMemstoreInsert() doesn't throw exception: public void completeMemstoreInsert(WriteEntry e) { I wonder if the last mvcc.completeMemstoreInsert(w) call can be latched onto (before) closeRegionOperation(): + mvcc.completeMemstoreInsert(w); } - if (writeToWAL) { - syncOrDefer(txid); // sync the transaction log outside the rowlock - } } finally { closeRegionOperation(); This way there is no need to add one more try / catch block. There are also some white spaces introduced in patch v5.
          Hide
          Lars Hofhansl added a comment -

          @Ted: I was thinking about that. It would have meant to put mvcc.beginMemstoreInsert up and out of the first try/catch block, and since we have to do this after the lock was acquired, we'd have to move that up too.

          Show
          Lars Hofhansl added a comment - @Ted: I was thinking about that. It would have meant to put mvcc.beginMemstoreInsert up and out of the first try/catch block, and since we have to do this after the lock was acquired, we'd have to move that up too.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12551452/4583-trunk-less-radical-v4.txt
          against trunk revision .

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

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

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

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

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

          -1 findbugs. The patch appears to introduce 4 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.client.TestMultiParallel
          org.apache.hadoop.hbase.regionserver.TestAtomicOperation
          org.apache.hadoop.hbase.client.TestFromClientSide
          org.apache.hadoop.hbase.client.TestFromClientSideWithCoprocessor
          org.apache.hadoop.hbase.regionserver.TestHRegion
          org.apache.hadoop.hbase.regionserver.TestRegionServerMetrics

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3190//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3190//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3190//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3190//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3190//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3190//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3190//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/12551452/4583-trunk-less-radical-v4.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 85 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 4 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.client.TestMultiParallel org.apache.hadoop.hbase.regionserver.TestAtomicOperation org.apache.hadoop.hbase.client.TestFromClientSide org.apache.hadoop.hbase.client.TestFromClientSideWithCoprocessor org.apache.hadoop.hbase.regionserver.TestHRegion org.apache.hadoop.hbase.regionserver.TestRegionServerMetrics Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3190//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3190//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3190//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3190//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3190//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3190//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3190//console This message is automatically generated.
          Hide
          Gregory Chanan added a comment -

          #1

          // only remove Puts that concurrent scanners cannot possibly see (readpoint - 1)

          the "readpoint -1" stuff is out of date I think.

          #2 I'm fine with updateColumnValue if it's only used in tests and the tests pass.

          #3 I'll file some JIRAs for adding a test for append (similar to your test for increment) and for doing what you have done here for checkAndPut. checkAndPut should be simple because I don't think it upserts.

          #4 We may also need to a test to check that append/increment properly wait for all writes to complete. Your test, as best as I can tell, checks that the upsert behaves properly with scanners, but the waiting for all writes to complete is sort of a separate issue. Something like what I suggested over in HBASE-7051:
          current cell value is 10. Do a put of 15 and an increment by 1. Only acceptable answers are 15 and 16, but without this patch, we can get 11.
          Again, this can be handled in a separate JIRA.

          #5 I'm fine with this for 0.96. What do you think about 0.94? JD makes a good point that we are far along to have such a big change, but on the other hand, atomic issues are pretty serious. Wondering what your thoughts are. I'd at least want to do some of my own performance testing before putting it in 0.94.

          Show
          Gregory Chanan added a comment - #1 // only remove Puts that concurrent scanners cannot possibly see (readpoint - 1) the "readpoint -1" stuff is out of date I think. #2 I'm fine with updateColumnValue if it's only used in tests and the tests pass. #3 I'll file some JIRAs for adding a test for append (similar to your test for increment) and for doing what you have done here for checkAndPut. checkAndPut should be simple because I don't think it upserts. #4 We may also need to a test to check that append/increment properly wait for all writes to complete. Your test, as best as I can tell, checks that the upsert behaves properly with scanners, but the waiting for all writes to complete is sort of a separate issue. Something like what I suggested over in HBASE-7051 : current cell value is 10. Do a put of 15 and an increment by 1. Only acceptable answers are 15 and 16, but without this patch, we can get 11. Again, this can be handled in a separate JIRA. #5 I'm fine with this for 0.96. What do you think about 0.94? JD makes a good point that we are far along to have such a big change, but on the other hand, atomic issues are pretty serious. Wondering what your thoughts are. I'd at least want to do some of my own performance testing before putting it in 0.94.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12551462/4583-trunk-less-radical-v5.txt
          against trunk revision .

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

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

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

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

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

          -1 findbugs. The patch appears to introduce 5 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 passed unit tests in .

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3192//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3192//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3192//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3192//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3192//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3192//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3192//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/12551462/4583-trunk-less-radical-v5.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 85 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 5 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 passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3192//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3192//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3192//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3192//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3192//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3192//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3192//console This message is automatically generated.
          Hide
          Lars Hofhansl added a comment -

          #1 Yeah, old comment.
          #2 Should refactor the tests to call Upsert on Store and Memstore directly rather than this - now defunct - method. The patch at least removes it from the public API on both cases.
          #3 I should add a test for Append. Bit more tricky than Increment as you get ever increasing values (in terms of size)
          #4 the test validates that. TestAtomicOperation would not return correct results without the Increments waiting for all previous changes. (before this patch this was handled by forcing the memTSs of all KV to 0 immediately)
          #5 I'm fine either here, actually. Definitely 0.96, +0 on 0.94.

          Show
          Lars Hofhansl added a comment - #1 Yeah, old comment. #2 Should refactor the tests to call Upsert on Store and Memstore directly rather than this - now defunct - method. The patch at least removes it from the public API on both cases. #3 I should add a test for Append. Bit more tricky than Increment as you get ever increasing values (in terms of size) #4 the test validates that. TestAtomicOperation would not return correct results without the Increments waiting for all previous changes. (before this patch this was handled by forcing the memTSs of all KV to 0 immediately) #5 I'm fine either here, actually. Definitely 0.96, +0 on 0.94.
          Hide
          Lars Hofhansl added a comment -

          Another interesting thought: If in addition to counting all the KVs older than the readpoint I also count all versions of the KVs seen so far we can use this for "memstore compactions". I.e. each memstore change would be an upsert that would add the new KVs and then remove all KVs that not be flushed to disk anyway.

          Show
          Lars Hofhansl added a comment - Another interesting thought: If in addition to counting all the KVs older than the readpoint I also count all versions of the KVs seen so far we can use this for "memstore compactions". I.e. each memstore change would be an upsert that would add the new KVs and then remove all KVs that not be flushed to disk anyway.
          Hide
          Lars Hofhansl added a comment -

          Alas, that would not work nicely with mslab.

          Jean-Daniel Cryans I was wondering whether you could do your same performance test with the "radical" version of this patch (4583-trunk-v3.txt)?

          Show
          Lars Hofhansl added a comment - Alas, that would not work nicely with mslab. Jean-Daniel Cryans I was wondering whether you could do your same performance test with the "radical" version of this patch (4583-trunk-v3.txt)?
          Hide
          Lars Hofhansl added a comment -

          This is hopefully the last version here.
          Add a test for Append.

          I tried to remove updateColumnValue, but the tests are in part testing specific updateColumnValue functionality, which is hard to separate from the upsert tests. That is for another jira, I think.

          Show
          Lars Hofhansl added a comment - This is hopefully the last version here. Add a test for Append. I tried to remove updateColumnValue, but the tests are in part testing specific updateColumnValue functionality, which is hard to separate from the upsert tests. That is for another jira, I think.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12551483/4583-trunk-less-radical-v6.txt
          against trunk revision .

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

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

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

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

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

          -1 findbugs. The patch appears to introduce 5 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 passed unit tests in .

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3196//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3196//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3196//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3196//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3196//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3196//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3196//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/12551483/4583-trunk-less-radical-v6.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 85 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 5 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 passed unit tests in . Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3196//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3196//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3196//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3196//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3196//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3196//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3196//console This message is automatically generated.
          Hide
          Jean-Daniel Cryans added a comment -

          Jean-Daniel Cryans I was wondering whether you could do your same performance test with the "radical" version of this patch (4583-trunk-v3.txt)?

          What I saw is it's 10% slower, but also that it was flushing tiny HFiles.

          Show
          Jean-Daniel Cryans added a comment - Jean-Daniel Cryans I was wondering whether you could do your same performance test with the "radical" version of this patch (4583-trunk-v3.txt)? What I saw is it's 10% slower, but also that it was flushing tiny HFiles.
          Hide
          Lars Hofhansl added a comment -

          You guys used this more extensively at SU than anybody else, I think. Would you stick to your statement that the upsert complexity and breaking of the general HBase VERSIONS contract is warranted for the performance gain?

          It would probably be am even bigger problem if there's other load (that fills the memstore) that now unnecessarily needs to be flushed because of many increments.

          Show
          Lars Hofhansl added a comment - You guys used this more extensively at SU than anybody else, I think. Would you stick to your statement that the upsert complexity and breaking of the general HBase VERSIONS contract is warranted for the performance gain? It would probably be am even bigger problem if there's other load (that fills the memstore) that now unnecessarily needs to be flushed because of many increments.
          Hide
          Jean-Daniel Cryans added a comment -

          You guys used this more extensively at SU than anybody else, I think. Would you stick to your statement that the upsert complexity and breaking of the general HBase VERSIONS contract is warranted for the performance gain?

          We're kind of dependent of the current performance now, unless we start double counting or missing increments then statu quo I'd say is preferred. It's still a bit unclear to me what the implications of not having this jira mean. And also what strick and non-strict means.

          Let me phrase it like this then: I think everyone can agree that we don't want data loss (eg no missed increments), so taking a performance hit is likely necessary. If it's only a case where a client reading a counter would miss the most updated values, then that's something at least I can live with.

          Show
          Jean-Daniel Cryans added a comment - You guys used this more extensively at SU than anybody else, I think. Would you stick to your statement that the upsert complexity and breaking of the general HBase VERSIONS contract is warranted for the performance gain? We're kind of dependent of the current performance now, unless we start double counting or missing increments then statu quo I'd say is preferred. It's still a bit unclear to me what the implications of not having this jira mean. And also what strick and non-strict means. Let me phrase it like this then: I think everyone can agree that we don't want data loss (eg no missed increments), so taking a performance hit is likely necessary. If it's only a case where a client reading a counter would miss the most updated values, then that's something at least I can live with.
          Hide
          Lars Hofhansl added a comment -

          The problem this fixes is how Increments/Appends interact with other operations.
          There are two problems:

          1. A race between a prior Put and a following Increment. The Increment might not see the latest state generated by the Put.
          2. An increment that updates multiple columns on a row can cause a concurrent scanner to see partially applied rows. The specifics here are that in order to make Increments strictly serializable upsert used to set the memstoreTS to 0, making the change visible immediately to all concurrent and future scanners.

          As long as only increments are used and you do not care for partially applied rows, the current code is correct.

          #1 is fixed by having the Increment wait for prior MVCC transactions with the lock held.
          #2 is fixed by either getting rid of upsert altogether (the "radical" version of this patch, which incurs the 10% overhead you measured) or make upsert MVCC aware (the "less radical" version of this patch, which should only incur minimal overhead).

          The objective of the "radical" patch was to fix these problems and to also get rid of a lot of code specific to increment (and which also breaks the VERSIONS contract).
          The "less radical" patch fixes the issues by fixing upsert to be MVCC aware.

          TL;DR: I'm happy with either patch, slightly leaning towards the "radical" version. But I think you're right that the current Increment performance is now expected, so the "less radical" version is probably the way to go.

          Show
          Lars Hofhansl added a comment - The problem this fixes is how Increments/Appends interact with other operations. There are two problems: A race between a prior Put and a following Increment. The Increment might not see the latest state generated by the Put. An increment that updates multiple columns on a row can cause a concurrent scanner to see partially applied rows. The specifics here are that in order to make Increments strictly serializable upsert used to set the memstoreTS to 0, making the change visible immediately to all concurrent and future scanners. As long as only increments are used and you do not care for partially applied rows, the current code is correct. #1 is fixed by having the Increment wait for prior MVCC transactions with the lock held. #2 is fixed by either getting rid of upsert altogether (the "radical" version of this patch, which incurs the 10% overhead you measured) or make upsert MVCC aware (the "less radical" version of this patch, which should only incur minimal overhead). The objective of the "radical" patch was to fix these problems and to also get rid of a lot of code specific to increment (and which also breaks the VERSIONS contract). The "less radical" patch fixes the issues by fixing upsert to be MVCC aware. TL;DR: I'm happy with either patch, slightly leaning towards the "radical" version. But I think you're right that the current Increment performance is now expected, so the "less radical" version is probably the way to go.
          Hide
          Jean-Daniel Cryans added a comment -

          Wonderful write-up, thanks for taking the time to do it Lars.

          I'm not too worried about #1, it seems like a weird use case but it looks like it's easily fixed.

          About #2, that's more important... I think we should always be correct, but I would skip putting this in 0.94

          Show
          Jean-Daniel Cryans added a comment - Wonderful write-up, thanks for taking the time to do it Lars. I'm not too worried about #1, it seems like a weird use case but it looks like it's easily fixed. About #2, that's more important... I think we should always be correct, but I would skip putting this in 0.94
          Hide
          Gregory Chanan added a comment -

          Great write-up, Lars.

          I'm leaning towards the "less radical version" at this point. 10% is right on the border of what I would consider acceptable without a way for the user to opt out (why wouldn't they opt out? if they care about breaking the VERSIONS contract I guess).

          Is there a sensible API change we could make here or a sensible change of the VERSIONS definition? We are just doing a write on a single row with "logical" deletes (not via delete markers). It seems like we should be able to fit this in our model.

          Show
          Gregory Chanan added a comment - Great write-up, Lars. I'm leaning towards the "less radical version" at this point. 10% is right on the border of what I would consider acceptable without a way for the user to opt out (why wouldn't they opt out? if they care about breaking the VERSIONS contract I guess). Is there a sensible API change we could make here or a sensible change of the VERSIONS definition? We are just doing a write on a single row with "logical" deletes (not via delete markers). It seems like we should be able to fit this in our model.
          Hide
          Lars Hofhansl added a comment -

          The logic in upsert could be changed to also count the number of versions (in addition to versions older than then the current readpoint) and then consider both counts before removing KVs. That way we get the current upsert logic (if you set VERSIONS => 1 for the CF) and also keep at least as many versions as declared in the CF.
          That's for another jira, though.

          OK. Any opposition to committing the "less radical" version to 0.96?

          Show
          Lars Hofhansl added a comment - The logic in upsert could be changed to also count the number of versions (in addition to versions older than then the current readpoint) and then consider both counts before removing KVs. That way we get the current upsert logic (if you set VERSIONS => 1 for the CF) and also keep at least as many versions as declared in the CF. That's for another jira, though. OK. Any opposition to committing the "less radical" version to 0.96?
          Hide
          Varun Sharma added a comment -

          Hi folks,

          Firstly, awesome to have this patch. Thanks, lars and everyone.

          We have been trying to build a consistent application on top of hbase which also works well with counters - consider a user and all his liked "pins" or "tweets". Row is User Id and liked pins are column qualifiers. Also, there is a column maintaining the "count". For maintaining consistent data, it would be nice if we could do the following - write a like and increment the count and delete a like and decrement the count.

          I am wondering if we can now couple "put(s)" and "increment(s)" or "delete(s)" and "decrement(s)" from the client side, as part of a single row mutation and ensure a consistent table view for the user in the above application (it will be of course be a separate JIRA, though).

          Thanks
          Varun

          Show
          Varun Sharma added a comment - Hi folks, Firstly, awesome to have this patch. Thanks, lars and everyone. We have been trying to build a consistent application on top of hbase which also works well with counters - consider a user and all his liked "pins" or "tweets". Row is User Id and liked pins are column qualifiers. Also, there is a column maintaining the "count". For maintaining consistent data, it would be nice if we could do the following - write a like and increment the count and delete a like and decrement the count. I am wondering if we can now couple "put(s)" and "increment(s)" or "delete(s)" and "decrement(s)" from the client side, as part of a single row mutation and ensure a consistent table view for the user in the above application (it will be of course be a separate JIRA, though). Thanks Varun
          Hide
          Lars Hofhansl added a comment -

          RowMutation is currently limited to Puts and Deletes. Generalizing this is not trivial:

          • You have make all the changes to the WAL first, sync the WAL, then change the memstore
          • Potentially you want to release the row lock before the WAL-sync, which mean a rollback phase to the memstore if the WAL sync failed, etc.
          • Puts and Deletes only need snapshot isolation for consistency, whereas Increment and Append need to be serializable.
          • Put/Delete/etc are idempotent (client can retry on error) whereas Incement/Append generally aren't (we could make so by passing tokens along).

          Long way of saying: It's possible, but maybe not as simple as might imagine.

          Show
          Lars Hofhansl added a comment - RowMutation is currently limited to Puts and Deletes. Generalizing this is not trivial: You have make all the changes to the WAL first, sync the WAL, then change the memstore Potentially you want to release the row lock before the WAL-sync, which mean a rollback phase to the memstore if the WAL sync failed, etc. Puts and Deletes only need snapshot isolation for consistency, whereas Increment and Append need to be serializable. Put/Delete/etc are idempotent (client can retry on error) whereas Incement/Append generally aren't (we could make so by passing tokens along). Long way of saying: It's possible, but maybe not as simple as might imagine.
          Hide
          Varun Sharma added a comment -

          Agreed - this is not simple. In fact, the complexity within this JIRA itself shows that MVCC'ing the increment has itself issues.

          Thanks for pointing out the above concerns. Lets forget concern #4 since that is an issue irrespective of whether the increment is called within a mutation or outside a mutation, you can have increments applied twice, thrice etc.

          I was thinking that we could potentially do this in steps.
          1) Class IncrementMutation which inherits from Increment and Mutation
          2) In the mutateRow call, we add a case for "IncrementMutation" object
          3) Factor out the code wrapped inside the "lock and MVCC" from increment() function to internalIncrement.
          4) Call internalIncrement from mutateRow and increment()

          It seems that walEdits for Increment and Append are indeed put(s) - so that might be okay. Shall I move this discussion over to another JIRA ?

          Show
          Varun Sharma added a comment - Agreed - this is not simple. In fact, the complexity within this JIRA itself shows that MVCC'ing the increment has itself issues. Thanks for pointing out the above concerns. Lets forget concern #4 since that is an issue irrespective of whether the increment is called within a mutation or outside a mutation, you can have increments applied twice, thrice etc. I was thinking that we could potentially do this in steps. 1) Class IncrementMutation which inherits from Increment and Mutation 2) In the mutateRow call, we add a case for "IncrementMutation" object 3) Factor out the code wrapped inside the "lock and MVCC" from increment() function to internalIncrement. 4) Call internalIncrement from mutateRow and increment() It seems that walEdits for Increment and Append are indeed put(s) - so that might be okay. Shall I move this discussion over to another JIRA ?
          Hide
          Lars Hofhansl added a comment -

          What I meant with the fourth issue is that if a RowMutation contains an Increment/Append it is no longer repeatable.
          Yes, let's move this into a different jira, probably against 0.96 only.

          Show
          Lars Hofhansl added a comment - What I meant with the fourth issue is that if a RowMutation contains an Increment/Append it is no longer repeatable. Yes, let's move this into a different jira, probably against 0.96 only.
          Hide
          Varun Sharma added a comment -

          Okay - created JIRA

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

          This might sound like a naive question though but why are mutations required to be idempotent - is it so that their result is always gauranteed (feel free to discuss over the other JIRA) ?

          Show
          Varun Sharma added a comment - Okay - created JIRA https://issues.apache.org/jira/browse/HBASE-7093 This might sound like a naive question though but why are mutations required to be idempotent - is it so that their result is always gauranteed (feel free to discuss over the other JIRA) ?
          Hide
          Lars Hofhansl added a comment -

          Typically the client will retry an operation. That can happen for example when a region moved or is moving. In that case an Increment will just fail, whereas a Put/Delete will be transparently retried by a client.
          As I said, we can make Increment idempotent too (in a sense) by having the client send a token along and then verifying (somehow, waving hands here) that token at the server to apply the Increment at most once.

          Show
          Lars Hofhansl added a comment - Typically the client will retry an operation. That can happen for example when a region moved or is moving. In that case an Increment will just fail, whereas a Put/Delete will be transparently retried by a client. As I said, we can make Increment idempotent too (in a sense) by having the client send a token along and then verifying (somehow, waving hands here) that token at the server to apply the Increment at most once.
          Hide
          Varun Sharma added a comment -

          I had a quick follow up question on the less radical patch - particularly when we have multi-column increment operation. When we apply an upsert with the correct MVCC read point, does upsert override the previous keyvalue ? In that case, is the previous keyvalue lost ?

          Varun

          Show
          Varun Sharma added a comment - I had a quick follow up question on the less radical patch - particularly when we have multi-column increment operation. When we apply an upsert with the correct MVCC read point, does upsert override the previous keyvalue ? In that case, is the previous keyvalue lost ? Varun
          Hide
          Lars Hofhansl added a comment -

          It only "replaces" (it actually add a KV and then removes the old one), if it can prove that no scanner can (or will) see the old KV. The previous KV is lost (that is also what currently happens), only the "radical" patch will fix that.

          Show
          Lars Hofhansl added a comment - It only "replaces" (it actually add a KV and then removes the old one), if it can prove that no scanner can (or will) see the old KV. The previous KV is lost (that is also what currently happens), only the "radical" patch will fix that.
          Hide
          Lars Hofhansl added a comment -

          If there're no objections I'll commit the "less radical" version to 0.96 tomorrow.

          But not after I mention another advantage of the "radical" version:
          It will work fine with mslab!
          mslab is not used for any KV added or deleted via upsert (that is true before and after this patch, and is due to the fact that upsert will fragment the slab quickly leading to OOMs).
          The radical patch is more friendly to the GC.

          Show
          Lars Hofhansl added a comment - If there're no objections I'll commit the "less radical" version to 0.96 tomorrow. But not after I mention another advantage of the "radical" version: It will work fine with mslab! mslab is not used for any KV added or deleted via upsert (that is true before and after this patch, and is due to the fact that upsert will fragment the slab quickly leading to OOMs). The radical patch is more friendly to the GC.
          Hide
          Jean-Daniel Cryans added a comment -

          In my latest comment I meant to say that I'm +1 on the more radical patch for 0.96

          Show
          Jean-Daniel Cryans added a comment - In my latest comment I meant to say that I'm +1 on the more radical patch for 0.96
          Hide
          stack added a comment -

          I'd be +1 for radical patch in 0.96.

          Show
          stack added a comment - I'd be +1 for radical patch in 0.96.
          Hide
          stack added a comment -

          I think I understand the implications – noticeably slower (TBD) – but more obviously correct and for those who come after, more obvious what the right way (correctness) forward is. Looking at the patch, do we suffer version explosion (I don't see memstore cleanup going on).

          Show
          stack added a comment - I think I understand the implications – noticeably slower (TBD) – but more obviously correct and for those who come after, more obvious what the right way (correctness) forward is. Looking at the patch, do we suffer version explosion (I don't see memstore cleanup going on).
          Hide
          Lars Hofhansl added a comment -

          Yes, the "radical" version has no cleanup (that is what upsert does). Not cleaning up makes it slab friendly and also fullfills the VERSIONS contracts, but it will lead to more frequent region flushes (at flush time most of the KVs will be removed, but it will flush a tiny HFile).

          Show
          Lars Hofhansl added a comment - Yes, the "radical" version has no cleanup (that is what upsert does). Not cleaning up makes it slab friendly and also fullfills the VERSIONS contracts, but it will lead to more frequent region flushes (at flush time most of the KVs will be removed, but it will flush a tiny HFile).
          Hide
          stack added a comment -

          How would we do cleanup? Delete from behind smallest read point? Do this in a memstore compressor step? Something that sits between current memstore and the snapshot we make when flushing?

          Show
          stack added a comment - How would we do cleanup? Delete from behind smallest read point? Do this in a memstore compressor step? Something that sits between current memstore and the snapshot we make when flushing?
          Hide
          Varun Sharma added a comment -

          I am more +1 on the less radical patch - the more frequent flushing and tiny HFiles means that counters are now using a much larger fraction of the memstore as opposed to before. Also, more hfiles means that data will be more fragmented on disk until compaction happens.

          Show
          Varun Sharma added a comment - I am more +1 on the less radical patch - the more frequent flushing and tiny HFiles means that counters are now using a much larger fraction of the memstore as opposed to before. Also, more hfiles means that data will be more fragmented on disk until compaction happens.
          Hide
          Lars Hofhansl added a comment -

          Stack That's what the less radical patch does, it removed old versions of the KVs when they are no longer visible to concurrent scanners.
          You cannot clean up the in memory KVs (without a lot of refactoring and repacking into new slabs) while still using mslab.
          There was talk about in memory compactions that could something like this... If we consider this an issue then that would be the proper route.

          Let me try to summarize:

          1. The "less radical" patch is correct w.r.t. to MVCC. It does not correct the Increment behavior when it comes to historical scans. For practical purposes there is only a single version of the Incremented column, which is changed - regardless of how the CF is configured.
          2. The "radical" removes upsert. Increments are just treated like Puts, all special code is removed.
            Upon flush all excess versions are removed before they are flushed to disk (HBASE-4241). The flushed files will be small, compaction will be fast. No attempt is made to clean up KVs before that, so it works with mslab, but the memstore will fill up more quickly. This patch causes a 10-15% performance degradation for pure Increments.

          As I stated before, in my mind only the "radical" version is true to HBase's design and upsert was a hack, which should be removed.
          However, I'm fine committing the "less radical" version, which retains (mostly) the current performance and fixes the behavior w.r.t. MVCC.

          Show
          Lars Hofhansl added a comment - Stack That's what the less radical patch does, it removed old versions of the KVs when they are no longer visible to concurrent scanners. You cannot clean up the in memory KVs (without a lot of refactoring and repacking into new slabs) while still using mslab. There was talk about in memory compactions that could something like this... If we consider this an issue then that would be the proper route. Let me try to summarize: The "less radical" patch is correct w.r.t. to MVCC. It does not correct the Increment behavior when it comes to historical scans. For practical purposes there is only a single version of the Incremented column, which is changed - regardless of how the CF is configured. The "radical" removes upsert. Increments are just treated like Puts, all special code is removed. Upon flush all excess versions are removed before they are flushed to disk ( HBASE-4241 ). The flushed files will be small, compaction will be fast. No attempt is made to clean up KVs before that, so it works with mslab, but the memstore will fill up more quickly. This patch causes a 10-15% performance degradation for pure Increments. As I stated before, in my mind only the "radical" version is true to HBase's design and upsert was a hack, which should be removed. However, I'm fine committing the "less radical" version, which retains (mostly) the current performance and fixes the behavior w.r.t. MVCC.
          Hide
          Jonathan Gray added a comment -

          My vote (if only for one implementation) would be for the less radical patch that removes in-memory versions that are not visible rather than doing this cleanup on flush which has a number of performance implications. I can see some reasons for wanting to keep versions around (providing support to an Omid-like transaction engine requires retaining old versions for at least some time), but it would be cool to have an option to prevent the deletion of the old versions rather than require that these exist in cases I won't ever use them. In all my increment performance tests, of which there have been many, the upsert/removal of old versions is one of the biggest gains, especially if you have particularly hot columns.

          I'm not sure which design you are referring to when you talk about being true to HBase's design Or maybe you're referring to the general principles of HBase (append-only), but the increment operation itself was not part of any original design or implementation of HBase and has been a hack in one way or another from the very first implementation. For the reason that the implementation has been targeted at performance over purity. I've always seen it as an atomic operation that would have any notion of versioning as opaque to the user of the atomic increment. Again, I can see use cases for it, but I'd lean towards having it as an option rather than requirement.

          Thanks for doing this work, good stuff. +1

          Show
          Jonathan Gray added a comment - My vote (if only for one implementation) would be for the less radical patch that removes in-memory versions that are not visible rather than doing this cleanup on flush which has a number of performance implications. I can see some reasons for wanting to keep versions around (providing support to an Omid-like transaction engine requires retaining old versions for at least some time), but it would be cool to have an option to prevent the deletion of the old versions rather than require that these exist in cases I won't ever use them. In all my increment performance tests, of which there have been many, the upsert/removal of old versions is one of the biggest gains, especially if you have particularly hot columns. I'm not sure which design you are referring to when you talk about being true to HBase's design Or maybe you're referring to the general principles of HBase (append-only), but the increment operation itself was not part of any original design or implementation of HBase and has been a hack in one way or another from the very first implementation. For the reason that the implementation has been targeted at performance over purity. I've always seen it as an atomic operation that would have any notion of versioning as opaque to the user of the atomic increment. Again, I can see use cases for it, but I'd lean towards having it as an option rather than requirement. Thanks for doing this work, good stuff. +1
          Hide
          Lars Hofhansl added a comment -

          As I said I'm fine with the "less radical" patch
          Increments are special in they are only useful when updating something existing.

          Show
          Lars Hofhansl added a comment - As I said I'm fine with the "less radical" patch Increments are special in they are only useful when updating something existing.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12551483/4583-trunk-less-radical-v6.txt
          against trunk revision .

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

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

          -1 patch. The patch command could not apply the patch.

          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3241//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/12551483/4583-trunk-less-radical-v6.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. -1 patch . The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3241//console This message is automatically generated.
          Hide
          Lars Hofhansl added a comment -

          Another thought... As I described above I can also make upsert VERSIONS aware, by also counting the versions in upsert. That way even historical queries would be correct... Could be slow if many version are to be retained.

          There's also one other options: Use upsert if VERSIONS is set to 1 and use the normal Store.add otherwise.

          In both cases Omid type transaction engines could still work.

          I'm going to take it as a given from now on that we want to keep the upsert logic as the default option.

          Show
          Lars Hofhansl added a comment - Another thought... As I described above I can also make upsert VERSIONS aware, by also counting the versions in upsert. That way even historical queries would be correct... Could be slow if many version are to be retained. There's also one other options: Use upsert if VERSIONS is set to 1 and use the normal Store.add otherwise. In both cases Omid type transaction engines could still work. I'm going to take it as a given from now on that we want to keep the upsert logic as the default option.
          Hide
          Lars Hofhansl added a comment -

          In that case both Append and Increment should also get an optional timestamp member (like Put and Delete) to set the TS to use.

          While looking at Increment and Append in trunk, were they never protobuf'ed?

          Show
          Lars Hofhansl added a comment - In that case both Append and Increment should also get an optional timestamp member (like Put and Delete) to set the TS to use. While looking at Increment and Append in trunk, were they never protobuf'ed?
          Hide
          Lars Hofhansl added a comment -

          Here's what I mean:
          Use upsert for Increment and Append if VERSIONS == 1, use Store.add otherwise (just like Puts in that case)

          TODO:
          I can easily adept Append to take the TS (in fact it already does, just not per column); for Increments it's a bit more tricky as currently just serializes a column -> long mapping.

          Show
          Lars Hofhansl added a comment - Here's what I mean: Use upsert for Increment and Append if VERSIONS == 1, use Store.add otherwise (just like Puts in that case) TODO: I can easily adept Append to take the TS (in fact it already does, just not per column); for Increments it's a bit more tricky as currently just serializes a column -> long mapping.
          Hide
          Jonathan Gray added a comment -

          That makes sense to me (versions = 1 means upsert).

          Big +1 from me on adding support for setting the timestamp.

          Show
          Jonathan Gray added a comment - That makes sense to me (versions = 1 means upsert). Big +1 from me on adding support for setting the timestamp.
          Hide
          Lars Hofhansl added a comment -

          Do you think we need this per column (as in Puts/Deletes) or just one TS per Increment/Append, which would be used for all columns?

          The latter would be easier to do, especially for Increment.

          Show
          Lars Hofhansl added a comment - Do you think we need this per column (as in Puts/Deletes) or just one TS per Increment/Append, which would be used for all columns? The latter would be easier to do, especially for Increment.
          Hide
          Jonathan Gray added a comment -

          A single timestamp for all columns is sufficient for the types of use cases I am imagining, so that's fine with me.

          Show
          Jonathan Gray added a comment - A single timestamp for all columns is sufficient for the types of use cases I am imagining, so that's fine with me.
          Hide
          Hadoop QA added a comment -

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

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

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

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

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

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

          -1 findbugs. The patch appears to introduce 17 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.regionserver.TestResettingCounters

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3247//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3247//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3247//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3247//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3247//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3247//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3247//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3247//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/12552419/4583-mixed.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 87 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 17 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.regionserver.TestResettingCounters Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3247//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3247//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3247//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3247//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3247//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3247//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3247//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3247//console This message is automatically generated.
          Hide
          stack added a comment -

          VERSIONs == 1 == upsert hack vs VERSIONs > 1 bypassing the hack sounds like good compromise to me – till we come around and introduce a memstore gleaning step as means of postponing small file flushes.

          Show
          stack added a comment - VERSIONs == 1 == upsert hack vs VERSIONs > 1 bypassing the hack sounds like good compromise to me – till we come around and introduce a memstore gleaning step as means of postponing small file flushes.
          Hide
          Lars Hofhansl added a comment -

          I started with adding a timerange to Append and TS to Increment, but that quickly leads to a lot of changes. Let's do that in another jira.

          Will some tests to 4583-mixed and have a final version for review.

          Show
          Lars Hofhansl added a comment - I started with adding a timerange to Append and TS to Increment, but that quickly leads to a lot of changes. Let's do that in another jira. Will some tests to 4583-mixed and have a final version for review.
          Hide
          Lars Hofhansl added a comment -

          Same patch, but runs TestAtomicOperation with Increments and Appends that atomically update two column families one with VERSIONS=1 and one with VERSIONS=3. So even atomicity between these is working.

          Show
          Lars Hofhansl added a comment - Same patch, but runs TestAtomicOperation with Increments and Appends that atomically update two column families one with VERSIONS=1 and one with VERSIONS=3. So even atomicity between these is working.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12552574/4583-mixed-v2.txt
          against trunk revision .

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

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

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

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

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

          -1 findbugs. The patch appears to introduce 17 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.regionserver.TestResettingCounters

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3261//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3261//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3261//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3261//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3261//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3261//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3261//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3261//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/12552574/4583-mixed-v2.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 87 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 17 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.regionserver.TestResettingCounters Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3261//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3261//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3261//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3261//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3261//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3261//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3261//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3261//console This message is automatically generated.
          Hide
          Ted Yu added a comment -

          For increment(), the first line below should have been kept:

          -          allKVs.addAll(entry.getValue());
          +          if (store.getFamily().getMaxVersions() == 1) {
          

          Patch v3 passes TestResettingCounters

          Show
          Ted Yu added a comment - For increment(), the first line below should have been kept: - allKVs.addAll(entry.getValue()); + if (store.getFamily().getMaxVersions() == 1) { Patch v3 passes TestResettingCounters
          Hide
          Ted Yu added a comment -
          +          if (versionsOlderThanReadpoint > 1) {
          +            // if we get here we have seen at least one version older than the readpoint,
          

          Does the above condition match the comment ? If one version old than readpoint is required, versionsOlderThanReadpoint == 1 should be enough, right ?

          Show
          Ted Yu added a comment - + if (versionsOlderThanReadpoint > 1) { + // if we get here we have seen at least one version older than the readpoint, Does the above condition match the comment ? If one version old than readpoint is required, versionsOlderThanReadpoint == 1 should be enough, right ?
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12552622/4583-mixed-v3.txt
          against trunk revision .

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

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

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

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

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

          -1 findbugs. The patch appears to introduce 17 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.io.encoding.TestLoadAndSwitchEncodeOnDisk
          org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3263//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3263//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/12552622/4583-mixed-v3.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 87 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 17 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.io.encoding.TestLoadAndSwitchEncodeOnDisk org.apache.hadoop.hbase.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3263//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3263//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3263//console This message is automatically generated.
          Hide
          Lars Hofhansl added a comment -

          Yep. that line was removed by accident. Cool that a test caught it.
          Thanks Ted!

          Show
          Lars Hofhansl added a comment - Yep. that line was removed by accident. Cool that a test caught it. Thanks Ted!
          Hide
          Lars Hofhansl added a comment -

          Does the above condition match the comment ? If one version old than readpoint is required, versionsOlderThanReadpoint == 1 should be enough, right ?

          I think the comment and code is correct. We need make sure that any scanner does not see the KV to be removed, which means that must be one that is newer than this one, but still older than the readpoint.

          (A possible change, though, would be to count KVs with cur.getMemstoreTS() <= readpoint, instead of cur.getMemstoreTS() < readpoint)

          Show
          Lars Hofhansl added a comment - Does the above condition match the comment ? If one version old than readpoint is required, versionsOlderThanReadpoint == 1 should be enough, right ? I think the comment and code is correct. We need make sure that any scanner does not see the KV to be removed, which means that must be one that is newer than this one, but still older than the readpoint. (A possible change, though, would be to count KVs with cur.getMemstoreTS() <= readpoint, instead of cur.getMemstoreTS() < readpoint)
          Hide
          Lars Hofhansl added a comment -

          Version that does this.

          Show
          Lars Hofhansl added a comment - Version that does this.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12552670/4583-mixed-v4.txt
          against trunk revision .

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

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

          +1 hadoop2.0. The patch compiles against the hadoop 2.0 profile.

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

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

          -1 findbugs. The patch appears to introduce 17 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.mapreduce.TestHFileOutputFormat

          Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3264//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3264//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3264//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3264//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3264//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3264//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3264//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html
          Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3264//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/12552670/4583-mixed-v4.txt against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 3 new or modified tests. +1 hadoop2.0 . The patch compiles against the hadoop 2.0 profile. -1 javadoc . The javadoc tool appears to have generated 87 warning messages. +1 javac . The applied patch does not increase the total number of javac compiler warnings. -1 findbugs . The patch appears to introduce 17 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.mapreduce.TestHFileOutputFormat Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/3264//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3264//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop2-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3264//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-examples.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3264//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-server.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3264//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop1-compat.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3264//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-common.html Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/3264//artifact/trunk/patchprocess/newPatchFindbugsWarningshbase-hadoop-compat.html Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/3264//console This message is automatically generated.
          Hide
          stack added a comment -

          I'm not expert in this area. I like the way you make it so can do both upsert and add (need to release note it). I like how you add the test only methods to HStore and not to the Interface. Is that test ok w/ 100 threads? Its pretty resource heavy? It runs ok? I'm +1 on committing to trunk if all tests pass. Good stuff Lars.

          Show
          stack added a comment - I'm not expert in this area. I like the way you make it so can do both upsert and add (need to release note it). I like how you add the test only methods to HStore and not to the Interface. Is that test ok w/ 100 threads? Its pretty resource heavy? It runs ok? I'm +1 on committing to trunk if all tests pass. Good stuff Lars.
          Hide
          Varun Sharma added a comment -

          In the spirit of HBASE-7051 which fixes the race condition between put and checkAndPut for both 0.94 and 0.96 - I think we should at least fix the race condition for 0.94 for Append and Increment operations, like we did for checkAndPut. I would also be in favour of fixing the entire issue for 0.94, now that we have a very elegant and clear patch for 0.96. Unless, there are strong objections, I can help replicate this patch for 0.94 ?

          Show
          Varun Sharma added a comment - In the spirit of HBASE-7051 which fixes the race condition between put and checkAndPut for both 0.94 and 0.96 - I think we should at least fix the race condition for 0.94 for Append and Increment operations, like we did for checkAndPut. I would also be in favour of fixing the entire issue for 0.94, now that we have a very elegant and clear patch for 0.96. Unless, there are strong objections, I can help replicate this patch for 0.94 ?
          Hide
          Lars Hofhansl added a comment - - edited

          @Varun: I mostly agree.
          Increment/Append won't play nicely with concurrent Puts/Deletes anyway (without this patch that is), whereas checkAndXXX does.

          Stack These tests finish pretty quickly on my laptop (about 2m), and < 15s on my (beefy) desktop. Should be OK.

          OK... Going to commit later today. TestHFileOutputFormat is unrelated - I'll double check.
          This is nice correctness improvement to HBase.

          Show
          Lars Hofhansl added a comment - - edited @Varun: I mostly agree. Increment/Append won't play nicely with concurrent Puts/Deletes anyway (without this patch that is), whereas checkAndXXX does. Stack These tests finish pretty quickly on my laptop (about 2m), and < 15s on my (beefy) desktop. Should be OK. OK... Going to commit later today. TestHFileOutputFormat is unrelated - I'll double check. This is nice correctness improvement to HBase.
          Hide
          Lars Hofhansl added a comment -

          Committed to 0.96... Pfeeww. After one year.
          Thanks for the reviews folks.

          Show
          Lars Hofhansl added a comment - Committed to 0.96... Pfeeww. After one year. Thanks for the reviews folks.
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK #3527 (See https://builds.apache.org/job/HBase-TRUNK/3527/)
          HBASE-4583 Integrate RWCC with Append and Increment operations (Revision 1407725)

          Result = FAILURE
          larsh :
          Files :

          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK #3527 (See https://builds.apache.org/job/HBase-TRUNK/3527/ ) HBASE-4583 Integrate RWCC with Append and Increment operations (Revision 1407725) Result = FAILURE larsh : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
          Hide
          Hudson added a comment -

          Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #255 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/255/)
          HBASE-4583 Integrate RWCC with Append and Increment operations (Revision 1407725)

          Result = FAILURE
          larsh :
          Files :

          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java
          • /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java
          • /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
          Show
          Hudson added a comment - Integrated in HBase-TRUNK-on-Hadoop-2.0.0 #255 (See https://builds.apache.org/job/HBase-TRUNK-on-Hadoop-2.0.0/255/ ) HBASE-4583 Integrate RWCC with Append and Increment operations (Revision 1407725) Result = FAILURE larsh : Files : /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java /hbase/trunk/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java /hbase/trunk/hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java
          Hide
          Lars Hofhansl added a comment -

          The jenkins builds are somewhat unstable, the first build after this hung (somewhere) the next one had some kind of the env problems with OOMs.
          I don't think that was caused by this patch, but it is hard to say.

          Show
          Lars Hofhansl added a comment - The jenkins builds are somewhat unstable, the first build after this hung (somewhere) the next one had some kind of the env problems with OOMs. I don't think that was caused by this patch, but it is hard to say.
          Hide
          Lars Hofhansl added a comment -

          The latest run looks good. Still fails an unrelated test, but no weird issue. So all's good it seems.

          Show
          Lars Hofhansl added a comment - The latest run looks good. Still fails an unrelated test, but no weird issue. So all's good it seems.
          Hide
          stack added a comment -

          Good on you Lars.

          Show
          stack added a comment - Good on you Lars.
          Hide
          Varun Sharma added a comment -

          Adding patch for 0.94 - I had to rework incrementColumnValue to use Increment() instead so that it is MVCC'ised as well.

          Show
          Varun Sharma added a comment - Adding patch for 0.94 - I had to rework incrementColumnValue to use Increment() instead so that it is MVCC'ised as well.
          Hide
          Varun Sharma added a comment -

          Attaching patch.

          Show
          Varun Sharma added a comment - Attaching patch.
          Hide
          Lars Hofhansl added a comment -

          Sorry, not for 0.94. (As much as I like this patch as improvement, this is too radical for 0.94)

          Show
          Lars Hofhansl added a comment - Sorry, not for 0.94. (As much as I like this patch as improvement, this is too radical for 0.94)
          Hide
          Varun Sharma added a comment -

          Oh okay - I thought from your previous comment that you agreed on a solution for 0.94 - maybe you only meant that we fix the Race condition b/w put and append/increment like we do it for checkAndPut and not really do the MVCC part ? Or did I misunderstand ?

          Varun

          Show
          Varun Sharma added a comment - Oh okay - I thought from your previous comment that you agreed on a solution for 0.94 - maybe you only meant that we fix the Race condition b/w put and append/increment like we do it for checkAndPut and not really do the MVCC part ? Or did I misunderstand ? Varun
          Hide
          Lars Hofhansl added a comment -

          I'm actually not opposed to 0.94, but other folks voiced (valid) concerns.
          We can fix the race condition between Puts followed by Increment or Append, but I don't think that would be that useful without the rest of this patch.

          I see this more as a strategic correctness fix. This has been "incorrect" since the beginning, so not fixing this in 0.94 is OK, I think.

          Anyway. Thanks for working on a 0.94 patch, Varun.
          So you have a strong usecase for this in 0.94?

          Show
          Lars Hofhansl added a comment - I'm actually not opposed to 0.94, but other folks voiced (valid) concerns. We can fix the race condition between Puts followed by Increment or Append, but I don't think that would be that useful without the rest of this patch. I see this more as a strategic correctness fix. This has been "incorrect" since the beginning, so not fixing this in 0.94 is OK, I think. Anyway. Thanks for working on a 0.94 patch, Varun. So you have a strong usecase for this in 0.94?
          Hide
          Varun Sharma added a comment -

          I see - i thought since 0.94 is the current stable version, the one we currently use and we are going heavily use it for counters - we wanted to see if we could use it. Eventually, the other JIRA i am looking into (allowing puts + increments + deletes) in a single mutation is also important for the use case - basically to have counts + puts go in as a single mutation and have a consistent view of the table - also, it helps cut down write latency for us.

          But judging from the reaction, it looks like we might not want that other JIRA to go into 0.94 either (even if we possibly flag protected the change) - in which case, I am okay with this not going through (we can probably use a manually patched version for our usage). Btw, when is the 0.96 expected to ship - it has lots of good features/fixes in it.

          Thanks !

          Show
          Varun Sharma added a comment - I see - i thought since 0.94 is the current stable version, the one we currently use and we are going heavily use it for counters - we wanted to see if we could use it. Eventually, the other JIRA i am looking into (allowing puts + increments + deletes) in a single mutation is also important for the use case - basically to have counts + puts go in as a single mutation and have a consistent view of the table - also, it helps cut down write latency for us. But judging from the reaction, it looks like we might not want that other JIRA to go into 0.94 either (even if we possibly flag protected the change) - in which case, I am okay with this not going through (we can probably use a manually patched version for our usage). Btw, when is the 0.96 expected to ship - it has lots of good features/fixes in it. Thanks !
          Hide
          Gregory Chanan added a comment -

          It is a little strange that checkAndPuts now follow MVCC in 0.94 (HBASE-7051) but append/increments don't. I think I agree with the reasoning here, though.

          Show
          Gregory Chanan added a comment - It is a little strange that checkAndPuts now follow MVCC in 0.94 ( HBASE-7051 ) but append/increments don't. I think I agree with the reasoning here, though.
          Hide
          Lars Hofhansl added a comment -

          it is, but checkAndXXX operation are must work together with Puts, whereas increment and append are typically used on their own.

          Show
          Lars Hofhansl added a comment - it is, but checkAndXXX operation are must work together with Puts, whereas increment and append are typically used on their own.
          Hide
          stack added a comment -

          Marking closed.

          Show
          stack added a comment - Marking closed.

            People

            • Assignee:
              Lars Hofhansl
              Reporter:
              Lars Hofhansl
            • Votes:
              0 Vote for this issue
              Watchers:
              15 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development