accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Christopher Tubbs" <ctubbsii...@apache.org>
Subject Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo
Date Tue, 06 Jan 2015 21:03:51 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29176/#review66906
-----------------------------------------------------------



core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java
<https://reviews.apache.org/r/29176/#comment110609>

    Should be generic interface "MetricsGatherer<T>" with a method "T getMetrics();"
to expose the metrics in ways other than printing.



core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java
<https://reviews.apache.org/r/29176/#comment110603>

    The javadoc should describe when init is called (eg. when it is registered with the RFile
Reader).



core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java
<https://reviews.apache.org/r/29176/#comment110604>

    This javadoc is incorrect, based on its usage here. What appears to be passed in is the
column family, not the locality group name.
    
    "new" also seems to imply something is being created, when it simply indicates that a
new one has been encountered. Maybe the "start" or "begin" instead of "new" would be more
clear.
    
    [I'm also entertaining the idea that this method could just take the first key (or key/value
pair) from the new locality group in order to make fewer assumptions about how it is used,
but I haven't fully convinced myself that's better than strictly defining this method to take
the column family portion only.]



core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java
<https://reviews.apache.org/r/29176/#comment110605>

    This could take a key and a value as two parameters, to enable gathering metrics of more
things.



core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java
<https://reviews.apache.org/r/29176/#comment110606>

    could be called "startBlock" or "beginBlock" instead of "newBlock" to avoid the implication
that something new is being created.



core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java
<https://reviews.apache.org/r/29176/#comment110607>

    javadoc should describe the output format, or rely on proposed getMetrics() method and
defer the printing to the PrintInfo command.



core/src/main/java/org/apache/accumulo/core/file/rfile/PrintInfo.java
<https://reviews.apache.org/r/29176/#comment110608>

    description should specify whether this "requires -v" or "implies -v"



core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java
<https://reviews.apache.org/r/29176/#comment110610>

    toString isn't safe here; probably better to just pass the column family directly, or
the whole key and let the gatherer decide how to use it.



core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java
<https://reviews.apache.org/r/29176/#comment110612>

    Needs javadoc. Should specify that you can only register one (it will clobber any previously
set one), and that you should do so before iterating. This could be enforced, by checking
state when this method is called, but minimally, the javadocs should explain.


- Christopher Tubbs


On Dec. 22, 2014, 10:25 a.m., Jenna Huston wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29176/
> -----------------------------------------------------------
> 
> (Updated Dec. 22, 2014, 10:25 a.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-3420
>     https://issues.apache.org/jira/browse/ACCUMULO-3420
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> Added an option to PrintInfo to get visibility metrics.  Prints the number of times a
visibilty is seen in each locality group.  Also shows how many blocks in the locality group
have his visibiltiy.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java PRE-CREATION

>   core/src/main/java/org/apache/accumulo/core/file/rfile/PrintInfo.java 43586dd 
>   core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java 9dcb3a5 
>   core/src/main/java/org/apache/accumulo/core/file/rfile/VisMetricsGatherer.java PRE-CREATION

>   core/src/test/java/org/apache/accumulo/core/file/rfile/RFileMetricsTest.java PRE-CREATION

> 
> Diff: https://reviews.apache.org/r/29176/diff/
> 
> 
> Testing
> -------
> 
> Tested with a few RFiles that I made.
> 
> 
> Thanks,
> 
> Jenna Huston
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message