This is an automatically generated e-mail. To reply, visit:
There is a lot of excellence in here. I'm going to look at the code itself with this diff applied to try and understand where/how CT is now being used. I'm a little unclear between the lines you'd like to draw and the lines you actually draw in this diff.
maybe note here that you should not be synchronized on metaAvailable (and it will do so in the method)... the next method below is nicely clear in this regard
verify the connection works, and also that the server is actually hosting the region we think it is... the comment makes me think this is looking up which server hosts the passed region but it's just verifying if we can connect to the server we think is hosting the region and verifies whether it's hosting it or not (so this fails if we can't connect or if the region is not on this server)
i'm still trying to understand exactly what you've changed and what is still a TODO, but this looks much nicer now!
same here! nice (old stuff looks ripe with race conditions)
missing copyright and year?
nice moving cruft to separate classes
seems like this should be moved to static methods in a helper class rather than exposing to our client-side Result
yeah, shouldn't this be in MetaReader or some such class?
missed some whitespace
this seems like an important public method. i like the rename and your additional comments, but maybe we should add more. default behavior is to use a cached location, if one is not found, it is looked up in a catalog. setting reload to true bypasses the cache and forces the lookup to a catalog. and then, under what cases do we get an exception? does this verify that the server is actually hosting the region? or it just looks up in the catalog (i guess failure there could cause IOE) and if it finds something, just returns a connection to that RS (w/ no verification)... correct?
why do you remove the javadoc on this method?
not even necessary to put this method in here at all now (we're just using it for getting the node name at this point but it's probably still nice to have the name in stacks and such)
30,000 ft desc? i guess test name is self descriptive?
On 2011-09-27 06:38:09, Michael Stack wrote:
This is an automatically generated e-mail. To reply, visit:
(Updated 2011-09-27 06:38:09)
Review request for hbase and Jonathan Gray.
Make the Meta* operations against meta retry. We do it by using HTable instances.
(HTable calls HConnection.getRegionServerWithRetries for get, put, scan etc).
In 0.89, we had special RetryableMetaOperation class that was a
subclass of Callable which reproduced the guts of HConnection.getRegionServerWithRetries
with its retry loop. Now we just use HTable instead (Costs some on setup but
otherwise, we avoid duplicating code). Upped the retries on serverside too.
Had problem with CatalogJanitor. MetaReader and MetaEditor were relying
heavily on CT methods getting proxy connections to meta and root servers.
CT needs to be cut back. This patch closes down access on (unused) public
methods and removes being able to get an HRegionInterface on meta and root
– this stuff is used internally to CT only now; use MetaEditor or
MetaReader if you want to update or read catalog tables. Opening new issue
to cutback CT use over the code base.
A little off topic but couldn't help it since was in MetaReader and MetaEditor
trying to clean them up, I ended up moving meta migration code out to its
own class rather than have it in all inside in MetaEditor.
Here is some detail to help reviews.
Clean up. Shutdown access on some of these unused methods. Don't
let out HRegionInterface instances in particular since we are going
away from raw HRI use to instead use a connection with retries:
Comments on state of this class. Javadoc edits.
getZooKeeperWatcher on HConnection is deprecated so don't use it
in constructor. Override MetaNodeTracker and on node delete
reset meta location (We used to do this over in MetaNodeTracker
but to do that we had to have a CatalogTracker over in zk package
which is silly – bad package encapsulation).
(waitForRootServer) Renamed getRootServerConnection and change it
from public to package private.
(waitForRootServerConnectionDefault, getRootServerConnection) Removed.
(getMetaServerConnection) Change from public to package private.
Use MetaReader to read the meta location in root rather than a
raw HRegionInterface so we get retrying.
(remaining, timedout) Added utility methods.
(waitForMetaServer) Changed from public to private.
(resetMetaLocation) Made it synchronized on metaAvailable.
Not all accesses were synchronized.
Refactor to use HTable instead of raw HRegionInterface so we get
retrying. For each operation we get an HTable, use it, then close it.
(putToMetaTable, putsToMetaTable, etc) Utility methods.
(updateRootWithMetaMigrationStatus, etc.) Moved out to own
class since these classes are for a one-time migration only.
New class that holds all Meta* methods updating meta table used
doing the one-time migration done to meta on startup. This class
is marked deprecated because its going to be dropped in 0.94.
Retrofit methods in here to use fullScan methods with Visitor.
(fullScan) Cleaned up the fullScans. Fixed up wrong javadoc.
(fullScanOfResults) Renamed as fullScan override.
(fullScanOfRoot) Added as deprecated. We should be doing
this against zk.
(metaRowToRegionPair, getServerNameFromResult) Moved to Result
Handle few cases where methods throw InterruptedException
(Don't let it out on the HBaseAdmin public API)
Populate new exception, RetriesExhaustedException.ThrowableWithExtraContext
on failure. Call ServerCallable connect AFTER beforeCall rather than
ServerCallable.instantiateServer BEFORE beforeCall.
Add to DEBUG message the connection name we were using.
Added new ThrowableWithExtraContext that takes extra context info.
instantiateServer renamed as connect
Javadoc. Renamed instantiateServer as connect.
Javadoc. Use MetaReader method instead of handcoding.
Allow hris can come back null when we ask for table regions.
Remove import of CatalogTracker.
Use utility in MetaReader instead of handcode it.
Use new HConnectionTestingUtility mocking tests (need to use it
because its a bit harder mocking tests now that we use HTable instead
of the more direct HRegionInterface).
Add some tests of broken out utility methods.
Add test of 3669 retrying.
New test utility that helps with mock of HConnection making it so can mock
an HConnection and then have an HTable use the mocked connection. Can do
a mock or a spied on HConnection
The migration code moved. Reference new location.
Was waiting on wrong events. Was waiting on Opens rather than Splits. Fix.
This addresses bug hbase-3446.
All tests passed recently. Rerunning again.