[
https://issues.apache.org/jira/browse/HBASE-11467?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14104820#comment-14104820
]
ryan rawson commented on HBASE-11467:
-------------------------------------
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:
TestClusterIdResource.java:
assertThat(JSON.parse(new String(response.getBody())).toString(),
is(TEST_UTIL.getHBaseCluster().getClusterStatus().getClusterId()));
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:
assertThat(response, isJSONString(CLUSTER_ID));
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));
assertThat(registry, tableOnline(META_TABLE));
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.
> New impl of Registry interface not using ZK + new RPCs on master protocol
> -------------------------------------------------------------------------
>
> Key: HBASE-11467
> URL: https://issues.apache.org/jira/browse/HBASE-11467
> Project: HBase
> Issue Type: Sub-task
> Components: Client, Consensus, Zookeeper
> Affects Versions: 2.0.0
> Reporter: Mikhail Antonov
> Assignee: Mikhail Antonov
> Fix For: 2.0.0
>
> Attachments: HBASE-11467.patch, HBASE-11467.patch, HBASE-11467.patch
>
>
> Currently there' only one implementation of Registry interface, which is
> using ZK to get info about meta. Need to create implementation which will be
> using RPC calls to master the client is connected to.
> Review of early version of patch is here: https://reviews.apache.org/r/24296/
--
This message was sent by Atlassian JIRA
(v6.2#6252)
