Jake Mannix commented on GIRAPH-36:

Ok, I'm digging into this again, finally... man patches get pretty stale if 
left unattended in the Incubator. :)

So, the idea with keeping BasicVertex which is *not* mutable is not for 
performance, but for implementation ease / coding safety: forcing users to 
implement mutating methods on their graph which doesn't allow such things is 
unfair to the users of this library, and having them throw 
UnsupportedOperationException should be avoided because then lots of problems 
are only seen at runtime, and then when the project moves forward and adds some 
innocuous thing which passes all unit tests, but ends up calling some mutator 
method that wasn't called before (maybe as part of a "post-initialization check 
of correctness, or something"), the users who are throwing exceptions in these 
methods only find out when they run their jobs which used to succeed, and now 
die with mysterious UOE getting thrown in new methods nowhere near their code.  
Bad news, if it can be avoided.

So I'm in favor of a) over b), in theory.  It's closest to the way things are 
done in hadoop, and seems the cleanest, from a purity of API standpoint.

However, having dug into the code a bit, it's going to be hairy to get a) to 
work easily, now that there is a proliferation of VertexInputFormat stuff that 
Jakob has been committing lately (yay! but boo. :( ) - all of this code would 
also need to be retrofitted to match.

So I'm going to take a stab at b).  It breaks the API least, and while it 
appears to allow mutation, it's pretty clear to the user that it's not really, 
and we can forcibly throw exceptions on multiple use, for example, to keep it 
from being reused.
> Ensure that subclassing BasicVertex is possible by user apps
> ------------------------------------------------------------
>                 Key: GIRAPH-36
>                 URL: https://issues.apache.org/jira/browse/GIRAPH-36
>             Project: Giraph
>          Issue Type: Improvement
>          Components: graph
>    Affects Versions: 0.70.0
>            Reporter: Jake Mannix
>            Assignee: Jake Mannix
>            Priority: Blocker
>             Fix For: 0.70.0
> Original assumptions in Giraph were that all users would subclass Vertex 
> (which extended MutableVertex extended BasicVertex).  Classes which wish to 
> have application specific data structures (ie. not a TreeMap<I, Edge<I,E>>) 
> may need to extend either MutableVertex or BasicVertex.  Unfortunately 
> VertexRange extends ArrayList<Vertex>, and there are other places where the 
> assumption is that vertex classes are either Vertex, or at least 
> MutableVertex.
> Let's make sure the internal APIs allow for BasicVertex to be the base class.

This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: 
For more information on JIRA, see: http://www.atlassian.com/software/jira


Reply via email to