hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "mbautin (Mikhail Bautin)" <phabrica...@reviews.facebook.net>
Subject [Differential] [Commented On] D87: [jira] [HBASE-4686] [89-fb] Fix per-store metrics aggregation
Date Mon, 31 Oct 2011 23:57:47 GMT
mbautin has commented on the revision "[jira] [HBASE-4686] [89-fb] Fix per-store metrics aggregation

  OK, this is now becoming a code style discussion, which is not a bad thing once in a while

  In general, the style guide for HBase seems to point to Oracle (former Sun)'s coding conventions:
http://www.oracle.com/technetwork/java/codeconvtoc-136057.html, with the part pertaining to
Javadoc at http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html.
I guess it might not be a bad idea to actually read those docs some time...

  src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java:386 Actually
I a lot of places around the codebase don't have these empty lines inside javadoc comments,
and I think it is not necessary here. Java formatter adds trailing whitespace to those empty
lines, which is annoying. I did not find a requirement that these lines have to be there at
http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html (although I
did not read that entire style guide).

  I will expand the documentation  of tmpMap.
  src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java:554 Will fix.
  src/main/java/org/apache/hadoop/hbase/regionserver/metrics/SchemaMetrics.java:704 Agreed.
  src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java:1148 I think this saves space
and is OK for one-line comments. This parses fine by Javadoc.
  src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java:1157 This empty line separates
the bulky function header and the function body.
  src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java:1219 I will add one between
1218 and 1219.
  src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java:90 OK.
  src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java:119 This
one is a matter of preference. There is one at the top, so I will keep the one at the bottom
of the class definition.


View raw message