----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/76/#review194 -----------------------------------------------------------
First pass. Maybe by the 3rd pass I'll have an idea of whats going on. General comment is that there is a lot of new code here but tests seem to test replication system. There are few instances of unit tests ensuring newly added methods are working properly. src/main/java/org/apache/hadoop/hbase/replication/master/ReplicationLogCleaner.java <http://review.hbase.org/r/76/#comment894> For sure setConf will have been called before we get here? So, stuff gets setup by setConf? Can setConf be called more than once? How do I know how to use this class? Not doc'd. Doesn't have a Constructor. src/main/java/org/apache/hadoop/hbase/replication/master/ReplicationLogCleaner.java <http://review.hbase.org/r/76/#comment895> The way this is done, if I didn't want to wait on the ttl, then I'd have to write a new class. Can't we have ttl and recplication be distinct and then if I want delete based off ttl and whether log up in zk, then chain them? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java <http://review.hbase.org/r/76/#comment896> I dont follow? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java <http://review.hbase.org/r/76/#comment897> Should read this out of config. rather than hardcode 10. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java <http://review.hbase.org/r/76/#comment898> Same here. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java <http://review.hbase.org/r/76/#comment900> Long while loop; can break it up? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java <http://review.hbase.org/r/76/#comment899> Only operate on the first kv? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java <http://review.hbase.org/r/76/#comment901> Do you have to write position back to zk? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java <http://review.hbase.org/r/76/#comment903> Can code from HLog be used here? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment904> This ain't a constructor? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment907> We have to copy? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceInterface.java <http://review.hbase.org/r/76/#comment908> Not a constructor. If javadoc in an interface, you don't need to reproduce the javadoc in the implementation. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment909> This should be SortedSet, not TreeSet... or NavigableSet. src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java <http://review.hbase.org/r/76/#comment910> Good src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java <http://review.hbase.org/r/76/#comment911> What does this class do? src/test/java/org/apache/hadoop/hbase/replication/TestReplicationSource.java <http://review.hbase.org/r/76/#comment912> No dfs in this test. Thats intentional? src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSink.java <http://review.hbase.org/r/76/#comment913> Can't you squash some of these tests together? They each start up own minidfscluster... just start it once? - stack On 2010-06-08 17:54:19, Jean-Daniel Cryans wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.hbase.org/r/76/ > ----------------------------------------------------------- > > (Updated 2010-06-08 17:54:19) > > > Review request for hbase. > > > Summary > ------- > > This is HBASE-2223 AKA Replication 2.0, it is currently only a "preview > patch" as it's pretty much feature complete, works on a cluster, has unit > tests and whatnot, but it could use a lot more testing and cleaning and ideas > from others. > > > This addresses bug HBASE-2223. > http://issues.apache.org/jira/browse/HBASE-2223 > > > Diffs > ----- > > bin/replication/add_peer.rb PRE-CREATION > bin/replication/copy_tables_desc.rb PRE-CREATION > pom.xml 03c6ec8 > src/main/java/org/apache/hadoop/hbase/HConstants.java 13aff26 > src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java b36f1df > src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 82148a6 > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java > a1baff4 > src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 034690e > src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogKey.java 5d4cffe > > src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperHelper.java > PRE-CREATION > > src/main/java/org/apache/hadoop/hbase/replication/master/ReplicationLogCleaner.java > PRE-CREATION > src/main/java/org/apache/hadoop/hbase/replication/package.html PRE-CREATION > > src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java > PRE-CREATION > > src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java > PRE-CREATION > > src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceInterface.java > PRE-CREATION > > src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java > PRE-CREATION > src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java 2f2f306 > > src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java > PRE-CREATION > src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java > PRE-CREATION > > src/test/java/org/apache/hadoop/hbase/replication/TestReplicationSource.java > PRE-CREATION > > src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSink.java > PRE-CREATION > > src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java > PRE-CREATION > > Diff: http://review.hbase.org/r/76/diff > > > Testing > ------- > > > Thanks, > > Jean-Daniel > >
