[
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
