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: aggregator at input super step
Date Tue, 04 Jun 2013 05:08:19 GMT

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


Getting better...Please take a look at the grammar and make sure everything looks polished.



giraph-core/src/main/java/org/apache/giraph/io/EdgeReader.java
<https://reviews.apache.org/r/11525/#comment44164>

    This comment is a bit messy.  Perhaps something like
    
    "Sets the aggreagator usage.  This method is only for use by the infrastructure."
    
    By the way, should we call it setWorkerAggregatorUse instead?



giraph-core/src/main/java/org/apache/giraph/io/VertexReader.java
<https://reviews.apache.org/r/11525/#comment44165>

    extra space.
    
    "for  vertex" -> "for vertex"



giraph-core/src/main/java/org/apache/giraph/io/VertexReader.java
<https://reviews.apache.org/r/11525/#comment44166>

    Please consider a cleaner comment (i.e. the same suggestion above).



giraph-core/src/main/java/org/apache/giraph/io/internal/WrappedEdgeReader.java
<https://reviews.apache.org/r/11525/#comment44167>

    // set -> // Set



giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java
<https://reviews.apache.org/r/11525/#comment44337>

    Please reword this comment.



giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java
<https://reviews.apache.org/r/11525/#comment44331>

    If you don't think it's necessary, feel free to take it out.



giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java
<https://reviews.apache.org/r/11525/#comment44338>

    bfore -> before



giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java
<https://reviews.apache.org/r/11525/#comment44332>

    Maybe change to something like "This method is called only after masterCompute is initialized."



giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java
<https://reviews.apache.org/r/11525/#comment44333>

    Should this method be moved closer to where its called? - appears only once I think.



giraph-core/src/main/java/org/apache/giraph/worker/BspServiceWorker.java
<https://reviews.apache.org/r/11525/#comment44334>

    Given this is a one line method called once, does it make more sense to inline it?



giraph-core/src/main/java/org/apache/giraph/worker/VertexInputSplitsCallable.java
<https://reviews.apache.org/r/11525/#comment44335>

    // Similar...



giraph-core/src/main/java/org/apache/giraph/worker/VertexInputSplitsCallable.java
<https://reviews.apache.org/r/11525/#comment44336>

    // Set...


- Avery Ching


On May 30, 2013, 4:46 p.m., Bingjing Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11525/
> -----------------------------------------------------------
> 
> (Updated May 30, 2013, 4:46 p.m.)
> 
> 
> Review request for giraph.
> 
> 
> Description
> -------
> 
> add aggregator for vertex reader and edge reader at input super step.
> 
> Changes in code:
> add "aggregate" interface to VertexReader and EdgeReader
> initialize "aggregator" in "readInputSplit" in "VertexInputSplitsCallable" and "EdgeInputSplitsCallable"
> initialize master aggregator before data loading in "coordinateSuperStep" in "BspServiceMaster"
> seperate master aggregator initialization and its computation for Super Step 0 (originally
they are mixed in one method "runMasterCompute")
> perpare aggregator in "setup" in "BspServiceWorker" before data loading
> 
> Test:
> Add SimpleVertexReader and  SimpleEdgeReader to AggregatorsTestComputation for genreating
input data.
> Also add "aggregate" call in "getCurrentVertex" and "getCurrentEdge".
> Register persistent aggregator with name "INPUT_VERTEX_PERSISTENT_AGG" and "INPUT_EDGE_PERSISTENT_AGG"in
AggregatorsTestMasterCompute.initialize
> Assert if aggregated values can be retrived in AggregatorsTestMasterCompute.compute when
"superstep >= 0"
> 
> 
> This addresses bug GIRAPH-673.
>     https://issues.apache.org/jira/browse/GIRAPH-673
> 
> 
> Diffs
> -----
> 
>   giraph-core/src/main/java/org/apache/giraph/io/EdgeReader.java 363a5e6 
>   giraph-core/src/main/java/org/apache/giraph/io/VertexReader.java 9695169 
>   giraph-core/src/main/java/org/apache/giraph/io/internal/WrappedEdgeReader.java aae7a72

>   giraph-core/src/main/java/org/apache/giraph/io/internal/WrappedVertexReader.java 54adfec

>   giraph-core/src/main/java/org/apache/giraph/master/BspServiceMaster.java bd48116 
>   giraph-core/src/main/java/org/apache/giraph/worker/BspServiceWorker.java 8b5e39a 
>   giraph-core/src/main/java/org/apache/giraph/worker/EdgeInputSplitsCallable.java 78cdd8e

>   giraph-core/src/main/java/org/apache/giraph/worker/VertexInputSplitsCallable.java 977e100

>   giraph-examples/src/main/java/org/apache/giraph/examples/AggregatorsTestComputation.java
db527f2 
>   giraph-examples/src/test/java/org/apache/giraph/aggregators/TestAggregatorsHandling.java
6d22800 
> 
> Diff: https://reviews.apache.org/r/11525/diff/
> 
> 
> Testing
> -------
> 
> Unit Test of persistent aggregator at input super step in modified "AggregatorsTestComputation"
is done.
> "mvn clean verify" was done on giraph-core
> "mvn clean verfiy" was done on giraph-example
> 
> rechecked "indent" problem. Hopefully they are all solved
> 
> 
> Thanks,
> 
> Bingjing Zhang
> 
>


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