HBASE-2863 -- HBASE-2553 removed an important edge case

Review Request #354 — Created July 21, 2010 and submitted

ryanobjc
old-hbase
HBASE-2863
hbase
There are tricky edge cases that were removed by HBASE-2553 (oopsy!)... flaky tests have been illustrating them. This patch fixes those flaky tests to be not flaky (using the EnvironmentEdgeManager thing) and also fixes them, and introduces tests that cover the particular use cases slightly better as well. Oh yes and and fixes the actual bug.

Without these fixes we would end up with KVs with different values with the same Timestamp which causes problems.  This can happen when we get more than 1 increment/millisecond and especially during a snapshot.


stack
  1. At least missing license needs fixing.  There's at least one question in the below too.  Good stuff.
  2. Nice here too
  3. Good stuff.
  4. Could this first kv on the row in memstore have a ts in advance of 'now'?  I suppose it can't -- least it shouldn't be possible, right?
    
    If  thousands of updates a second, could this be a prob?  This logic?
    1. so what was happening previously:
      - snapshot happens
      - we do an increment which found there was no 'existing' TS in memstore to 'update' so we make a new one with 'now'.
      - we end up with two KVs, one in snapshot one in memstore both with the same timestamp.
      
      Another issue (which the unit test perfectly tests for) is so:
      - time=1 snapshot occurs with a KV ts=1 into snapshot
      - time=1 ICV happens, but now the KV in memstore _must_ have a TS=2 (or else we get duplicate TS - bad!)
      - time=1 ICV happens, but the KV in memstore is TS=2, but now=1, so we need to keep the max(now,TS in memstore) or else we get potential duplicates
      
      In both these cases if we have a Put with ts=Now+X we have problems:
      - in HRegion we do a Get and we see Value=100 ts=Now+X
      - in Store we do updateColumnValue with Value=101, TS=now
      - If we dont clear that ts=Now+X we will put a shadowed KV, and the _next_ ICV will see value '100' not value '101' and we will never actually 'increment' until we get past Now+X.  The compromise is to clear out all KVs in memstore period.
      
      We can end up with a situation where the KV in memstore leads 'real time' by a millisecond during snapshots.  If we had a way of comprehensively deduping equivalent TSs during post-hoc reads perhaps we wouldnt need this.
    1. thanks, the EnvironmentEdge stuff is paying off already.
  5. 
      
stack
  1. +1 I buy your rationale.  Please add missing license on commit (Oh, +1 predicated on all tests passing).
  2. 
      
Loading...