HBASE-2578

Review Request #83 - Created May 24, 2010 and submitted

Daniel Ploeg
old-hbase
HBASE-2578
hbase
HBASE-2578 - Add ability for tests to override server-side timestamp setting (currentTimeMillis).
The solution in this patch ensures that tests use a different timestamp with a minimal change to the production code paths.
One question I would like to know is whether the change that was made to HRegion.FIXED_OVERHEAD would cause any other side effects.


  1. Looks good. Few notes:
    - Please add the Apache license header to all the new files
    - Do we really need to set this up as an instance variable for each HRegion? It seems like it may be more useful as a global singleton - I'm wondering whether we're going to run into issues if we try to extend this to do tests that cross HRegion->Store->StoreFile boundaries. What do you think?
  2. may need to be synchronized, or use AtomicLong
    1. Thanks Todd. 
      
      - I'm modifying the HowToContribute page and didn't see a reference to the inclusion of the license header there or on the Code review page. I haven't committed my next change to the HowToContribute page yet, so I'm adding a reference there (plus modifying my files).
      - The singleton approach I think might be better. I thought that having the instance variable smelled a bit, that's why I asked about the fixed overhead. I'll make some changes. 
      - IncrementingEE - I'll make some sync type changes here.
      
      I'll upload a new patch later today with these changes.
  3. 
      
  1. What other possible approaches could we take to solving this issue?  I seem to recalling a different strategy but I cannot recall it right now. We are really not set up for DI right now, and the ultimate direction of this patch is a fully dependency injected solution. 
  2. this seems overly HRegion specific - what is the "standard" mechanism for providing mocked out systems level time-based replacements?
  3. lets not use assertEquals(String,int,int) and just use assertEquals(int,int).  
  4. 
      
  1. Implementation is well done.  The need for this though strikes me as odd.  So I'm +1 on the patch but let me go comment up on the bigger issue I see up in the JIRA.
  2. 
      
  1. 
      
  2. this is a lot of synchronization for what ends up being on the fast path of every query, perhaps there is a non synchronized manner in which we can accomplish this?  the use of volatile might help here
    1. thanks Ryan. I think I might use AtomicReference instead - can simplify even further this way. I'll upload another patch a bit later.
  3. 
      
  1. 
      
  2. I'm thinking perhaps a Static interface could be used instead of the getDelegate().currentTimeMillis() that way the individual call sites could say:
    
    EnvironmentEdge.currentTimeMillis()
    instead of
    System.currentTimeMillis()
    
    but the static calls would be a simple wrapper around the existing getDelegate mechanisms... this would just make the code more explicit and easier to follow for future generations.
    
    OTher than that, looking great! Thanks for making all the changes!
    1. Ok sounds good. The EnvironmentEdge is the interface, so I think I'll need to put the static method on the manager. The call would be:
      
      EnvironmentEdgeManager.currentTimeMillis()
      
      I'm adding a mocking library to the pom (EasyMock) to help with the testing.
  3. 
      
Review request changed
Loading...