Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
HBase >> mail # dev >> Re: Review Request: HBASE-2223


Copy link to this message
-
Re: Review Request: HBASE-2223

-----------------------------------------------------------
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 locke