> On 2010-05-26 23:48:35, Benoit Sigoure wrote: > > src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java, > > line 120 > > <http://review.hbase.org/r/76/diff/2/?file=664#file664line120> > > > > I don't find `replication.source.size.capacity' particularly explicit. > > What is it supposed to control?
I'll put more details into their attributes. Also if we document those configs I will add details in hbase-default.xml > On 2010-05-26 23:48:35, Benoit Sigoure wrote: > > src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java, > > line 167 > > <http://review.hbase.org/r/76/diff/2/?file=664#file664line167> > > > > Nit pick: mapOfAddr since in English "Address" starts with "Addr" :) Avoue que ça te mélanges autant que moi ;) > On 2010-05-26 23:48:35, Benoit Sigoure wrote: > > src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java, > > line 179 > > <http://review.hbase.org/r/76/diff/2/?file=664#file664line179> > > > > If HServerAddress respects the equal contract, consider using a set > > instead of a map. And then, even better, might as well just use a HashSet :P > On 2010-05-26 23:48:35, Benoit Sigoure wrote: > > src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java, > > line 208 > > <http://review.hbase.org/r/76/diff/2/?file=664#file664line208> > > > > Is there any reason why you're checking this here? It's a symptom of a problem with my code, I'll fix that. > On 2010-05-26 23:48:35, Benoit Sigoure wrote: > > src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java, > > line 229 > > <http://review.hbase.org/r/76/diff/2/?file=664#file664line229> > > > > I find this comment confusing. Can you make it a bit more explicit, > > especially towards the "normally has a position" part? Yeah, in fact I could just remove it... It means that we recovered a queue from another region server, and normally you are tailing a file but a RS could fail between 2 logs and that means that the new one wasn't read from yet. > On 2010-05-26 23:48:35, Benoit Sigoure wrote: > > src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java, > > line 268 > > <http://review.hbase.org/r/76/diff/2/?file=664#file664line268> > > > > All the double-negations with noIOE are harder to read than if instead > > you were starting with boolean gotIOE = false and set it to true when you > > get an IOE. y > On 2010-05-26 23:48:35, Benoit Sigoure wrote: > > src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java, > > line 270 > > <http://review.hbase.org/r/76/diff/2/?file=664#file664line270> > > > > This `try' block is massive, would it be possible to refactor it using > > a private method to make the code a bit more readable? I'll do my best. > On 2010-05-26 23:48:35, Benoit Sigoure wrote: > > src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java, > > line 329 > > <http://review.hbase.org/r/76/diff/2/?file=664#file664line329> > > > > It seems that considerDumping will always be true except when you fail > > to stat() the file due to an unexpected error. That seems suspicious to > > me. Is this intended? If yes, can you explain? I agree that part is weird, I built this while HDFS wasn't stable yet for tail'ing files and ended up having to decide if I should ditch a file that was broken. Currently you consider dumping the file when you get an EOF and if: - The queue was recovered and is empty. It's very suspicious. Usually you get EOFs on file that are opened but totally empty. Might as well get rid of it? - The number of entries read is greater than 0. That means you were reading the file, and instead of just getting null at the end you get an EOF in the face. By experience, it's usually a race condition à la HDFS-1057 then just redo the reading. If it failed 10 times, it means that that file is totally broken. Comments? > On 2010-05-26 23:48:35, Benoit Sigoure wrote: > > src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java, > > line 109 > > <http://review.hbase.org/r/76/diff/2/?file=666#file666line109> > > > > Does this statement need to be in the synchronized block? If yes, why? Nah doesn't need to. > On 2010-05-26 23:48:35, Benoit Sigoure wrote: > > src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java, > > line 191 > > <http://review.hbase.org/r/76/diff/2/?file=666#file666line191> > > > > Document. Already documented in the interface > On 2010-05-26 23:48:35, Benoit Sigoure wrote: > > src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java, > > line 206 > > <http://review.hbase.org/r/76/diff/2/?file=666#file666line206> > > > > Document. >From the interface > On 2010-05-26 23:48:35, Benoit Sigoure wrote: > > src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java, > > line 12 > > <http://review.hbase.org/r/76/diff/2/?file=668#file668line12> > > > > No javadoc in this file? All from the implemented interface > On 2010-05-26 23:48:35, Benoit Sigoure wrote: > > src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java, > > line 55 > > <http://review.hbase.org/r/76/diff/2/?file=669#file669line55> > > > > Why is this `protected' instead of `private'? Excellent question... > On 2010-05-26 23:48:35, Benoit Sigoure wrote: > > src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java, > > line 126 > > <http://review.hbase.org/r/76/diff/2/?file=669#file669line126> > > > > Pardon my ignorance but what's the `2' supposed to mean here? Start 2 region servers > On 2010-05-26 23:48:35, Benoit Sigoure wrote: > > src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java, > > line 151 > > <http://review.hbase.org/r/76/diff/2/?file=669#file669line151> > > > > Maybe you can add a watcher for yourself too so you get notified when > > you can continue instead of sleeping SLEEP_TIME and hoping that the timing > > will work out OK? Seems the class would be even more complicated. > On 2010-05-26 23:48:35, Benoit Sigoure wrote: > > src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java, > > line 182 > > <http://review.hbase.org/r/76/diff/2/?file=669#file669line182> > > > > Do you need an empty method? Was keeping it around in case I need it again. - Jean-Daniel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/76/#review84 ----------------------------------------------------------- On 2010-05-26 18:09:30, Jean-Daniel Cryans wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.hbase.org/r/76/ > ----------------------------------------------------------- > > (Updated 2010-05-26 18:09:30) > > > 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 > ----- > > src/main/java/org/apache/hadoop/hbase/HConstants.java 13aff26 > src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 4cbe52a > src/main/java/org/apache/hadoop/hbase/master/ServerManager.java a197b8f > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java > b5ff43a > src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 12a3cd8 > src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogKey.java 7c1184c > > 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 ed8709f > > 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 > >
