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