[
https://issues.apache.org/jira/browse/HBASE-2223?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12872119#action_12872119
]
HBase Review Board commented on HBASE-2223:
-------------------------------------------
Message from: "Benoit Sigoure" <[email protected]>
-----------------------------------------------------------
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:
^bq.
^bq. -----------------------------------------------------------
^bq. This is an automatically generated e-mail. To reply, visit:
^bq. http://review.hbase.org/r/76/
^bq. -----------------------------------------------------------
^bq.
^bq. (Updated 2010-05-26 18:09:30)
^bq.
^bq.
^bq. Review request for hbase.
^bq.
^bq.
^bq. Summary
^bq. -------
^bq.
^bq. 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.
^bq.
^bq.
^bq. This addresses bug HBASE-2223.
^bq. http://issues.apache.org/jira/browse/HBASE-2223
^bq.
^bq.
^bq. Diffs
^bq. -----
^bq.
^bq. src/main/java/org/apache/hadoop/hbase/HConstants.java 13aff26
^bq. src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 4cbe52a
^bq. src/main/java/org/apache/hadoop/hbase/master/ServerManager.java a197b8f
^bq. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
b5ff43a
^bq. src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java
12a3cd8
^bq. src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogKey.java
7c1184c
^bq.
src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperHelper.java
PRE-CREATION
^bq.
src/main/java/org/apache/hadoop/hbase/replication/master/ReplicationLogCleaner.java
PRE-CREATION
^bq. src/main/java/org/apache/hadoop/hbase/replication/package.html
PRE-CREATION
^bq.
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java
PRE-CREATION
^bq.
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
PRE-CREATION
^bq.
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceInterface.java
PRE-CREATION
^bq.
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
PRE-CREATION
^bq. src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java ed8709f
^bq.
src/test/java/org/apache/hadoop/hbase/replication/ReplicationSourceDummy.java
PRE-CREATION
^bq. src/test/java/org/apache/hadoop/hbase/replication/TestReplication.java
PRE-CREATION
^bq.
src/test/java/org/apache/hadoop/hbase/replication/TestReplicationSource.java
PRE-CREATION
^bq.
src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSink.java
PRE-CREATION
^bq.
src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java
PRE-CREATION
^bq.
^bq. Diff: http://review.hbase.org/r/76/diff
^bq.
^bq.
^bq. Testing
^bq. -------
^bq.
^bq.
^bq. Thanks,
^bq.
^bq. Jean-Daniel
^bq.
^bq.
> Handle 10min+ network partitions between clusters
> -------------------------------------------------
>
> Key: HBASE-2223
> URL: https://issues.apache.org/jira/browse/HBASE-2223
> Project: HBase
> Issue Type: Sub-task
> Reporter: Jean-Daniel Cryans
> Assignee: Jean-Daniel Cryans
> Fix For: 0.21.0
>
> Attachments: HBASE-2223.patch
>
>
> We need a nice way of handling long network partitions without impacting a
> master cluster (which pushes the data). Currently it will just retry over and
> over again.
> I think we could:
> - Stop replication to a slave cluster if it didn't respond for more than 10
> minutes
> - Keep track of the duration of the partition
> - When the slave cluster comes back, initiate a MR job like HBASE-2221
> Maybe we want less than 10 minutes, maybe we want this to be all automatic or
> just the first 2 parts. Discuss.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
