hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ryan Rawson" <ryano...@gmail.com>
Subject Re: Review Request: HBASE-2618 Don't inherit from HConstants.
Date Mon, 07 Jun 2010 19:46:58 GMT


> On 2010-06-06 09:08:19, Jonathan Gray wrote:
> > This patch changes a bunch of tabbing patterns to be different from what is currently
done in the codebase.  Primarily the full indenting of arguments to align with each other.
> > 
> > As far as I know, this format is not used anywhere else in HBase.  I don't think
patches to clean up HConstants inheriting should do a partial change to a different style
for something unrelated?
> 
> Benoit Sigoure wrote:
>     As far as I can tell HBase doesn't seem to follow any consistent coding style, so
I did what everyone else seems to be doing, I let my editor do the re-indenting the way I
typically do.
>     I'll upload a new patch with the indentation change.
>     
>     Other than that, what do you think about the change?
> 
> Jonathan Gray wrote:
>     It is mostly consistent.  In general the practice is to keep the same styling as
the file you're in or if a new file, in whatever else is like it that's already there.
>     
>     Other than that, I'm a big +1 on removing the extending interface pattern we use
on HConstants and your patch looks good to me on that front.
> 
> Jonathan Gray wrote:
>     As far as the tests go, I've never actually seen that failing pattern.  I've been
running unit tests on trunk in at least 4 different environments.
>     
>     I wonder where this is coming from... Is there a jira open for this specific failure?
 If not, let's open one so we can get to the bottom of it.

I have noticed that HBase is mostly K&R with a continuation indent not alignment.  It
tends to match the IntelliJ default code style with tabs=2, continuation-indent=4.  Perhaps
tweak your editor to only use align-braces on method parameters (as per IJ) and do continuation
indent on all others.


- Ryan


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


On 2010-06-05 23:47:11, Benoit Sigoure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/132/
> -----------------------------------------------------------
> 
> (Updated 2010-06-05 23:47:11)
> 
> 
> Review request for hbase.
> 
> 
> Summary
> -------
> 
> HBASE-2618 Don't inherit from HConstants.
> 
> Bonus: minor aesthetic / coding style clean ups and minor code changes.
> 
> 
> This addresses bug HBASE-2618.
> 
> 
> Diffs
> -----
> 
>   trunk/src/main/java/org/apache/hadoop/hbase/HMerge.java 951516 
>   trunk/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java 951516 
>   trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 951516 
>   trunk/src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java 951516 
>   trunk/src/main/java/org/apache/hadoop/hbase/master/BaseScanner.java 951516 
>   trunk/src/main/java/org/apache/hadoop/hbase/master/HMaster.java 951516 
>   trunk/src/main/java/org/apache/hadoop/hbase/master/RegionManager.java 951516 
>   trunk/src/main/java/org/apache/hadoop/hbase/master/RegionServerOperation.java 951516

>   trunk/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 951516 
>   trunk/src/main/java/org/apache/hadoop/hbase/master/TableOperation.java 951516 
>   trunk/src/main/java/org/apache/hadoop/hbase/regionserver/CompactSplitThread.java 951516

>   trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 951516 
>   trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 951516

>   trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 951516 
>   trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java 951516 
>   trunk/src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 951516 
>   trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/HQuorumPeer.java 951516 
>   trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZKServerTool.java 951516 
>   trunk/src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWrapper.java 951516

>   trunk/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java 951516 
>   trunk/src/test/java/org/apache/hadoop/hbase/PerformanceEvaluation.java 951516 
>   trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestExplicitColumnTracker.java
951516 
>   trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestGetClosestAtOrBefore.java
951516 
>   trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestGetDeleteTracker.java
951516 
>   trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestKeyValueHeap.java 951516

>   trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestQueryMatcher.java 951516

>   trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestScanDeleteTracker.java
951516 
>   trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestWildcardColumnTracker.java
951516 
>   trunk/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java 951516 
> 
> Diff: http://review.hbase.org/r/132/diff
> 
> 
> Testing
> -------
> 
> Code compiles.
> Tests are acting up on my machine right now (many of them fail with a weird message [1]
and Todd says he's been seeing similar failures for some time already, so I guess I'll try
to run them again next week when the New Moon arrives).
> 
> 
> [1] A number of tests fail with:
> org.apache.hadoop.hbase.client.NoServerForRegionException: Timed out trying to locate
root region because: Failed setting up proxy to /192.168.0.7:63773 after attempts=1
>         at org.apache.hadoop.hbase.client.HConnectionManager$TableServers.locateRootRegion(HConnectionManager.java:1031)
> Where, of course, 192.168.0.7 is my IP address.
> Some of the tests that are acting up:
> org.apache.hadoop.hbase.TestZooKeeper, org.apache.hadoop.hbase.regionserver.wal.TestLogRolling,
org.apache.hadoop.hbase.rest.TestScannersWithFilters, org.apache.hadoop.hbase.master.TestMasterWrongRS,
org.apache.hadoop.hbase.thrift.TestThriftServer, org.apache.hadoop.hbase.master.TestMasterTransitions,
org.apache.hadoop.hbase.rest.TestStatusResource, org.apache.hadoop.hbase.client.TestFromClientSide,
org.apache.hadoop.hbase.TestMultiParallelPut, org.apache.hadoop.hbase.master.TestRegionManager,
org.apache.hadoop.hbase.mapreduce.TestTimeRangeMapRed
> 
> 
> Thanks,
> 
> Benoit
> 
>


Mime
View raw message