You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@giraph.apache.org by Avery Ching <av...@gmail.com> on 2012/04/19 09:27:14 UTC
Review Request: HBase/Accumulo Input and Output formats (on behalf of Brian)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4801/
-----------------------------------------------------------
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
Re: Review Request: HBase/Accumulo Input and Output formats (on behalf of
Brian)
Posted by Avery Ching <av...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request: HBase/Accumulo Input and Output formats (on behalf of
Brian)
Posted by Avery Ching <av...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4801/#review7404
-----------------------------------------------------------
Hi Brian, the patch applies nicely, but it filled with duplicates. Also there are some javadoc indentation fixes to make. I just gave a couple of examples. Can you please fix this and resubmit? Thanks!
http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/LICENSE.txt
<https://reviews.apache.org/r/4801/#comment16328>
This license is duplicated several times.
http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/license-header.txt
<https://reviews.apache.org/r/4801/#comment16329>
This license is duplicated several times.
http://svn.apache.org/repos/asf/incubator/giraph/trunk/giraph-formats-contrib/src/main/assembly/compile.xml
<https://reviews.apache.org/r/4801/#comment16330>
More duplication.
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/#comment16331>
Please fix indentation.
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/#comment16332>
Please fix indentation.
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/#comment16333>
Please fix indentation.
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/#comment16334>
Please fix indentation.
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/#comment16335>
Code duplication.
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/#comment16336>
Extra *
- Avery
On 2012-04-30 23:54:55, Avery Ching wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4801/
> -----------------------------------------------------------
>
> (Updated 2012-04-30 23:54:55)
>
>
> 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/accumulo/package-info.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/main/java/org/apache/giraph/format/hbase/package-info.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
>
> Diff: https://reviews.apache.org/r/4801/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Avery
>
>
Re: Review Request: HBase/Accumulo Input and Output formats (on behalf of
Brian)
Posted by Avery Ching <av...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4801/
-----------------------------------------------------------
(Updated 2012-04-30 23:54:55.758151)
Review request for giraph.
Changes
-------
Update of Brian's 153.2.
Summary
-------
Brian's patch for GIRAPH-153.
This addresses bug GIRAPH-153.
https://issues.apache.org/jira/browse/GIRAPH-153
Diffs (updated)
-----
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/accumulo/package-info.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/main/java/org/apache/giraph/format/hbase/package-info.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
Diff: https://reviews.apache.org/r/4801/diff
Testing
-------
Thanks,
Avery