hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "stack (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-4199) blockCache summary - backend
Date Thu, 18 Aug 2011 21:24:27 GMT

    [ https://issues.apache.org/jira/browse/HBASE-4199?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13087302#comment-13087302
] 

stack commented on HBASE-4199:
------------------------------

Here's some feedback.

This is very nice funcionality.  Thanks for adding it.

The constructor that takes no args has empty javadoc.  Remove it or fill it in (perhaps note
this method is for deserializing only)

No need to assign "" to table and columnfamily on declaration?  Let a NPE out if this class
ill-initialized

You are inconsistent w/ lines separating methods.  Looks sloppy (Usually we put a single line
between methods)

Whats up with this BlockCacheSummaryEntry?  Its a summary or is it an entry?  If a summary,
its odd that I can set things like blocks and heapSize post construction and even table and
columnfamily.  Maybe this should be an immmutable object where you pass in all variables on
construction?

I suppose its a summary for the passed table and cf?  Hmm.  Thats what you say in the class
javadoc so strike the above rumination on class name (the whether it should be immutable comment
stands).

+    this.heapSize = this.heapSize + heapSize; is usually written this.heapSize += heapSize...
but no biggie.

In the below
{code}
+  public void readFields(DataInput arg0) throws IOException {
{code}

'arg0' is a bad name for a param (Same on the write).

In hashCode you do:

{code}
+    final int prime = 31;
+    int result = 1;
+    result = prime * result
...
{code}

Why the 'int result = 1' at all?  Why not 'int result = prime + ....'

I like your equals implementation.

In your toString, why include name of this class?

Should we throw exception if path does not have four elements in it?

Would suggest you add the stuff you have in comments down in createFromStoreFilePath up into
the javadoc; would help clarify the kinda path we are expecting.

You are inconsistent with spacings below:

{code}
+      String table = s[ s.length - 4];  // 4th from the end
+      String cf = s[ s.length - 2];     // 2nd from the end
{code}

Be careful w/ line lengths.  Usually < 80.

How often is getBlockCacheSummary called?  For each invocation we do inspection of all under
hbase.rootdir?  This seems like a pretty costly operation.

Looking at getTableStoreFilePathMap, we should make a storefiles filter.  Seems like it'd
get used more than once (but thats for another JIRA).

This comment looks wrong?  " Under each of these, should be one file only."

Why do this:

{code}
+    BlockCacheSummaryEntry e2 = new BlockCacheSummaryEntry();
+    e2.setTable(e.getTable());
+    e2.setColumnFamily(e.getColumnFamily());
+    return e2;
{code}

Why not do return BlockCacheSummaryEntry(e.getTable(), e.getColumnFamily());

The below should be a Set?

{code}
+    Map<BlockCacheSummaryEntry, BlockCacheSummaryEntry> bcs = 
+      new HashMap<BlockCacheSummaryEntry, BlockCacheSummaryEntry>();
{code}

Or, strike that... I see how you are using it.  Its a little unusual that equality is on only
two of the datamembers.  Nothing wrong with this but I'd call this out in the class comment
for this class, that two instances are compared the same if table+cf agree (though counts
differ).

I like doing if (s.length <= 0) continue rather than below.

{code}
+      if (s.length > 0) {
{code}

Advantage of former is that save an indentation.  Similar for the if (path != null).. I'd
rather do if (path == null) continue;

In javadoc should you should say that getBlockCacheSummary returns a list sorted by table
name + cf (or you don't want to have sort in contract for this method?)

Why do this:

{code}
+    BlockCacheSummaryEntry[] ar = new BlockCacheSummaryEntry[list.size()];
+    for (int i = 0; i < list.size(); i++) {
+      ar[i]=list.get(i);
+    }
+    return ar;
{code}

Why not return the List?  (In future, doing something like above, you can use http://download.oracle.com/javase/6/docs/api/java/util/List.html#toArray(T[]))

I see now that this is a feature use with low frequency so the fact that it is heavyweight
should be fine.  You might add this to javadoc though that it includes scan of fs

Why the double javadoc?  You have 'Performs block cache summary' but then you also have @Override
(we will pick up the javadoc from the interface so the extra stuff in here from HREgionServer
is not needed).

Tests look good.

Just remove the below:

{code}
+  public void setUp()  {
+  }
{code}

Remove javadoc w/ nothing in it.  Looks bad.

Good stuff Doug.



> blockCache summary - backend
> ----------------------------
>
>                 Key: HBASE-4199
>                 URL: https://issues.apache.org/jira/browse/HBASE-4199
>             Project: HBase
>          Issue Type: Sub-task
>            Reporter: Doug Meil
>            Assignee: Doug Meil
>            Priority: Minor
>         Attachments: java_HBASE_4199.patch, java_HBASE_4199_v2.patch, java_HBASE_4199_v3.patch
>
>
> This is the backend work for the blockCache summary.  Change to BlockCache interface,
Summarization in LruBlockCache, BlockCacheSummaryEntry, addition to HRegionInterface, and
HRegionServer.
> This will NOT include any of the web UI or anything else like that.  That is for another
sub-task.

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message