Review Board 1.7.22

HBASE-5712 Parallelize load of .regioninfo file sin diagnostic/repair portion of hbck

Review Request #4883 - Created April 26, 2012 and submitted

Jonathan Hsieh
0.92, 0.94, 0.96
jxiang, tedyu
* Parallelized load of .regioninfo files
* changed TreeMap to SortedMap in method signatures
* renamed a test's name.
Ran patch 10x on trunk, passes.  Ran 1x on 0.92 and 0.94.

Ther 0.90 version that is nearly identical except for ignoring changes near lines HBaseFsck lines 671-680.
Posted (April 26, 2012, 2:25 a.m.)
Looks very good.
This is ok. However, I think it is better to use a timed wait in case it hangs.
  1. There is another section of code that uses the same logic and has been around for a while.  I'll file a follow on issue to convert this homebrew fork join stuff in to a Futures.
Posted (April 27, 2012, 9:33 p.m.)


What does 'Adopting' mean ?
The grammar doesn't seem to be right.
  1. "Adopting" is a term that I use for taking an "orphaned" hdfs region dir (a region in hdfs missing .regioninfo file, and likely not in META/deployed) and adding it to meta, adding a .regioninfo file, and potentially deploying the region.  
    How about I change it to this?
    "Attempt to adopt orphan region skipped because no files present in " ... 
Ship it!
Posted (April 27, 2012, 11:27 p.m.)
+1  Minor comments below if interested
This'll work but why not ConcurrentSkipListMap?
  1. Sure, changed.
+1 on suggested change
  1. done.
Is this flag needed?  Why not just check thread is alive?  I see we can return with an error.  What happens if the return on 2816 happens?  Will the wait at #643 above be for ever?
  1. This is not a thread but actually fed to an executor (thread pool) at line 637.  If the return happens on 2816, this is in a finally which will always mark the workitem as done. 
    There are two other instances of this pattern that were originally in this code before I got to it -- I'd have used Futures (and have filed a follow on issue for it) but it works.