----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/76/#review84 -----------------------------------------------------------
I'll pick up where I stopped in TestReplication.java tomorrow. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment412> Terminate this sentence with a period. Generally speak you must terminate the first sentence of a javadoc with a period. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment413> I don't find `replication.source.size.capacity' particularly explicit. What is it supposed to control? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment414> Ditto. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment415> This is unnecessary. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment416> Nit pick: mapOfAddr since in English "Address" starts with "Addr" :) src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment417> s/adr/addr/ :) src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment418> How about using a do-while instead of repeating this line of code? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment419> If HServerAddress respects the equal contract, consider using a set instead of a map. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment420> Move this outside of the try block. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment422> The explicit call to toString() here is useless. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment421> Is there any reason why you're checking this here? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment423> Log a message too. This will call Log#error(java.lang.Object message) and I presume you meant to call Log#error(java.lang.Object message, java.lang.Throwable t) src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment424> I find this comment confusing. Can you make it a bit more explicit, especially towards the "normally has a position" part? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment425> Don't lock in the `try' block. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment426> Why are you checking this again here? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment432> 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. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment427> This `try' block is massive, would it be possible to refactor it using a private method to make the code a bit more readable? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment428> Use a local variable for entry.getKey() instead of calling it so many times in a row. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment429> Unnecessary call to toString(). src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment430> 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? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment431> Need a message in first argument. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment433> Please document. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment434> Please document everything properly. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment435> Document. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment436> How about storing the result of edit.getKeyValues() in a local instead of repeating that code and repeating the call twice at every loop? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment437> Please document everything. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment438> Put this in a try-finally block. Just because you're nulling one reference doesn't protect you from this thread getting interrupted and then you're left with a locked lock. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment439> Why are you doing this? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment440> s/adr/addr/ src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java <http://review.hbase.org/r/76/#comment441> :) src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceInterface.java <http://review.hbase.org/r/76/#comment442> Please document. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment443> You need to enclose te <li> in <ul> or <ol> and close the tags properly. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment444> Make this private. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment445> Can you please document everything non obvious. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment446> s/ and that/. This/ src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment447> ?? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment448> Document. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment449> queueRecovered should be documented too. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment450> Does this statement need to be in the synchronized block? If yes, why? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment451> Unnecessary call to toString(). src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment452> "Add" vs "tries" – be consistent. Use imperative everywhere or don't use it. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment453> Since log messages can be intertwined if the server is busy logging a lot, how about making this and the previous line a single message. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment457> Document. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment454> This doesn't need to be in the synchronized block, does it? src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment456> Document. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment455> Document. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment458> Remove this line and the next to avoid code duplication with line 250/251. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment459> Don't do this. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment460> Document. src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java <http://review.hbase.org/r/76/#comment461> Don't you only need to synchronize on those two lines instead of all the other ones too? src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java <http://review.hbase.org/r/76/#comment464> No copyright? src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java <http://review.hbase.org/r/76/#comment463> No javadoc in this file? src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java <http://review.hbase.org/r/76/#comment462> Not properly indented. src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java <http://review.hbase.org/r/76/#comment465> Why is this `protected' instead of `private'? src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java <http://review.hbase.org/r/76/#comment466> Pardon my ignorance but what's the `2' supposed to mean here? src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java <http://review.hbase.org/r/76/#comment467> I'm not sure whether this is useful since junit will already give us all the info in the event of an unexpected exception. Furthermore, the first argument to LOG.error must be a message. src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java <http://review.hbase.org/r/76/#comment468> This would be a better API if it received a boolean in argument and transformed it in a String itself. src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java <http://review.hbase.org/r/76/#comment470> 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? src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java <http://review.hbase.org/r/76/#comment469> ?? src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java <http://review.hbase.org/r/76/#comment471> Do you need an empty method? - Benoit 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 > >
