> On 2011-11-07 19:12:55, Avery Ching wrote:
> > Claudio, really nice stuff here.  Most of my comments are related to 
> > indenting.  But otherwise, this is a lot better IMO.  Please take a look at 
> > CODE_CONVENTIONS and fix accordingly.  While the official policy is 2 
> > space, at this time, for the 4 space indented files, please keep to 4 
> > spaces for consistency.  We will transition everything over at some point.  
> > New files can be 2 space (new convention) if desired.

Ok, still have to understand a bit the code conventions. Trying to stick to 
them. Maybe an Eclipse format conf file would help? Could you share yours, if 
you have one?


> On 2011-11-07 19:12:55, Avery Ching wrote:
> > http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimplePageRankVertex.java,
> >  lines 91-92
> > <https://reviews.apache.org/r/2746/diff/1/?file=56634#file56634line91>
> >
> >     These no longer need to be static anymore, could be private variables 
> > that have public accessor method.

Not sure we can do this. How will tests get to their values. Can't access those 
members if not static.


> On 2011-11-07 19:12:55, Avery Ching wrote:
> > http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleCheckpointVertex.java,
> >  lines 250-251
> > <https://reviews.apache.org/r/2746/diff/1/?file=56632#file56632line250>
> >
> >     Align to GiraphJob.WORKER_CONTEXT_CLASS

What do you mean? I aligned to the example, all classes are set with 
.setClass() there. Fixing the whole thing.


> On 2011-11-07 19:12:55, Avery Ching wrote:
> > http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleCheckpointVertex.java,
> >  line 150
> > <https://reviews.apache.org/r/2746/diff/1/?file=56632#file56632line150>
> >
> >     This doesn't need to be static anymore.

Can't make it non static. Won't be able to read from tests.


- Claudio


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2746/#review3082
-----------------------------------------------------------


On 2011-11-07 19:09:08, Avery Ching wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2746/
> -----------------------------------------------------------
> 
> (Updated 2011-11-07 19:09:08)
> 
> 
> Review request for giraph.
> 
> 
> Summary
> -------
> 
> Claudio's patch for GIRAPH-47.
> 
> 
> This addresses bug GIRAPH-47.
>     https://issues.apache.org/jira/browse/GIRAPH-47
> 
> 
> Diffs
> -----
> 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/benchmark/PageRankBenchmark.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/benchmark/RandomMessageBenchmark.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/bsp/CentralizedService.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/bsp/CentralizedServiceWorker.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleCheckpointVertex.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleMutateGraphVertex.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimplePageRankVertex.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleVertexWithWorkerContext.java
>  PRE-CREATION 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspService.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspServiceWorker.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspUtils.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/DefaultWorkerContext.java
>  PRE-CREATION 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/GiraphJob.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/Vertex.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/WorkerContext.java
>  PRE-CREATION 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/test/java/org/apache/giraph/TestBspBasic.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/test/java/org/apache/giraph/TestManualCheckpoint.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/test/java/org/apache/giraph/TestMutateGraphVertex.java
>  1198865 
>   
> http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/test/java/org/apache/giraph/TestVertexRangeBalancer.java
>  1198865 
> 
> Diff: https://reviews.apache.org/r/2746/diff
> 
> 
> Testing
> -------
> 
> mvn install
> 
> 
> Thanks,
> 
> Avery
> 
>

Reply via email to