----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2746/#review3082 -----------------------------------------------------------
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. http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleCheckpointVertex.java <https://reviews.apache.org/r/2746/#comment6885> This doesn't need to be static anymore. http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleCheckpointVertex.java <https://reviews.apache.org/r/2746/#comment6870> Indenting should be 8 spaces. http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleCheckpointVertex.java <https://reviews.apache.org/r/2746/#comment6873> extra line. http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleCheckpointVertex.java <https://reviews.apache.org/r/2746/#comment6874> extra line. http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleCheckpointVertex.java <https://reviews.apache.org/r/2746/#comment6875> 4 spaces. http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleCheckpointVertex.java <https://reviews.apache.org/r/2746/#comment6871> 4 spaces indenting. http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleCheckpointVertex.java <https://reviews.apache.org/r/2746/#comment6872> 4 spaces indenting. http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleCheckpointVertex.java <https://reviews.apache.org/r/2746/#comment6876> Align to GiraphJob.WORKER_CONTEXT_CLASS http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleMutateGraphVertex.java <https://reviews.apache.org/r/2746/#comment6877> VERTEX_COUNT shouldn't be capitalized. All caps should be reserved for only static values. http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleMutateGraphVertex.java <https://reviews.apache.org/r/2746/#comment6878> EDGE_COUNT shouldn't be capitalized. All caps should be reserved for only static values. http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimplePageRankVertex.java <https://reviews.apache.org/r/2746/#comment6887> These no longer need to be static anymore, could be private variables that have public accessor method. http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/examples/SimpleVertexWithWorkerContext.java <https://reviews.apache.org/r/2746/#comment6879> Might want to add a comment about this example. I.e. /** * Fully runnable example of how to * emit worker data to HDFS during a graph * computation. */ http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java <https://reviews.apache.org/r/2746/#comment6880> extra line. http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspService.java <https://reviews.apache.org/r/2746/#comment6881> Awesome, I hated this. http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspServiceWorker.java <https://reviews.apache.org/r/2746/#comment6882> indenting. http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/DefaultWorkerContext.java <https://reviews.apache.org/r/2746/#comment6883> extra line. http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/WorkerContext.java <https://reviews.apache.org/r/2746/#comment6884> Other javadoc has lines in between comment and params (i.e. * superstep starts. * * @throws IllegalAccessException - Avery 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 > >
