hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ryan rawson (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-11467) New impl of Registry interface not using ZK + new RPCs on master protocol
Date Wed, 20 Aug 2014 23:57:38 GMT

    [ 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)

Mime
View raw message