-Re: Review for HBASE-7568 [replication] Create an interface for replication queues
Chris Trezzo 2013-03-28, 21:17
On Thu, Mar 28, 2013 at 2:10 PM, Jean-Daniel Cryans <[EMAIL PROTECTED]>wrote:
> Two general comments about the patch. The code itself looks good.
> 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
> > 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
Good point. I do have a follow on jira (HBASE-8149) for increasing coverage
around the new interfaces, but I can add some new tests as part of this
patch as well.
> > 2. Make the code more understandable. In the code's current form, there
> > 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.
Another good point. I will rename the impl classes to indicate that they
are the Zookeeper implementation.
> > Thanks!
> > Chris Trezzo