Re: some weird code

2012-01-08 Thread Claudio Martella
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.marte...@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 ach...@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) {
                 ((MutableVertexI, V, E, M)
 vertex).setVertexId(vertexIndex);
                 partition.putVertex((BasicVertexI, 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((BasicVertexI, V, E, M) vertex);
            } else {
                partition.removeVertex(originalVertex.getVertexId());
            }
        }

 What do you think?

 Avery


 --
    Claudio Martella
    claudio.marte...@gmail.com



-- 
   Claudio Martella
   claudio.marte...@gmail.com


Re: some weird code

2012-01-08 Thread Avery Ching

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.marte...@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 Chingach...@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) {
 ((MutableVertexI, V, E, M)
vertex).setVertexId(vertexIndex);
 partition.putVertex((BasicVertexI, 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((BasicVertexI, V, E, M) vertex);
} else {
partition.removeVertex(originalVertex.getVertexId());
}
}

What do you think?

Avery


--
Claudio Martella
claudio.marte...@gmail.com







Re: some weird code

2012-01-06 Thread Avery Ching

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) {
 ((MutableVertexI, V, E, M) vertex).setVertexId(vertexIndex);
 partition.putVertex((BasicVertexI, 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((BasicVertexI, V, E, M) vertex);
} else {
partition.removeVertex(originalVertex.getVertexId());
}
}

What do you think?

Avery


Re: some weird code

2012-01-06 Thread Claudio Martella
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 ach...@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) {
 ((MutableVertexI, V, E, M)
vertex).setVertexId(vertexIndex);
 partition.putVertex((BasicVertexI, 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((BasicVertexI, V, E, M) vertex);
} else {
partition.removeVertex(originalVertex.getVertexId());
}
}

 What do you think?

 Avery


-- 
   Claudio Martella
   claudio.marte...@gmail.com