Re: some weird code
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
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
some weird code
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? Am I missing something or did I just realize that GIRAPH-95 is solved by just removing that line? :) Thanks -- Claudio Martella claudio.marte...@gmail.com
Re: some weird code
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