incubator-giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Claudio Martella" <claudio.marte...@gmail.com>
Subject Re: Review Request: GIRAPH-47 Export Worker's Context/State to vertices through pre/post/Application/Superstep
Date Mon, 07 Nov 2011 20:10:59 GMT


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


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