[
https://issues.apache.org/jira/browse/HBASE-2223?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12882738#action_12882738
]
HBase Review Board commented on HBASE-2223:
-------------------------------------------
Message from: [email protected]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/76/#review281
-----------------------------------------------------------
Missing is more desciption of how this feature works. There is package doc. on
how to get it going but needs fuller description of how replication works (how
zk is used, how it manages hlogs, how it sends edits to remote cluster, etc.).
There is insufficient description in javadoc. Without this, there is a danger
that only the author will be able to figure whats going on in here.
How does one get insight on to how well or badly replication is working?
Alot of errors are logged and then we just move on. Thats fine for an alpha
package but need a plan for making it all more robust? What you thinking?
bin/replication/add_peer.rb
<http://review.hbase.org/r/76/#comment1187>
These log messages add nothing? Remove? Just restating what was passed on
cmdline.
bin/replication/add_peer.rb
<http://review.hbase.org/r/76/#comment1188>
Why do this? Its annoying?
It'd be better spending time interrogating what the user passed to see if
it makes sense throwing errors if incorrectly formatted or if it doesn't look
like it makes sense?
bin/replication/add_peer.rb
<http://review.hbase.org/r/76/#comment1189>
A message on end saying what it did can be comforting to users.
bin/replication/copy_tables_desc.rb
<http://review.hbase.org/r/76/#comment1190>
See above comments.
src/main/java/org/apache/hadoop/hbase/HConstants.java
<http://review.hbase.org/r/76/#comment1191>
Change name of this config to hbase.replication.
src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java
<http://review.hbase.org/r/76/#comment1192>
Does this method have prerequisites? For example, does replication have to
be enabled?
src/main/java/org/apache/hadoop/hbase/master/HMaster.java
<http://review.hbase.org/r/76/#comment1193>
Is this a leak from another patch?
src/main/java/org/apache/hadoop/hbase/master/RegionServerOperationQueue.java
<http://review.hbase.org/r/76/#comment1194>
This is a leak from another patch?
src/main/java/org/apache/hadoop/hbase/master/ServerManager.java
<http://review.hbase.org/r/76/#comment1195>
Whats going on here? We're setting into config the logcleaner to use but
then in the lines after this we go ahead and create OldLogsCleaner anyways?
src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<http://review.hbase.org/r/76/#comment1196>
Just call this 'replication' or 'replicationEnabled'
src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<http://review.hbase.org/r/76/#comment1198>
We are a ReplicationSink or a ReplicationMaster? One or the other? Can we
not move all of this Replication stuff into a Replication class including the
setup of whether we are a Sink or Master rather than have it hang out here in
HRS?
src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<http://review.hbase.org/r/76/#comment1197>
I don't grok the comment
src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
<http://review.hbase.org/r/76/#comment1199>
This could be null if we are a replication master?
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java
<http://review.hbase.org/r/76/#comment1200>
If the above mentioned Replication class were a singleton, it could be
shared here with HRS
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java
<http://review.hbase.org/r/76/#comment1201>
It'd be good if we didn't have to create this byte [] per edit. I should
reinstitute the comparator I had that took KVs but only compared family portion
of the KV... no need to create family byte [] then.
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java
<http://review.hbase.org/r/76/#comment1202>
YOu might say why its being moved. Should be INFO level? WALs are kinda
important to track?
src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperWrapper.java
<http://review.hbase.org/r/76/#comment1203>
isReplicating is what you would name the method that accesses the boolean
replicating... with this name the accessor should be named isIsReplicating.
src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperWrapper.java
<http://review.hbase.org/r/76/#comment1204>
Why do it this way? Why an if/else? Why not do if (...length != 3) throw
.... no need of an else.
src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperWrapper.java
<http://review.hbase.org/r/76/#comment1205>
This is an error? Is it critical fail (We dropped recording a WAL) Are we
just logging and moving on?
src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperWrapper.java
<http://review.hbase.org/r/76/#comment1206>
Same here.... We just log and keep going? Are any of these fatal?
src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperWrapper.java
<http://review.hbase.org/r/76/#comment1207>
If this fails, what are the repercusssions?
src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperWrapper.java
<http://review.hbase.org/r/76/#comment1208>
What happens if this server dies? It gets cleaned up by master?
What happens to replication if master dies? Does master failover need to
do anything special to pick up running replication?
src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperWrapper.java
<http://review.hbase.org/r/76/#comment1209>
spelling
src/main/java/org/apache/hadoop/hbase/replication/master/ReplicationLogCleaner.java
<http://review.hbase.org/r/76/#comment1213>
Why this implementation have to know about other implementations? Can't we
do a chain of decision classes? Any class can say no? As soon as any decision
class says no, we exit the chain.... So in this case, first on the chain would
be the ttl decision... then would be this one... and third would be the
snapshotting decision. You don't have to do the chain as part of this patch but
please open an issue to implement.
src/main/java/org/apache/hadoop/hbase/replication/package.html
<http://review.hbase.org/r/76/#comment1210>
Its not just MDC, right?
src/main/java/org/apache/hadoop/hbase/replication/package.html
<http://review.hbase.org/r/76/#comment1211>
Is it user tables or CF in user tables?
src/main/java/org/apache/hadoop/hbase/replication/package.html
<http://review.hbase.org/r/76/#comment1212>
Good doc.
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java
<http://review.hbase.org/r/76/#comment1214>
What is the 'main thread'?
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java
<http://review.hbase.org/r/76/#comment1215>
Can you make this clearer?
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java
<http://review.hbase.org/r/76/#comment1216>
Whats this mean? Lost edits?
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java
<http://review.hbase.org/r/76/#comment1217>
Maybe say something about how it works... pool of HTables to write remote
peer, etc.
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java
<http://review.hbase.org/r/76/#comment1218>
Why again does replication keep its own set of logs rather than just keep
positions on HRS logs (whether local HRS or the HRS of others?) So it prevails
across HRS failures? Aren't logs archived now rather than thrown away so this
should be fine? Otherwise, we are writing edits to FS twice, right?
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java
<http://review.hbase.org/r/76/#comment1220>
This should be inside a finally block?
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java
<http://review.hbase.org/r/76/#comment1221>
Duplicated code -- see up about ten lines.
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSink.java
<http://review.hbase.org/r/76/#comment1222>
You are stopping the regionserver because replication failed?
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
<http://review.hbase.org/r/76/#comment1223>
One second seems short?
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
<http://review.hbase.org/r/76/#comment1226>
We carry edits in memory? And its outside of our normal accounting -- e.g.
cache and max for memstores? How we get it into the mix? Else we could OOME
if we don't account for this memory payload.
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
<http://review.hbase.org/r/76/#comment1224>
Ratio?
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
<http://review.hbase.org/r/76/#comment1227>
Not a constructor
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
<http://review.hbase.org/r/76/#comment1228>
Stop cluster or stop the host?
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
<http://review.hbase.org/r/76/#comment1230>
Whats this about? IN particular, the 'handling story'...
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
<http://review.hbase.org/r/76/#comment1231>
When would it make sense to ever replicate catalog table entries?
Why we even in here reading a file if replicating == false
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
<http://review.hbase.org/r/76/#comment1232>
Are you checking size of these edits? You'll read 25k entries of any size?
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
<http://review.hbase.org/r/76/#comment1233>
Where do you explain replication hlog filenaming convention?
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceInterface.java
<http://review.hbase.org/r/76/#comment1234>
Can only do one sink, right? Might want to say so. How does the source
know the sink to replicate to? Should that be part of the Interface?
The comment for the Interface could say how the Interface is used... init,
then set sink, then per file archived, call ... etc.
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
<http://review.hbase.org/r/76/#comment1235>
Make this "This class manages all the replication sources"
"There are two classes of them"... what you mean to say here? Two classes
of source?
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
<http://review.hbase.org/r/76/#comment1236>
I don't follow this commentary.
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
<http://review.hbase.org/r/76/#comment1238>
You should say more here about whats going on.. either here or up in the
class comment. It has watchers on all other RSs in current cluster and will
try to take on the workloads of others on crash (?).
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
<http://review.hbase.org/r/76/#comment1239>
Comment is cut off.
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
<http://review.hbase.org/r/76/#comment1240>
You reporting status or updating updated position into zk? There is a
running status being kept up in zk? Shouldn't this log message have the log
path in it too?
This method does more than update position or report status... looks like
it changes state in this class updating set of logs?
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
<http://review.hbase.org/r/76/#comment1241>
Shouldn't this be nonmodifiable List that you give out here since it seems
you have it synchronized internally here.
src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSourceManager.java
<http://review.hbase.org/r/76/#comment1243>
What other regionserver? There is alot that goes unexplained in here?
Could point to the doc. on how replication works. Same for the transer method
above (I have an idea because you white boarded it for me).
src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java
<http://review.hbase.org/r/76/#comment1244>
Good.
- stack
> 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.
