You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Jenna Huston <je...@gmail.com> on 2014/12/17 21:31:04 UTC
Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from
PrintInfo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29176/
-----------------------------------------------------------
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
Posted by Mike Drob <md...@mdrob.com>.
> 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
> > <https://reviews.apache.org/r/29176/diff/1/?file=795044#file795044line34>
> >
> > 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
Posted by Christopher Tubbs <ct...@apache.org>.
> 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
> > <https://reviews.apache.org/r/29176/diff/1/?file=795044#file795044line34>
> >
> > 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
Posted by Christopher Tubbs <ct...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from
PrintInfo
Posted by ke...@deenlo.com.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/29176/#comment108703>
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<String,ArrayList<ByteSequence>>` 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
<https://reviews.apache.org/r/29176/#comment108705>
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
<https://reviews.apache.org/r/29176/#comment108704>
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
>
>
Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from
PrintInfo
Posted by Christopher Tubbs <ct...@apache.org>.
-----------------------------------------------------------
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
Posted by Jenna Huston <je...@gmail.com>.
-----------------------------------------------------------
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
Posted by Christopher Tubbs <ct...@apache.org>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/29176/#comment111532>
Should have some javadoc explaining the kinds of metrics this class provides.
In other words, what can we expect to be contained in `Map<String,ArrayList<String>>` when we call `getMetrics()`?
core/src/main/java/org/apache/accumulo/core/file/rfile/VisMetricsGatherer.java
<https://reviews.apache.org/r/29176/#comment111534>
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
Posted by Jenna Huston <je...@gmail.com>.
-----------------------------------------------------------
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
Posted by ke...@deenlo.com.
-----------------------------------------------------------
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
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
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
Posted by Jenna Huston <je...@gmail.com>.
-----------------------------------------------------------
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
Posted by ke...@deenlo.com.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/29176/#comment110579>
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
Posted by Christopher Tubbs <ct...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 29176: ACCUMULO-3420 Get Visibility Metrics from
PrintInfo
Posted by Jenna Huston <je...@gmail.com>.
-----------------------------------------------------------
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
Posted by Jenna Huston <je...@gmail.com>.
-----------------------------------------------------------
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
Posted by Jenna Huston <je...@gmail.com>.
-----------------------------------------------------------
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
Posted by Jenna Huston <je...@gmail.com>.
> 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
> > <https://reviews.apache.org/r/29176/diff/1/?file=795044#file795044line36>
> >
> > 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
Posted by Josh Elser <jo...@gmail.com>.
> 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
> > <https://reviews.apache.org/r/29176/diff/1/?file=795044#file795044line36>
> >
> > 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
Posted by Christopher Tubbs <ct...@apache.org>.
> 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
> > <https://reviews.apache.org/r/29176/diff/1/?file=795044#file795044line94>
> >
> > 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
Posted by Jenna Huston <je...@gmail.com>.
> 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
> > <https://reviews.apache.org/r/29176/diff/1/?file=795044#file795044line36>
> >
> > 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
Posted by Josh Elser <jo...@gmail.com>.
> 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
> > <https://reviews.apache.org/r/29176/diff/1/?file=795044#file795044line36>
> >
> > 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
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/29176/#comment108706>
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
<https://reviews.apache.org/r/29176/#comment108707>
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
<https://reviews.apache.org/r/29176/#comment108708>
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
<https://reviews.apache.org/r/29176/#comment108709>
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
>
>