hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jean-Daniel Cryans" <jdcry...@apache.org>
Subject Re: Review Request: HBASE-2223
Date Mon, 14 Jun 2010 18:37:29 GMT


> On 2010-06-11 12:45:29, stack wrote:
> > bin/replication/add_peer.rb, line 21
> > <http://review.hbase.org/r/76/diff/5/?file=1104#file1104line21>
> >
> >     Should you point at some replication documentation here?  Is there such a thing?

package.html later, should I point to it?


> On 2010-06-11 12:45:29, stack wrote:
> > bin/replication/copy_tables_desc.rb, line 58
> > <http://review.hbase.org/r/76/diff/5/?file=1105#file1105line58>
> >
> >     This could get a bit annoying I'd say.

It helped me a lot, remove if people complain?


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/HConstants.java, line 342
> > <http://review.hbase.org/r/76/diff/5/?file=1107#file1107line342>
> >
> >     This has to go here?  Can it go into one of the replication classes?

Used by master and region server, to me it belongs there.


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/master/ServerManager.java, line 156
> > <http://review.hbase.org/r/76/diff/5/?file=1109#file1109line156>
> >
> >     Can't you just do c.get("key", defaultvalue)?

No, I also do a check on replication.


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 929
> > <http://review.hbase.org/r/76/diff/5/?file=1110#file1110line929>
> >
> >     You writing startcode into zk?  Why not write servername -- the host+port+startcode
combo?

To be coherent with the rest of the code that uses zookeeper.


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java, line 1075
> > <http://review.hbase.org/r/76/diff/5/?file=1110#file1110line1075>
> >
> >     Is this directory name?  Confusingly named given rootdir+regLogPathStr only
adds up to repLogPath.

I don't understand you, but this code is going to be removed in my next patch as I'm simplifying
RepSink.


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperHelper.java,
line 55
> > <http://review.hbase.org/r/76/diff/5/?file=1113#file1113line55>
> >
> >     Peers are named '1', '2'?  Can't we have more meaningful names here?

We agreed that peers are identified with a short internally as it is stored. We could use
an external mapping of short->cute_name.


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperHelper.java,
line 59
> > <http://review.hbase.org/r/76/diff/5/?file=1113#file1113line59>
> >
> >     Use servername instead of startcode

Same comment as before, needs to be coherent.


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperHelper.java,
line 60
> > <http://review.hbase.org/r/76/diff/5/?file=1113#file1113line60>
> >
> >     All RS's in a master cluster replicate?

Yep... was that an implicit way of saying that I need to document that in RZH?


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperHelper.java,
line 107
> > <http://review.hbase.org/r/76/diff/5/?file=1113#file1113line107>
> >
> >     Should this class be called WRapper instaad of Helper?

Sure


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperHelper.java,
line 185
> > <http://review.hbase.org/r/76/diff/5/?file=1113#file1113line185>
> >
> >     You mean 'ensemble' here rather than 'quorum' (Patrick will kill you if he sees
you calling it a 'quorum' when you mean the other)

Argh I'm trying to correct myself but I'm still missing some of them. Thx!


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperHelper.java,
line 263
> > <http://review.hbase.org/r/76/diff/5/?file=1113#file1113line263>
> >
> >     We keep up the replication position in zk?  How much do we replicate in one
go?  Its not a single edit, is it?  We do this for every log file?

Yes. A defined amount specified in ReplicationSource. No. Every current log file, we only
replicate one at a time per region server.


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperHelper.java,
line 328
> > <http://review.hbase.org/r/76/diff/5/?file=1113#file1113line328>
> >
> >     LOG.warn instead?
> >

I'll do like the rest and log.error


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperHelper.java,
line 354
> > <http://review.hbase.org/r/76/diff/5/?file=1113#file1113line354>
> >
> >     We return empty map if clusters size is == 1?  Should that be clusters.size
== 0?

That part isn't clear enough, so the reason it's 1 and not 0 is that we put a lock in there
so it's listed in the znodes we fetch. Actually this should be <= 1 rather than ==.


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperHelper.java,
line 356
> > <http://review.hbase.org/r/76/diff/5/?file=1113#file1113line356>
> >
> >     Whats this about?

See previous comment, we lock the dead region server's znode by putting a lock in there, but
we don't want to process the hlogs under since... it's not a cluster. Could use more doc.


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/replication/ReplicationZookeeperHelper.java,
line 402
> > <http://review.hbase.org/r/76/diff/5/?file=1113#file1113line402>
> >
> >     Just logging errors?  What if session expired (our discussion from last day)?

Yes I need to review how I handle it in RZH, but I'd also need to review ZKW since some methods
will hid it in there.


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/replication/package.html, line 41
> > <http://review.hbase.org/r/76/diff/5/?file=1115#file1115line41>
> >
> >     Call it alpha

yeah! (j/k)


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/replication/package.html, line 64
> > <http://review.hbase.org/r/76/diff/5/?file=1115#file1115line64>
> >
> >     Whats this about?  You need to run zk yourself but no zoo.cfg?

I... don't remember why I wrote this.


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/replication/package.html, line 73
> > <http://review.hbase.org/r/76/diff/5/?file=1115#file1115line73>
> >
> >     And if not?  What if replicating single-family only?

Forgot to update that after we added scoping, updating.


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/replication/package.html, line 83
> > <http://review.hbase.org/r/76/diff/5/?file=1115#file1115line83>
> >
> >     Has to be offline?  Will this always be the case?

Currently everything is static, but I hope we can move on from that in the future.


> On 2010-06-11 12:45:29, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/replication/package.html, line 108
> > <http://review.hbase.org/r/76/diff/5/?file=1115#file1115line108>
> >
> >     whats ratio?

This is a log snippet that's coming from a region server. Do you want to see more documentation
about it in package.html or in the logging itself?


- Jean-Daniel


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/76/#review191
-----------------------------------------------------------


On 2010-06-08 17:54:19, Jean-Daniel Cryans wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/76/
> -----------------------------------------------------------
> 
> (Updated 2010-06-08 17:54:19)
> 
> 
> 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
> -----
> 
>   bin/replication/add_peer.rb PRE-CREATION 
>   bin/replication/copy_tables_desc.rb PRE-CREATION 
>   pom.xml 03c6ec8 
>   src/main/java/org/apache/hadoop/hbase/HConstants.java 13aff26 
>   src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java b36f1df 
>   src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 82148a6 
>   src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java a1baff4 
>   src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 034690e 
>   src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogKey.java 5d4cffe 
>   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 2f2f306 
>   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
> 
>


Mime
View raw message