incubator-giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Avery Ching" <avery.ch...@gmail.com>
Subject Re: Review Request: HBase/Accumulo Input and Output formats (on behalf of Brian)
Date Thu, 19 Apr 2012 07:43:22 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message