Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
HBase >> mail # dev >> Review for HBASE-7568 [replication] Create an interface for replication queues


Copy link to this message
-
Re: Review for HBASE-7568 [replication] Create an interface for replication queues
Two general comments about the patch. The code itself looks good.

J-D

On Thu, Mar 28, 2013 at 1:40 PM, Chris Trezzo <[EMAIL PROTECTED]> wrote:
> Hi All,
>
> I recently posted a patch for HBASE-7568 and it would be great if I could
> get some feedback/eyes on it. If you need a higher-level context, the
> parent issue HBASE-7564 has a brief document that outlines the intended
> class structure of the refactor. I have split the refactor into multiple
> patches to hopefully make this more digestible. This patch only covers
> state related to replication queues. State for replication peers will be
> split out in a follow on patch (HBASE-7567) that is almost ready.
>
> Review: https://reviews.apache.org/r/9987/
>
> Some of the major motivations for refactoring the replication code are:
> 1. Make it more testable. With clear interfaces we can easily unit test how
> replication maintains its state.

I think your argument for a refactoring would be a lot stronger if the
patch was adding new tests, at least to demonstrate the added
testability.

> 2. Make the code more understandable. In the code's current form, there is
> a lot of complexity in the interaction between ReplicationZookeeper,
> ReplicationSource and ReplicationSourceManager. Hopefully, the refactor
> will make these interactions more explicit.
> 3. Make the code more flexible for alternate implementations. If in the
> future we wanted to store replication queues in something other than
> Zookeeper (not saying we necessarily want to do this), having the
> interfaces will make this easier.

If we want to be able to add other implementations, should it be
better giving more accurate names to the current ones? If what we have
is ZK-based, I would then replace "Impl" in the class names with "ZK"
or something like that.

>
> Thanks!
> Chris Trezzo