incubator-giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Avery Ching <ach...@apache.org>
Subject Re: some weird code
Date Sun, 08 Jan 2012 21:31:37 GMT
Given our changes to Vertex, I think so. +1

Avery

On 1/8/12 8:28 AM, Claudio Martella wrote:
> One thing about the VertexResolver. Doesn't it make more sense if the
> interface is called VertexResolver and the default basic
> implementation is called BasicVertexResolver?
>
> On Fri, Jan 6, 2012 at 10:19 PM, Claudio Martella
> <claudio.martella@gmail.com>  wrote:
>> 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
View raw message