Review Board 1.7.22


HBASE-451 Remove HTableDescriptor from HRegionInfo

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

Michael Stack
HBASE-451
Reviewers
hbase
hbase-git
Posting for Subbu  See issue for his description of change.

 
Posted (June 2, 2011, 11:07 p.m.)
This looks excellent.  Lets get it committed before it rots.   Do we still need .regioninfo after this goes in?
Copy/paste error.
  1. Fixed.
WhooHoo... this is starting out good!

I think you should take the opportunity of changing HRI to be a VersionedWritable.
Excellent! (HRI not having ref to HTD)
This works?  Thats interesting.
When would this be used?
  1. Currently used from test classes. can remove it after validation.
  2. Its ok.  Just flag it as used from tests w/ a comment.
This should not be allowed?  Are you not going to mess up stuff if table gets changed on an HRI?
  1. Yup. totally. removed it.
Should these be deprecated rather than removed?  Even if we returned a null only?
  1. Hmm. Any advantage of keeping it deprecated and returning NULL versus totally slaughtering? I can bring it back on.
  2. We like to deprecate for one major release before removing.  Can you deprecate and then actually read the FS to get the actual HTD and return it?  You can deprecate it and say its EXPENSIVE to call.
This is perverse, begin able to change the HTD under an HRI.  Can't believe this actually was allowed exist.
You could just do an equals since its boolean... Bytes.equals...
  1. Good. Done.
ditto
  1. Good. Done.
This is incorrect.  Should be isMetaTable() or isRootRegion... a 'meta' region is a .META. or -ROOT- member.
  1. Not sure I get it. We do need support for isMetaRegion API call to determine if the Region represented by this HRI is a meta one. How shall we do it? 
  2. Well, IIUC, isMetaTable currently returns TRUE if either -ROOT- or .META.  I was just flagging that you could introduce strange bugs if you change how this method works.  if you need a method for is.META. then add new one I'd suggest (though it'll clash with this one).
    
    Or, hang on, perhaps I am misleading you.  I am confusing isMetaTable with isMetaRegion?  The latter is the one that returns true if -ROOT- or .META.  isMetaTable should be true only if its .META. table.
    
    Sorry about that.
Put this into a migration subpackage?
  1. Could not agree more on this. I shall create a migration package *.hbase.migration and move this there.
  2. Good.
    
Oh, so what is this?  Its old-school HRI?  If so, call it HRI090?  Or if you have it in a subpackage  migration, it can have same name and just be fully specified whereever it is used.. the migration subpackage will make it explicity taht this is not same as new HRI.
  1. hmm.. makes sense. so you want to remove it from the class name.
  2. Up to you.  I'd remove 'ForMigration' from class name or replace it with 090x so its HRegionInfo090x so it indicates its old style HRI.  Up to you.
This is from elsewhere?  Does it belong here?
  1. Its from old school HRI "as is". 
  2. Ok
    
I don't think we should do this by default.  What if KV has a value of MBs?
  1. yup. good call. removed it.
What if we crash half-way through migration?  We should be able to deal w/ a .META. that is half the old style HRI and half the new style?
  1. Yup agreed. Need to think about how to handle this. any ideas? We might probably need to read the META rows using old school HRI till a failure (IOException trying to serialize a new HRI into old HRI) and assume that the rest are all processed? 
  2. I think our suggestion of failed deserialization of old type HRI triggers attempt at deserializing it as new-type a good one.
    
    Only other thought I had was of serializing new type with some MAGIC bytes first, magic that you discarded when undoing it.... but I think doing this will probably make things way more complicated.
You want to leave these in place?
  1. Nope. slipped through the cracks
What a crazy way of getting HTDs.
  1. You want to rename? suggestions?
  2. No.  You removed it which is good.  I was just commenting on how crazy our old implementation was.
Do we need these last two? Can't we use this first method to get the latter?
  1. We definitely can. Thought we can avoid iterating over the loop esp if we have large number of tables in the cluster. 
  2. Up to you.  Just seems a little gratuitous adding these three methods.   If you can cut out one at least....?
Is this going to the FS to read tables?  And we're holding sync while we are doing it?  Is that necessary?
This method belongs in master since its going to run the migration?
  1. Yup moved it to Master.
Close the file?
  1. Yup. Done
Good.  The human readable version is included.
I wonder if you have to create the file elsewhere and then do a rename to put it in place?  Else if crash, could be half-written?  Do we do this w/ .regioninfo?
  1. Let me check this. I have seen this creating in tmp and renaming it later pattern. 
  2. Yes.  Odd things can happen when regionserver crashes mid-writing.  Zero files that make no sense. Table schema would be pretty bad to lose.