Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Christopher Tubbs]

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

Ship it!


Ship It!

- Christopher Tubbs


On Jan. 9, 2015, 4:39 p.m., Jenna Huston wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29176/
> ---
> 
> (Updated Jan. 9, 2015, 4:39 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 
> f29efcc 
>   core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java 0b464d8 
>   
> core/src/main/java/org/apache/accumulo/core/file/rfile/VisMetricsGatherer.java
>  PRE-CREATION 
>   
> core/src/main/java/org/apache/accumulo/core/file/rfile/VisibilityMetric.java 
> PRE-CREATION 
>   
> core/src/test/java/org/apache/accumulo/core/file/rfile/RFileMetricsTest.java 
> PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/file/rfile/RFileTest.java 
> 1a83f33 
> 
> Diff: https://reviews.apache.org/r/29176/diff/
> 
> 
> Testing
> ---
> 
> Tested with a few RFiles that I made.
> 
> 
> Thanks,
> 
> Jenna Huston
> 
>

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Jenna Huston]

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

(Updated Jan. 9, 2015, 9:39 p.m.)


Review request for accumulo.


Changes
---

Fixed items from Christopher's feedback


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 (updated)
-

  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 f29efcc 
  core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java 0b464d8 
  
core/src/main/java/org/apache/accumulo/core/file/rfile/VisMetricsGatherer.java 
PRE-CREATION 
  core/src/main/java/org/apache/accumulo/core/file/rfile/VisibilityMetric.java 
PRE-CREATION 
  core/src/test/java/org/apache/accumulo/core/file/rfile/RFileMetricsTest.java 
PRE-CREATION 
  core/src/test/java/org/apache/accumulo/core/file/rfile/RFileTest.java 1a83f33 

Diff: https://reviews.apache.org/r/29176/diff/


Testing
---

Tested with a few RFiles that I made.


Thanks,

Jenna Huston

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Christopher Tubbs]

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



core/src/main/java/org/apache/accumulo/core/file/rfile/VisMetricsGatherer.java


Should have some javadoc explaining the kinds of metrics this class 
provides.

In other words, what can we expect to be contained in 
`Map>` when we call `getMetrics()`?



core/src/main/java/org/apache/accumulo/core/file/rfile/VisMetricsGatherer.java


Instead of serializing to a string, you could return use simple object 
(with getters) representing these pieces of data, and move the formatting 
outside.


- Christopher Tubbs


On Jan. 9, 2015, 11:19 a.m., Jenna Huston wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29176/
> ---
> 
> (Updated Jan. 9, 2015, 11:19 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 
> f29efcc 
>   core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java 0b464d8 
>   
> 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 
>   core/src/test/java/org/apache/accumulo/core/file/rfile/RFileTest.java 
> 1a83f33 
> 
> Diff: https://reviews.apache.org/r/29176/diff/
> 
> 
> Testing
> ---
> 
> Tested with a few RFiles that I made.
> 
> 
> Thanks,
> 
> Jenna Huston
> 
>

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Jenna Huston]

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

(Updated Jan. 9, 2015, 4:19 p.m.)


Review request for accumulo.


Changes
---

Fixed a few javadoc problems.


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 (updated)
-

  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 f29efcc 
  core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java 0b464d8 
  
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 
  core/src/test/java/org/apache/accumulo/core/file/rfile/RFileTest.java 1a83f33 

Diff: https://reviews.apache.org/r/29176/diff/


Testing
---

Tested with a few RFiles that I made.


Thanks,

Jenna Huston

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[keith]

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

Ship it!


Ship It!

- kturner


On Jan. 8, 2015, 9:24 p.m., Jenna Huston wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29176/
> ---
> 
> (Updated Jan. 8, 2015, 9:24 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 
>   
> 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 
>   core/src/test/java/org/apache/accumulo/core/file/rfile/RFileTest.java 
> 969b179 
> 
> Diff: https://reviews.apache.org/r/29176/diff/
> 
> 
> Testing
> ---
> 
> Tested with a few RFiles that I made.
> 
> 
> Thanks,
> 
> Jenna Huston
> 
>

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Josh Elser]

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

Ship it!


LGTM -- I'll leave some time for other reviewers to add more comments before 
applying though. Thanks, Jenna!

- Josh Elser


On Jan. 8, 2015, 9:24 p.m., Jenna Huston wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29176/
> ---
> 
> (Updated Jan. 8, 2015, 9:24 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 
>   
> 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 
>   core/src/test/java/org/apache/accumulo/core/file/rfile/RFileTest.java 
> 969b179 
> 
> Diff: https://reviews.apache.org/r/29176/diff/
> 
> 
> Testing
> ---
> 
> Tested with a few RFiles that I made.
> 
> 
> Thanks,
> 
> Jenna Huston
> 
>

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Jenna Huston]

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

(Updated Jan. 8, 2015, 9:24 p.m.)


Review request for accumulo.


Changes
---

Incorporated comments from Keith and Christopher


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 (updated)
-

  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 
  core/src/test/java/org/apache/accumulo/core/file/rfile/RFileTest.java 969b179 

Diff: https://reviews.apache.org/r/29176/diff/


Testing
---

Tested with a few RFiles that I made.


Thanks,

Jenna Huston

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Christopher Tubbs]

---
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


Should be generic interface "MetricsGatherer" 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


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


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


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


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


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


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



core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java


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


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
> 
>

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Christopher Tubbs]

> On Dec. 18, 2014, 12:20 p.m., Christopher Tubbs wrote:
> > core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java,
> >  line 34
> > 
> >
> > 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).
> 
> Mike Drob wrote:
> You mean like 
> https://lucene.apache.org/solr/4_10_2/solr-core/org/apache/solr/core/SolrInfoMBean.html?

I don't know what that is for. The updated patch does what I was thinking.


- Christopher


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


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
> 
>

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Mike Drob]

> On Dec. 18, 2014, 5:20 p.m., Christopher Tubbs wrote:
> > core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java,
> >  line 34
> > 
> >
> > 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).

You mean like 
https://lucene.apache.org/solr/4_10_2/solr-core/org/apache/solr/core/SolrInfoMBean.html?


- Mike


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


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

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[keith]

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



core/src/test/java/org/apache/accumulo/core/file/rfile/RFileMetricsTest.java


Is this copied from the RFile unit test?  If can the two test be modified 
to share this code?


- kturner


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

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Jenna Huston]

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

(Updated Dec. 22, 2014, 3:25 p.m.)


Review request for accumulo.


Changes
---

Fixed whitespace issues on test


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 (updated)
-

  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

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Jenna Huston]

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

(Updated Dec. 22, 2014, 3:22 p.m.)


Review request for accumulo.


Changes
---

Added test


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 (updated)
-

  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

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Josh Elser]

> On Dec. 18, 2014, 4:19 p.m., Josh Elser wrote:
> > core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java,
> >  line 36
> > 
> >
> > Why the concurrent maps? I don't see anything in these changes that 
> > would require it now, do you have plans for something else in the future?
> 
> Jenna Huston wrote:
> These keep separate metrics.  One for the density of that metric in the 
> locality group and the other for how many blocks in that locality group this 
> visiblity is seen in.
> 
> Josh Elser wrote:
> I understand why you're using a map, I'm confused about why you need a 
> concurrent datastructure. This appears to be single-threaded.
> 
> Jenna Huston wrote:
> The AtomicLongMap made it easier to increment counters of visibilities.  
> With a regular map you have to delete what is there and then put in a new 
> entry with the incremented count.

Ok, thanks for the explanation. The extra synchronization imposed by the maps 
is likely not of any concern, I was just worried that I was missing something 
else.


- Josh


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


On Dec. 18, 2014, 9:37 p.m., Jenna Huston wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29176/
> ---
> 
> (Updated Dec. 18, 2014, 9:37 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 
>   
> core/src/main/java/org/apache/accumulo/core/file/rfile/VisMetricsGatherer.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29176/diff/
> 
> 
> Testing
> ---
> 
> Tested with a few RFiles that I made.
> 
> 
> Thanks,
> 
> Jenna Huston
> 
>

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Jenna Huston]

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

(Updated Dec. 18, 2014, 9:37 p.m.)


Review request for accumulo.


Changes
---

Still working on a test, but wanted to get these changes out.


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 (updated)
-

  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 

Diff: https://reviews.apache.org/r/29176/diff/


Testing
---

Tested with a few RFiles that I made.


Thanks,

Jenna Huston

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Jenna Huston]

> On Dec. 18, 2014, 4:19 p.m., Josh Elser wrote:
> > core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java,
> >  line 36
> > 
> >
> > Why the concurrent maps? I don't see anything in these changes that 
> > would require it now, do you have plans for something else in the future?
> 
> Jenna Huston wrote:
> These keep separate metrics.  One for the density of that metric in the 
> locality group and the other for how many blocks in that locality group this 
> visiblity is seen in.
> 
> Josh Elser wrote:
> I understand why you're using a map, I'm confused about why you need a 
> concurrent datastructure. This appears to be single-threaded.

The AtomicLongMap made it easier to increment counters of visibilities.  With a 
regular map you have to delete what is there and then put in a new entry with 
the incremented count.


- Jenna


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


On Dec. 17, 2014, 8: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, 8: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
> 
>

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Josh Elser]

> On Dec. 18, 2014, 4:19 p.m., Josh Elser wrote:
> > core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java,
> >  line 36
> > 
> >
> > Why the concurrent maps? I don't see anything in these changes that 
> > would require it now, do you have plans for something else in the future?
> 
> Jenna Huston wrote:
> These keep separate metrics.  One for the density of that metric in the 
> locality group and the other for how many blocks in that locality group this 
> visiblity is seen in.

I understand why you're using a map, I'm confused about why you need a 
concurrent datastructure. This appears to be single-threaded.


- Josh


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


On Dec. 17, 2014, 8: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, 8: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
> 
>

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Jenna Huston]

> On Dec. 18, 2014, 4:19 p.m., Josh Elser wrote:
> > core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java,
> >  line 36
> > 
> >
> > Why the concurrent maps? I don't see anything in these changes that 
> > would require it now, do you have plans for something else in the future?

These keep separate metrics.  One for the density of that metric in the 
locality group and the other for how many blocks in that locality group this 
visiblity is seen in.


- Jenna


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


On Dec. 17, 2014, 8: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, 8: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
> 
>

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Christopher Tubbs]

> On Dec. 18, 2014, 11:19 a.m., Josh Elser wrote:
> > core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java,
> >  line 94
> > 
> >
> > What does hashing the visibility label get you? If you can invoke this 
> > program on some file, your data is already insecure (by virtue of it being 
> > accessible outside of Accumulo).

I think it's useful if you don't care about what the visibilities actually are, 
but just want to know the numbers. It makes for a more sane layout, if the 
visibilities would otherwise vary in length.


- Christopher


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


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
> 
>

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Christopher Tubbs]

---
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


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


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


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


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
> 
>

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[Josh Elser]

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


A unit test would be nice. There's a few dangling whitespace at end of line 
(one in javadoc and a few on empty lines) that would be nice to clean up too.


core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java


RFileVisibilityMetrics might be a better name. This class isn't actually 
doing any gathering, the caller is.



core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java


Why the concurrent maps? I don't see anything in these changes that would 
require it now, do you have plans for something else in the future?



core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java


It would be nicer to use a logger instead of System.out, but I can 
understand the intent of what you did since PrintInfo works that way too. Just 
a comment.



core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java


What does hashing the visibility label get you? If you can invoke this 
program on some file, your data is already insecure (by virtue of it being 
accessible outside of Accumulo).


- Josh Elser


On Dec. 17, 2014, 8: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, 8: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
> 
>

Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from PrintInfo

[keith]

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



core/src/main/java/org/apache/accumulo/core/file/rfile/MetricsGatherer.java


Is there an assumption that the order of elements in the list returned in 
this method will end up corresponding to the order of groups in the other list? 
 If so that could easily break.  

Could add a unit test that checks this assumption.  

Alternatively could use locality group names to join these two data sets.  
Have  `getLocalityGroupCF()` return `Map>` where 
the map key is LG name.  The method `newLocalityGroup()` could take a name and 
keep a mapping of names to ints.



core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java


RFile has extensive test in the unit test RFileTest.  These test create 
RFiles in memory.  Would be nice to add test to verify metrics work as expected.



core/src/main/java/org/apache/accumulo/core/file/rfile/RFile.java


AFAICT there is no printing happening here?  Should this method be called 
`setMetricsGatherer()`?


- kturner


On Dec. 17, 2014, 8: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, 8: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
> 
>