So I checked this patch out, a few things really stood out to me:
- Lets not have init() in the MasterAddressesProvider interface. The interface should describe what an object does, not how it does it. I talked to Mikhail and this is going to get moved to the constructor. This'll make it easier to reuse this object in other places as well.
- Let's also not have it in Registry if possible. Consider this a stretch goal.
- Having an extra REST RPC to find the cluster ID is an acceptable solution - it maintains RPC roundtrips compared to the current implementation. It'd be nice if we didn't have to have it, but I think that shouldn't hold this issue up.
There are a few things, eg:
This could be made easier to read if we refactored elements of the functionality into static methods. for example we could have a JSON matcher so we have something like:
Note we refactored out the parsing, and the 'train wreck' call to get getClusterId (shouldn't this be a property of an HBaseCluster anyways?) and it's substantially cleaner to read. This is only one possibility here.
We can also use hamcrest matchers to make things easier to read, eg:
assertThat(registry.isTableOnlineState(TableName.META_TABLE_NAME, true), is(true));
assertThat(registry.isTableOnlineState(TableName.META_TABLE_NAME, false), is(false));
Right now the boolean flags don't mean much to me just from looking at that snippet (unless I have also, for some reason, memorized the registry API).
Also, I am seeing a few Thread.sleep() -> if we can get an async notice that the thing we are waiting for has happened, that'd be pretty sweet.
Other than those things, this is shaping up nicely.