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 Thu, 18 Dec 2014 17:20:22 GMT

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



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

    It'd be neat if there were a MetricsGatherer interface, which this class implemented.
That way, you could register other MetricsGatherers with the RFile Reader.
    
    The interface should have javadocs which explain when each method is expected to be called
(e.g., how they are used by RFile).



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

    The constructor doesn't need to take an RFile.Reader anymore, since you're using a visitor
pattern. When the gatherer is registered with the RFile, it can call an init method which
sets the locality groups or they can be populated as it progresses. The iter field doesn't
appear to be used for anything else.



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

    Might be useful to accept a PrintStream here instead of assume System.out.



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

    Maybe a better name is "registerMetricsGatherer" or "registerMetrics" or "gatherMetrics"
or similar.
    
    visMetrics parameter could also just be a check for null on visibilityMetrics (which should
be renamed, since the metrics doesn't have to be just be for visibilities).
    
    (Thought: might be useful to support a list of registered MetricsGatherers, so you could
pass more than one; then again, setting one that delegates to several others is possible without
making this change.)


- Christopher Tubbs


On Dec. 17, 2014, 3:31 p.m., Jenna Huston wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29176/
> -----------------------------------------------------------
> 
> (Updated Dec. 17, 2014, 3:31 p.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 
> 
> 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