Thanks J-D! 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. > > 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. > 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 > 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. > > Another good point. I will rename the impl classes to indicate that they are the Zookeeper implementation. > > > > Thanks! > > Chris Trezzo >
