FLUME 417 : Add command to purge only DECOMMISSIONED or LOST node status

Review Request #1788 - Created June 2, 2011 and updated

satish kumar eerpini
old-flume
0.9.4
flume
aaron, jon
Modified the existing purge and purgeAll commands to purge only those nodes which are in either DECOMMISSIONED or LOST states.
Tested with pseudo distributed setup. Works. More testing needed.
  1. Satish,
    
    I have a few nits, but overall look reasonable.  We require new or modified tests to verify behavior so please add some unit tests.  Some suggestions:
    - If you take a look at TestFlumeShell, you could see a unit test that tests the purge command, or
    - Could probably add a simple test in TestStatusManager (since this modifies StatusManager)
    
    Thanks,
    Jon
  2. I think these lines need to be synchronized -- you could probably wrap all the code with:
    
    synchronized(statuses) {
     // all of function's code.
    }
    
    
  3. typo? (looks fixed in v2)
  4. prefer '//' style comments here.
  5. this synchronized not needed (already has statuses lock)
  6. nit: use spaces not tabs  
  7. src/docs/UserGuide/FlumeShell (Diff revision 1)
     
     
    ... DECOMMISSIONED or LOST state.
  8. 
      
Review request changed

Change Summary:

Made the changes specified in the comments, please take a look.

Diff:

Revision 3 (+41 -13)

Show changes

  1. Sorry it too me a while to get this.  Looks pretty good.  There are a few style nits.  The main blocker to this is that we definitely still need unit test cases.
    
    
    1. Here's probably the easiest way to make some unit tests of this new behavior.
      
      Look at TestFlumeShell.testPurge.   Add another test or modify the existing one so that it nodes are in different states.  (the call to updateHeartbeStatus). Make some of them DECOMISSIONED or LOST.  Run purgeAll.  Verify only the expected entries are purged, and that the entries that should remain continue to remain.
    2. Hi Satish, 
      
      It has been a little while, please let us know if you will be able to write a unit test for this? 
      
      Thanks,
      Jon
      
      
  2. style nits: spacing. (please make it look like the other code)
  3. style nits: space after  ) and before { 
  4. 
      
Loading...