giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alessandro Presta" <alessan...@fb.com>
Subject Re: Review Request: GIRAPH-547: Allow in-place modification of edges (apresta)
Date Wed, 20 Mar 2013 17:39:45 GMT


> On March 20, 2013, 12:13 a.m., Avery Ching wrote:
> > giraph-core/src/main/java/org/apache/giraph/graph/Vertex.java, lines 216-271
> > <https://reviews.apache.org/r/9943/diff/1-2/?file=270855#file270855line216>
> >
> >     Should this be in a separate class?  It's kind of a big anonymous class =).

Yes, good idea.


> On March 20, 2013, 12:13 a.m., Avery Ching wrote:
> > giraph-core/src/main/java/org/apache/giraph/graph/Vertex.java, line 279
> > <https://reviews.apache.org/r/9943/diff/1-2/?file=270855#file270855line279>
> >
> >     Do you want users to be able to override this?  Shouldn't we hide this from
users?

As usual, we have this problem because the caller (ComputeCallable) is in a different package
(other examples are setGraphState() and initialize()).
I don't expect the user to override this, so we could turn it to final (but that applies to
most of our codebase: right now we don't have a good separation between API and internals).
You could argue that a power user may want to call this explicitly in compute after iterating
over the mutable edges and breaking early, although it seems very unlikely.


> On March 20, 2013, 12:13 a.m., Avery Ching wrote:
> > giraph-core/src/main/java/org/apache/giraph/edge/MutableEdgesWrapper.java, lines
30-38
> > <https://reviews.apache.org/r/9943/diff/2/?file=271731#file271731line30>
> >
> >     Might want to add a bit more information to the comment that the edges are "unwrapped"
after computation of the vertex is complete.

Will do.


> On March 20, 2013, 12:13 a.m., Avery Ching wrote:
> > giraph-core/src/main/java/org/apache/giraph/edge/MutableEdgesWrapper.java, line
42
> > <https://reviews.apache.org/r/9943/diff/2/?file=271731#file271731line42>
> >
> >     I think this can be final if you make the right constructor.

Ok, I'll check that.


> On March 20, 2013, 12:13 a.m., Avery Ching wrote:
> > giraph-core/src/main/java/org/apache/giraph/edge/MutableEdgesWrapper.java, line
57
> > <https://reviews.apache.org/r/9943/diff/2/?file=271731#file271731line57>
> >
> >     Given you are using a static method to instantiate, do you want to make the
default constructor private?  Or better yet, make a private constructor that allows most of
your private variables to be final?

I'll see what can be done.


- Alessandro


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


On March 19, 2013, 12:47 a.m., Alessandro Presta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9943/
> -----------------------------------------------------------
> 
> (Updated March 19, 2013, 12:47 a.m.)
> 
> 
> Review request for giraph.
> 
> 
> Description
> -------
> 
> - Renamed the current MutableEdge to ReusableEdge, which must be considered a Giraph
internal. Introduced MutableEdge as an edge whose value can be mutated. This is what we expose
to the user when we want to allow in-place mutations.
> - Added Vertex#getMutableEdge() which returns an iterable of mutable edges. The remove()
method of its iterator works as expected.
> There is a default implementation that builds a new VertexEdges as we iterate, and specialized
implementations where it makes sense.
> - Added setEdgeValue() for completeness since we have getEdgeValue().
> - Added test cases to cover all default and specialized implementations.
> - Removed unused PairListWritable class.
> 
> 
> This addresses bug GIRAPH-547.
>     https://issues.apache.org/jira/browse/GIRAPH-547
> 
> 
> Diffs
> -----
> 
>   giraph-core/src/main/java/org/apache/giraph/conf/ImmutableClassesGiraphConfiguration.java
7075999826b66d59911e1ab864245fed5de88d02 
>   giraph-core/src/main/java/org/apache/giraph/edge/ArrayListEdges.java 98b1aefc7c8df38cc60675f8759cce890e50d5a7

>   giraph-core/src/main/java/org/apache/giraph/edge/ByteArrayEdges.java 6201d257fb11c9607cf13b5ccd9834a5c729fc39

>   giraph-core/src/main/java/org/apache/giraph/edge/DefaultEdge.java 461bff37ac4f1ffe13a4fd12572c7f23998a6d00

>   giraph-core/src/main/java/org/apache/giraph/edge/EdgeFactory.java 35992074c15dcbdbdf6c9596b9cb383b3ada15ad

>   giraph-core/src/main/java/org/apache/giraph/edge/EdgeNoValue.java dd22aecf996f0ffa098df213a2a5b87afa51d006

>   giraph-core/src/main/java/org/apache/giraph/edge/EdgeStore.java 1f6e9bb4d37aa9a5f37f24e3c14fd0945b4cacc8

>   giraph-core/src/main/java/org/apache/giraph/edge/HashMapEdges.java 2600992b39a7fab770c33470e110700d331d96b7

>   giraph-core/src/main/java/org/apache/giraph/edge/HashMultimapEdges.java 143d7a4bf76fd0dfe2882eea08e7421611dc9db5

>   giraph-core/src/main/java/org/apache/giraph/edge/LongDoubleArrayEdges.java f164484bc7b5ce5c900c37eae39671ca320ee4e1

>   giraph-core/src/main/java/org/apache/giraph/edge/LongDoubleHashMapEdges.java 68bd85f94f1029d53b4e27e98154d3623c693320

>   giraph-core/src/main/java/org/apache/giraph/edge/LongNullArrayEdges.java 528acb278507b441a044dbcdff507ca8ab0e89d9

>   giraph-core/src/main/java/org/apache/giraph/edge/LongNullHashSetEdges.java 26c57aef6326acdee06a28f53b5a2691d1c0eb5e

>   giraph-core/src/main/java/org/apache/giraph/edge/MapMutableEdge.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/edge/MutableEdge.java bf00b4f851db703f2097cc0a5608d425aa8f03b1

>   giraph-core/src/main/java/org/apache/giraph/edge/MutableEdgesWrapper.java PRE-CREATION

>   giraph-core/src/main/java/org/apache/giraph/edge/MutableVertexEdges.java PRE-CREATION

>   giraph-core/src/main/java/org/apache/giraph/edge/ReusableEdge.java PRE-CREATION 
>   giraph-core/src/main/java/org/apache/giraph/edge/StrictRandomAccessVertexEdges.java
36381a70533e93296280bd3507df05b16f16d8b3 
>   giraph-core/src/main/java/org/apache/giraph/graph/ComputeCallable.java 439ee5b2a46a290c2c4c8f184dbf39806f2a9f9a

>   giraph-core/src/main/java/org/apache/giraph/graph/Vertex.java 66f081a37a6533457ebe5d0dd1c50cc80ae347da

>   giraph-core/src/main/java/org/apache/giraph/utils/ByteArrayVertexIdEdges.java 8a5fb01685a96dc503cf98b6daa3579e9ea891fd

>   giraph-core/src/main/java/org/apache/giraph/utils/PairListWritable.java 413656b530ecb47c39f6e68a1dfd55e8e327938a

>   giraph-core/src/test/java/org/apache/giraph/edge/TestNullValueEdges.java ef3fbc8469bfdc849657852d713839379cfcc2a1

>   giraph-core/src/test/java/org/apache/giraph/edge/TestStrictRandomAccessEdges.java e6daa94f92a2e201019618692426fa5a59415b57

>   giraph-core/src/test/java/org/apache/giraph/graph/TestVertexAndEdges.java 425abe7f7adf852aec198b15e516c3f1e8a6d7b0

>   giraph-hive/src/main/java/org/apache/giraph/hive/input/edge/HiveEdgeReader.java e0c502c8942158be1c151d986302077c0feb520b

> 
> Diff: https://reviews.apache.org/r/9943/diff/
> 
> 
> Testing
> -------
> 
> 1) mvn verify
> 2) Implemented weighted pagerank with weight normalization that uses the new functionality
> 
> 
> Thanks,
> 
> Alessandro Presta
> 
>


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