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 Fri, 06 Jan 2012 20:09:45 GMT
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

Mime
View raw message