----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4801/#review7020 -----------------------------------------------------------
Brian, this kind of stuff will really help lower the bar for using Giraph. Great work! Probably my main comment is that we should get this code to use the same checkstyle.xml as the parent pom.xml. Making that change will likely take you a little bit of time. Once that gets worked out, we should get this committed. I definitely intend to use your same approach to adding the Hive formats as well. In response to your comments. 1) Users must 'mvn install' the giraph artifact in their local repo, at least until we get something posted on maven central. Agreed. This isn't too bad, but as you mentioned, a README would be useful. Or you could edit https://cwiki.apache.org/confluence/display/GIRAPH/, which would be possibly easier than maintaining documentation here. 2) I modified the pom.xml to exclude the artifact from the rat plugin. I realize this is less than desirable, but I couldn't get anything running despite numerous attempts at fixing the "too many unapproved licenses" issues. I'm interested to hear your guys thoughts. I'm confused why there were too many unapproved licenses. This is probably needed though, this the code has to have the Apache license I believe. 3) Duplicate BspCase in my submodule, at least until Giraph has a test artifact. Yeah, this sucks, but at least you noted it in the comment. 4) Initializing the AccumuloVertexInputFormat has some procedural limitations inherent in the format design when run with the GiraphJob. It really expects to have control of the Job instance. These can be difficult to track down. I tried to document these in my unit tests and provide some simple error wrapping to help notify users when they see these. Not an Accumulo expert, so.....okay. =) 5) No README.txt or any wiki entry yet. I figured I'd wait and see what feedback you guys had. See my response to 1). http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/pom.xml <https://reviews.apache.org/r/4801/#comment15593> Brian, I've noticed that there is no usage of the checkstyle plugin, like the parent pom.xml uses (i.e. <plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-checkstyle-plugin</artifactId> <version>2.9</version> <configuration> <configLocation>checkstyle.xml</configLocation> <enableRulesSummary>false</enableRulesSummary> <headerLocation>license-header.txt</headerLocation> <failOnError>true</failOnError> <includeTestSourceDirectory>false</includeTestSourceDirectory> </configuration> <executions> <execution> <phase>verify</phase> <goals> <goal>check</goal> </goals> </execution> </executions> </plugin> This would be good to have code uniformity. http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/main/java/org/apache/giraph/format/accumulo/AccumuloVertexInputFormat.java <https://reviews.apache.org/r/4801/#comment15594> I'm specifying a few examples where the checkstyle.xml will catch you. Here you would need to add javadoc for your members. http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/main/java/org/apache/giraph/format/accumulo/AccumuloVertexInputFormat.java <https://reviews.apache.org/r/4801/#comment15595> @Override http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/main/java/org/apache/giraph/format/accumulo/AccumuloVertexInputFormat.java <https://reviews.apache.org/r/4801/#comment15596> Extra * http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/main/java/org/apache/giraph/format/accumulo/AccumuloVertexInputFormat.java <https://reviews.apache.org/r/4801/#comment15597> javadoc http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/main/java/org/apache/giraph/format/accumulo/AccumuloVertexInputFormat.java <https://reviews.apache.org/r/4801/#comment15598> @Override http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/main/java/org/apache/giraph/format/accumulo/AccumuloVertexInputFormat.java <https://reviews.apache.org/r/4801/#comment15599> if (e http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/test/java/org/apache/giraph/format/accumulo/TestAccumuloVertexFormat.java <https://reviews.apache.org/r/4801/#comment15600> extra lines http://svn.apache.org/repos/asf/incubator/giraph/trunk/pom.xml <https://reviews.apache.org/r/4801/#comment15592> Would be great to get Apache Rat working with giraph-formats-contrib so that we don't forget any licenses here. - Avery On 2012-04-19 07:27:14, Avery Ching wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4801/ > ----------------------------------------------------------- > > (Updated 2012-04-19 07:27:14) > > > Review request for giraph. > > > Summary > ------- > > Brian's patch for GIRAPH-153. > > > This addresses bug GIRAPH-153. > https://issues.apache.org/jira/browse/GIRAPH-153 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/LICENSE.txt > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/license-header.txt > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/pom.xml > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/main/assembly/compile.xml > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/main/java/org/apache/giraph/format/accumulo/AccumuloVertexInputFormat.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/main/java/org/apache/giraph/format/accumulo/AccumuloVertexOutputFormat.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/main/java/org/apache/giraph/format/hbase/HBaseVertexInputFormat.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/main/java/org/apache/giraph/format/hbase/HBaseVertexOutputFormat.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/test/java/org/apache/giraph/BspCase.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/test/java/org/apache/giraph/format/accumulo/TestAccumuloVertexFormat.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/test/java/org/apache/giraph/format/accumulo/edgemarker/AccumuloEdgeInputFormat.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/test/java/org/apache/giraph/format/accumulo/edgemarker/AccumuloEdgeOutputFormat.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/test/java/org/apache/giraph/format/hbase/TestHBaseRootMarkerVertextFormat.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/test/java/org/apache/giraph/format/hbase/edgemarker/TableEdgeInputFormat.java > PRE-CREATION > > http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/test/java/org/apache/giraph/format/hbase/edgemarker/TableEdgeOutputFormat.java > PRE-CREATION > http://svn.apache.org/repos/asf/incubator/giraph/trunk/pom.xml 1327637 > > Diff: https://reviews.apache.org/r/4801/diff > > > Testing > ------- > > > Thanks, > > Avery > >
