You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@giraph.apache.org by Maja Kabiljo <ma...@fb.com> on 2013/02/08 02:57:29 UTC

Review Request: GIRAPH-504: Create PartitionContext

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

Review request for giraph.


Description
-------

Create PartitionContext as a thread-safe place in which to keep data vertices share, and do global computation.


This addresses bug GIRAPH-504.
    https://issues.apache.org/jira/browse/GIRAPH-504


Diffs
-----

  giraph-core/src/main/java/org/apache/giraph/conf/GiraphClasses.java d2641f1 
  giraph-core/src/main/java/org/apache/giraph/conf/GiraphConfiguration.java 9ca1e7e 
  giraph-core/src/main/java/org/apache/giraph/conf/GiraphConstants.java fb4e8a3 
  giraph-core/src/main/java/org/apache/giraph/conf/ImmutableClassesGiraphConfiguration.java 30a7da7 
  giraph-core/src/main/java/org/apache/giraph/examples/PartitionContextTestVertex.java PRE-CREATION 
  giraph-core/src/main/java/org/apache/giraph/graph/ComputeCallable.java 94ed6d9 
  giraph-core/src/main/java/org/apache/giraph/graph/GraphState.java 9cdec7c 
  giraph-core/src/main/java/org/apache/giraph/partition/BasicPartition.java PRE-CREATION 
  giraph-core/src/main/java/org/apache/giraph/partition/ByteArrayPartition.java d34af11 
  giraph-core/src/main/java/org/apache/giraph/partition/DefaultPartitionContext.java PRE-CREATION 
  giraph-core/src/main/java/org/apache/giraph/partition/Partition.java 55ce8c0 
  giraph-core/src/main/java/org/apache/giraph/partition/PartitionContext.java PRE-CREATION 
  giraph-core/src/main/java/org/apache/giraph/partition/SimplePartition.java 479011f 
  giraph-core/src/main/java/org/apache/giraph/vertex/Vertex.java 974232e 
  giraph-core/src/test/java/org/apache/giraph/BspCase.java 6aab533 
  giraph-core/src/test/java/org/apache/giraph/TestPartitionContext.java PRE-CREATION 

Diff: https://reviews.apache.org/r/9372/diff/


Testing
-------

Added test for PartitionContext
mvn clean verify


Thanks,

Maja Kabiljo

Re: Review Request: GIRAPH-504: Create PartitionContext

Posted by Maja Kabiljo <ma...@fb.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9372/
-----------------------------------------------------------

(Updated Feb. 8, 2013, 5:30 p.m.)


Review request for giraph.


Description
-------

Create PartitionContext as a thread-safe place in which to keep data vertices share, and do global computation.


This addresses bug GIRAPH-504.
    https://issues.apache.org/jira/browse/GIRAPH-504


Diffs (updated)
-----

  giraph-core/src/main/java/org/apache/giraph/conf/GiraphClasses.java d2641f1 
  giraph-core/src/main/java/org/apache/giraph/conf/GiraphConfiguration.java 7e48103 
  giraph-core/src/main/java/org/apache/giraph/conf/GiraphConstants.java fb4e8a3 
  giraph-core/src/main/java/org/apache/giraph/conf/ImmutableClassesGiraphConfiguration.java 30a7da7 
  giraph-core/src/main/java/org/apache/giraph/examples/PartitionContextTestVertex.java PRE-CREATION 
  giraph-core/src/main/java/org/apache/giraph/graph/ComputeCallable.java 94ed6d9 
  giraph-core/src/main/java/org/apache/giraph/graph/GraphState.java 9cdec7c 
  giraph-core/src/main/java/org/apache/giraph/partition/BasicPartition.java PRE-CREATION 
  giraph-core/src/main/java/org/apache/giraph/partition/ByteArrayPartition.java d34af11 
  giraph-core/src/main/java/org/apache/giraph/partition/DefaultPartitionContext.java PRE-CREATION 
  giraph-core/src/main/java/org/apache/giraph/partition/Partition.java 55ce8c0 
  giraph-core/src/main/java/org/apache/giraph/partition/PartitionContext.java PRE-CREATION 
  giraph-core/src/main/java/org/apache/giraph/partition/SimplePartition.java 479011f 
  giraph-core/src/main/java/org/apache/giraph/vertex/Vertex.java 974232e 
  giraph-core/src/test/java/org/apache/giraph/BspCase.java 6aab533 
  giraph-core/src/test/java/org/apache/giraph/TestPartitionContext.java PRE-CREATION 

Diff: https://reviews.apache.org/r/9372/diff/


Testing
-------

Added test for PartitionContext
mvn clean verify


Thanks,

Maja Kabiljo

Re: Review Request: GIRAPH-504: Create PartitionContext

Posted by Maja Kabiljo <ma...@fb.com>.

> On Feb. 8, 2013, 10:58 a.m., Claudio Martella wrote:
> > Overall looks good to me. Somebody might argue that Partition should stay an interface though.

We still have Partition as an interface, I just added BasicPartition to remove duplication from current two implementations.


- Maja


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


On Feb. 8, 2013, 5:30 p.m., Maja Kabiljo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9372/
> -----------------------------------------------------------
> 
> (Updated Feb. 8, 2013, 5:30 p.m.)
> 
> 
> Review request for giraph.
> 
> 
> Description
> -------
> 
> Create PartitionContext as a thread-safe place in which to keep data vertices share, and do global computation.
> 
> 
> This addresses bug GIRAPH-504.
>     https://issues.apache.org/jira/browse/GIRAPH-504
> 
> 
> Diffs
> -----
> 
>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphClasses.java d2641f1 
>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphConfiguration.java 7e48103 
>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphConstants.java fb4e8a3 
>   giraph-core/src/main/java/org/apache/giraph/conf/ImmutableClassesGiraphConfiguration.java 30a7da7 
>   giraph-core/src/main/java/org/apache/giraph/examples/PartitionContextTestVertex.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/graph/ComputeCallable.java 94ed6d9 
>   giraph-core/src/main/java/org/apache/giraph/graph/GraphState.java 9cdec7c 
>   giraph-core/src/main/java/org/apache/giraph/partition/BasicPartition.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/partition/ByteArrayPartition.java d34af11 
>   giraph-core/src/main/java/org/apache/giraph/partition/DefaultPartitionContext.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/partition/Partition.java 55ce8c0 
>   giraph-core/src/main/java/org/apache/giraph/partition/PartitionContext.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/partition/SimplePartition.java 479011f 
>   giraph-core/src/main/java/org/apache/giraph/vertex/Vertex.java 974232e 
>   giraph-core/src/test/java/org/apache/giraph/BspCase.java 6aab533 
>   giraph-core/src/test/java/org/apache/giraph/TestPartitionContext.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/9372/diff/
> 
> 
> Testing
> -------
> 
> Added test for PartitionContext
> mvn clean verify
> 
> 
> Thanks,
> 
> Maja Kabiljo
> 
>

Re: Review Request: GIRAPH-504: Create PartitionContext

Posted by Claudio Martella <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9372/#review16349
-----------------------------------------------------------

Ship it!


Overall looks good to me. Somebody might argue that Partition should stay an interface though.

- Claudio Martella


On Feb. 8, 2013, 1:57 a.m., Maja Kabiljo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9372/
> -----------------------------------------------------------
> 
> (Updated Feb. 8, 2013, 1:57 a.m.)
> 
> 
> Review request for giraph.
> 
> 
> Description
> -------
> 
> Create PartitionContext as a thread-safe place in which to keep data vertices share, and do global computation.
> 
> 
> This addresses bug GIRAPH-504.
>     https://issues.apache.org/jira/browse/GIRAPH-504
> 
> 
> Diffs
> -----
> 
>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphClasses.java d2641f1 
>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphConfiguration.java 9ca1e7e 
>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphConstants.java fb4e8a3 
>   giraph-core/src/main/java/org/apache/giraph/conf/ImmutableClassesGiraphConfiguration.java 30a7da7 
>   giraph-core/src/main/java/org/apache/giraph/examples/PartitionContextTestVertex.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/graph/ComputeCallable.java 94ed6d9 
>   giraph-core/src/main/java/org/apache/giraph/graph/GraphState.java 9cdec7c 
>   giraph-core/src/main/java/org/apache/giraph/partition/BasicPartition.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/partition/ByteArrayPartition.java d34af11 
>   giraph-core/src/main/java/org/apache/giraph/partition/DefaultPartitionContext.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/partition/Partition.java 55ce8c0 
>   giraph-core/src/main/java/org/apache/giraph/partition/PartitionContext.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/partition/SimplePartition.java 479011f 
>   giraph-core/src/main/java/org/apache/giraph/vertex/Vertex.java 974232e 
>   giraph-core/src/test/java/org/apache/giraph/BspCase.java 6aab533 
>   giraph-core/src/test/java/org/apache/giraph/TestPartitionContext.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/9372/diff/
> 
> 
> Testing
> -------
> 
> Added test for PartitionContext
> mvn clean verify
> 
> 
> Thanks,
> 
> Maja Kabiljo
> 
>

Re: Review Request: GIRAPH-504: Create PartitionContext

Posted by Maja Kabiljo <ma...@fb.com>.

> On Feb. 8, 2013, 10:57 a.m., Claudio Martella wrote:
> > giraph-core/src/main/java/org/apache/giraph/graph/ComputeCallable.java, line 209
> > <https://reviews.apache.org/r/9372/diff/1/?file=256870#file256870line209>
> >
> >     not sure why you set it again here.

How do you mean again? Vertex accesses it through GraphState, and this it the only place where it's set.


- Maja


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


On Feb. 8, 2013, 5:30 p.m., Maja Kabiljo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9372/
> -----------------------------------------------------------
> 
> (Updated Feb. 8, 2013, 5:30 p.m.)
> 
> 
> Review request for giraph.
> 
> 
> Description
> -------
> 
> Create PartitionContext as a thread-safe place in which to keep data vertices share, and do global computation.
> 
> 
> This addresses bug GIRAPH-504.
>     https://issues.apache.org/jira/browse/GIRAPH-504
> 
> 
> Diffs
> -----
> 
>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphClasses.java d2641f1 
>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphConfiguration.java 7e48103 
>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphConstants.java fb4e8a3 
>   giraph-core/src/main/java/org/apache/giraph/conf/ImmutableClassesGiraphConfiguration.java 30a7da7 
>   giraph-core/src/main/java/org/apache/giraph/examples/PartitionContextTestVertex.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/graph/ComputeCallable.java 94ed6d9 
>   giraph-core/src/main/java/org/apache/giraph/graph/GraphState.java 9cdec7c 
>   giraph-core/src/main/java/org/apache/giraph/partition/BasicPartition.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/partition/ByteArrayPartition.java d34af11 
>   giraph-core/src/main/java/org/apache/giraph/partition/DefaultPartitionContext.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/partition/Partition.java 55ce8c0 
>   giraph-core/src/main/java/org/apache/giraph/partition/PartitionContext.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/partition/SimplePartition.java 479011f 
>   giraph-core/src/main/java/org/apache/giraph/vertex/Vertex.java 974232e 
>   giraph-core/src/test/java/org/apache/giraph/BspCase.java 6aab533 
>   giraph-core/src/test/java/org/apache/giraph/TestPartitionContext.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/9372/diff/
> 
> 
> Testing
> -------
> 
> Added test for PartitionContext
> mvn clean verify
> 
> 
> Thanks,
> 
> Maja Kabiljo
> 
>

Re: Review Request: GIRAPH-504: Create PartitionContext

Posted by Claudio Martella <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9372/#review16347
-----------------------------------------------------------



giraph-core/src/main/java/org/apache/giraph/graph/ComputeCallable.java
<https://reviews.apache.org/r/9372/#comment34810>

    not sure why you set it again here.


- Claudio Martella


On Feb. 8, 2013, 1:57 a.m., Maja Kabiljo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9372/
> -----------------------------------------------------------
> 
> (Updated Feb. 8, 2013, 1:57 a.m.)
> 
> 
> Review request for giraph.
> 
> 
> Description
> -------
> 
> Create PartitionContext as a thread-safe place in which to keep data vertices share, and do global computation.
> 
> 
> This addresses bug GIRAPH-504.
>     https://issues.apache.org/jira/browse/GIRAPH-504
> 
> 
> Diffs
> -----
> 
>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphClasses.java d2641f1 
>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphConfiguration.java 9ca1e7e 
>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphConstants.java fb4e8a3 
>   giraph-core/src/main/java/org/apache/giraph/conf/ImmutableClassesGiraphConfiguration.java 30a7da7 
>   giraph-core/src/main/java/org/apache/giraph/examples/PartitionContextTestVertex.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/graph/ComputeCallable.java 94ed6d9 
>   giraph-core/src/main/java/org/apache/giraph/graph/GraphState.java 9cdec7c 
>   giraph-core/src/main/java/org/apache/giraph/partition/BasicPartition.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/partition/ByteArrayPartition.java d34af11 
>   giraph-core/src/main/java/org/apache/giraph/partition/DefaultPartitionContext.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/partition/Partition.java 55ce8c0 
>   giraph-core/src/main/java/org/apache/giraph/partition/PartitionContext.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/partition/SimplePartition.java 479011f 
>   giraph-core/src/main/java/org/apache/giraph/vertex/Vertex.java 974232e 
>   giraph-core/src/test/java/org/apache/giraph/BspCase.java 6aab533 
>   giraph-core/src/test/java/org/apache/giraph/TestPartitionContext.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/9372/diff/
> 
> 
> Testing
> -------
> 
> Added test for PartitionContext
> mvn clean verify
> 
> 
> Thanks,
> 
> Maja Kabiljo
> 
>

Re: Review Request: GIRAPH-504: Create PartitionContext

Posted by Nitay Joffe <ni...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9372/#review16355
-----------------------------------------------------------


+1


giraph-core/src/main/java/org/apache/giraph/conf/ImmutableClassesGiraphConfiguration.java
<https://reviews.apache.org/r/9372/#comment34818>

    nit: one line return ... ?



giraph-core/src/main/java/org/apache/giraph/partition/ByteArrayPartition.java
<https://reviews.apache.org/r/9372/#comment34820>

    we have this in a bunch of places, might as well just make a helper method progress() in BasicPartition?



giraph-core/src/main/java/org/apache/giraph/partition/PartitionContext.java
<https://reviews.apache.org/r/9372/#comment34819>

    fix comment


- Nitay Joffe


On Feb. 8, 2013, 1:57 a.m., Maja Kabiljo wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9372/
> -----------------------------------------------------------
> 
> (Updated Feb. 8, 2013, 1:57 a.m.)
> 
> 
> Review request for giraph.
> 
> 
> Description
> -------
> 
> Create PartitionContext as a thread-safe place in which to keep data vertices share, and do global computation.
> 
> 
> This addresses bug GIRAPH-504.
>     https://issues.apache.org/jira/browse/GIRAPH-504
> 
> 
> Diffs
> -----
> 
>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphClasses.java d2641f1 
>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphConfiguration.java 9ca1e7e 
>   giraph-core/src/main/java/org/apache/giraph/conf/GiraphConstants.java fb4e8a3 
>   giraph-core/src/main/java/org/apache/giraph/conf/ImmutableClassesGiraphConfiguration.java 30a7da7 
>   giraph-core/src/main/java/org/apache/giraph/examples/PartitionContextTestVertex.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/graph/ComputeCallable.java 94ed6d9 
>   giraph-core/src/main/java/org/apache/giraph/graph/GraphState.java 9cdec7c 
>   giraph-core/src/main/java/org/apache/giraph/partition/BasicPartition.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/partition/ByteArrayPartition.java d34af11 
>   giraph-core/src/main/java/org/apache/giraph/partition/DefaultPartitionContext.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/partition/Partition.java 55ce8c0 
>   giraph-core/src/main/java/org/apache/giraph/partition/PartitionContext.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/partition/SimplePartition.java 479011f 
>   giraph-core/src/main/java/org/apache/giraph/vertex/Vertex.java 974232e 
>   giraph-core/src/test/java/org/apache/giraph/BspCase.java 6aab533 
>   giraph-core/src/test/java/org/apache/giraph/TestPartitionContext.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/9372/diff/
> 
> 
> Testing
> -------
> 
> Added test for PartitionContext
> mvn clean verify
> 
> 
> Thanks,
> 
> Maja Kabiljo
> 
>