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: some weird code
Date Fri, 06 Jan 2012 21:19:13 GMT
Hi avery,
sorry forgot resolver was exported to user space. I ll consider this. About
your idea, it makes sense although I somehow I believe that if user space
messes up it s not our fault. Your solution though makes evrrybody happy.
Will implement this and send the separate patch. Thanks

On Friday, January 6, 2012, Avery Ching <aching@apache.org> wrote:
> Hi Claudio, answers inline:
>
> On 1/6/12 8:25 AM, Claudio Martella wrote:
>>
>> Hello,
>>
>> I hope somebody can shed some light on a piece of code i'm looking at
>> while working on GIRAPH-45 (and this code is also the object of
>> GIRAPH-95, so we'd probably get two birds with one stone here).
>>
>> The code is taking care of vertex resolving in
>> BasicRPCCommunication::prepareSuperstep():
>> [line 1091]:
>>            if (vertex != null) {
>>                 ((MutableVertex<I, V, E, M>)
vertex).setVertexId(vertexIndex);
>>                 partition.putVertex((BasicVertex<I, V, E, M>) vertex);
>>             } else if (originalVertex != null) {
>>                 partition.removeVertex(originalVertex.getVertexId());
>>             }
>>
>> First, vertex cannot be null as it's resolved by vertexRevolver, but i
>> guess it's a sanity check. But the real question is: why would you
>> setVertex() considering it's been already initialized correctly in
>> vertexResolver?
>
> Actually it can be null.  Since user's can implement their own vertex
resolver, they are allowed to return null from the javadoc.
>
>    /**
>     * A vertex may have been removed, created zero or more times and had
>     * zero or more messages sent to it.  This method will handle all
situations
>     * excluding the normal case (a vertex already exists and has zero or
more
>     * messages sent it to).
>     *
>     * @param vertexId Vertex id (can be used for {@link BasicVertex}'s
>     *        initialize())
>     * @param vertex Original vertex or null if none
>     * @param vertexChanges Changes that happened to this vertex or null
if none
>     * @param messages messages received in the last superstep or null if
none
>     * @return Vertex to be returned, if null, and a vertex currently
exists
>     *         it will be removed
>     */
>
>> Am I missing something or did I just realize that GIRAPH-95 is solved
>> by just removing that line? :)
>>
>> Thanks
>>
> Well, not sure about that.  The set is done there I think to ensure
safety.  Here's the issue:  Suppose that the resolve() doesn't set the
vertex id correctly (i.e. in this partition).  That would be a bug and
probably cause issues.  Probably this should be changed to be a check
though.  Something like...
>
>        if (vertex != null) {
>            if (vertex.getVertexId().equals(vertexIndex)) {
>                throw new IllegalStateException("BasicRPCCommunications:
Illegal to set the vertex index differently from " + vertexIndex);
>            if (originalVertex == null) {
>                partition.putVertex((BasicVertex<I, V, E, M>) vertex);
>            } else {
>                partition.removeVertex(originalVertex.getVertexId());
>            }
>        }
>
> What do you think?
>
> Avery
>

-- 
   Claudio Martella
   claudio.martella@gmail.com

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