> 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.
> 
> Claudio Martella wrote:
>     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?

Mine is all messed up too.  I have to manually fix some things.


> 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.
> 
> Claudio Martella wrote:
>     Can't make it non static. Won't be able to read from tests.

Sorry I wasn't more clear, I was suggesting that we fix this.  But it's not 
really related to this issue.  So don't worry about it.  Please ignore my 
comment.


> 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.
> 
> Claudio Martella wrote:
>     Not sure we can do this. How will tests get to their values. Can't access 
> those members if not static.

I was suggesting that we fix this, maybe give the user the worker context at 
the end.  Actually not sure it's the right solution and it's not really related 
to this issue.  So don't worry about it.  Please ignore my comment.


- Avery


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