You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@giraph.apache.org by "Sebastian Schelter (Created) (JIRA)" <ji...@apache.org> on 2011/11/14 17:15:52 UTC
[jira] [Created] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Don't expose the list holding the messages in BasicVertex
---------------------------------------------------------
Key: GIRAPH-80
URL: https://issues.apache.org/jira/browse/GIRAPH-80
Project: Giraph
Issue Type: Improvement
Affects Versions: 0.70.0
Reporter: Sebastian Schelter
I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13170542#comment-13170542 ]
jiraposter@reviews.apache.org commented on GIRAPH-80:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3203/#review3929
-----------------------------------------------------------
/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java
line 189, 192: broken code conventions, put a new line after you open the block and close at new line as well. the convention you're using is for fields. Also please explain with one sentence about the semantic of the parameter and how it is supposed to use it. users might not read the JIRAs.
For the rest I'm happy with it, nice work.
I don't have availability of real cluster here at the moment, if somebody can launch the testing on a real cluster I'd be ready to commit this one.
Otherwise I should be able to run in on monday or so.
- Claudio
On 2011-12-15 10:42:39, Sebastian Schelter wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/3203/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-12-15 10:42:39)
bq.
bq.
bq. Review request for giraph.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. refactoring that gives BasicVertex this 3 new methods:
bq.
bq. public abstract Iterable<M> getMessages()
bq.
bq. returns an unmodifiable iterable allowing access to the current messages
bq.
bq. public abstract void setMessages(Iterable<M> messages);
bq.
bq. replacement for getMsgList().clear() followed by getMsgList().addAll(...);
bq.
bq. public abstract void releaseResources();
bq.
bq. after a vertex voted to halt, all references to messages it could still hold should be removed to enable earlier GC, instead of externally calling replacement for getMsgList().clear(), this method should be used
bq.
bq. Local unit tests pass, unfortunately I wasn't yet able to run the tests on my hadoop cluster (still have problems because I can only access it via a socks proxy)
bq.
bq.
bq. This addresses bug GIRAPH-80.
bq. https://issues.apache.org/jira/browse/GIRAPH-80
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. /trunk/src/main/java/org/apache/giraph/comm/BasicRPCCommunications.java 1214675
bq. /trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java 1214675
bq. /trunk/src/main/java/org/apache/giraph/graph/BasicVertexResolver.java 1214675
bq. /trunk/src/main/java/org/apache/giraph/graph/EdgeListVertex.java 1214675
bq. /trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1214675
bq. /trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java 1214675
bq. /trunk/src/main/java/org/apache/giraph/graph/Vertex.java 1214675
bq. /trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java 1214675
bq. /trunk/src/main/java/org/apache/giraph/utils/ComparisonUtils.java PRE-CREATION
bq. /trunk/src/main/java/org/apache/giraph/utils/MemoryUtils.java 1214675
bq. /trunk/src/test/java/org/apache/giraph/utils/ComparisonUtilsTest.java PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/3203/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Sebastian
bq.
bq.
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Assigned] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "Avery Ching (Assigned) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Avery Ching reassigned GIRAPH-80:
---------------------------------
Assignee: Sebastian Schelter
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
> Assignee: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "Avery Ching (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13170651#comment-13170651 ]
Avery Ching commented on GIRAPH-80:
-----------------------------------
By the way Sebastian, you can run the Hadoop tests against a single node Hadoop instance (I often do this on my laptop). It makes it much easier to run this test and takes me about 17 minutes or so. Not too bad.
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "Arun Suresh (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13150354#comment-13150354 ]
Arun Suresh commented on GIRAPH-80:
-----------------------------------
While the getMessageIterator() method seems like a good idea, I was wondering why we would need and addMessages(Iterable<M> messages) method. What would that method signify ? Does it allow the Vertex to add More messages to the list of messages it already received at the beginning of that superstep ? Does it mean the added messages are available to the compute function in the current superstep or next superstep ? In which case we already have the sendMsg(I id, M msg) function.
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13170645#comment-13170645 ]
jiraposter@reviews.apache.org commented on GIRAPH-80:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3203/#review3936
-----------------------------------------------------------
I think that overall this looks pretty nice. I do have a couple of suggestions. Also in the CODE_CONVENTIONS, comments should start with a capital letter i.e. (// This convention is silly).
/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java
<https://reviews.apache.org/r/3203/#comment8881>
Should be package-private to avoid the user from mucking around with the message data structure.
/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java
<https://reviews.apache.org/r/3203/#comment8884>
Should be package-private to only be called by the infrastructure.
Can we capitalize the comments? I.e. /** Release...
Also the comment is not quite right. releaseResources() will be called after the computation of the vertex, not only after a halted vertex.
/trunk/src/main/java/org/apache/giraph/utils/MemoryUtils.java
<https://reviews.apache.org/r/3203/#comment8880>
Thanks for fixing this (my bad)! Argh.
- Avery
On 2011-12-15 10:42:39, Sebastian Schelter wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/3203/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-12-15 10:42:39)
bq.
bq.
bq. Review request for giraph.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. refactoring that gives BasicVertex this 3 new methods:
bq.
bq. public abstract Iterable<M> getMessages()
bq.
bq. returns an unmodifiable iterable allowing access to the current messages
bq.
bq. public abstract void setMessages(Iterable<M> messages);
bq.
bq. replacement for getMsgList().clear() followed by getMsgList().addAll(...);
bq.
bq. public abstract void releaseResources();
bq.
bq. after a vertex voted to halt, all references to messages it could still hold should be removed to enable earlier GC, instead of externally calling replacement for getMsgList().clear(), this method should be used
bq.
bq. Local unit tests pass, unfortunately I wasn't yet able to run the tests on my hadoop cluster (still have problems because I can only access it via a socks proxy)
bq.
bq.
bq. This addresses bug GIRAPH-80.
bq. https://issues.apache.org/jira/browse/GIRAPH-80
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. /trunk/src/main/java/org/apache/giraph/comm/BasicRPCCommunications.java 1214675
bq. /trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java 1214675
bq. /trunk/src/main/java/org/apache/giraph/graph/BasicVertexResolver.java 1214675
bq. /trunk/src/main/java/org/apache/giraph/graph/EdgeListVertex.java 1214675
bq. /trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1214675
bq. /trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java 1214675
bq. /trunk/src/main/java/org/apache/giraph/graph/Vertex.java 1214675
bq. /trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java 1214675
bq. /trunk/src/main/java/org/apache/giraph/utils/ComparisonUtils.java PRE-CREATION
bq. /trunk/src/main/java/org/apache/giraph/utils/MemoryUtils.java 1214675
bq. /trunk/src/test/java/org/apache/giraph/utils/ComparisonUtilsTest.java PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/3203/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Sebastian
bq.
bq.
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "Claudio Martella (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13170178#comment-13170178 ]
Claudio Martella commented on GIRAPH-80:
----------------------------------------
is this re-based after GIRAPH-104?
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "Avery Ching (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13149876#comment-13149876 ]
Avery Ching commented on GIRAPH-80:
-----------------------------------
Since
public abstract void compute(Iterator<M> msgIterator) throws IOException;
already gives the msgIterator to the user, getMsgList() should be used to internally transfer the messages to the vertex and get the message iterator.
So instead of in BasicVertex
public abstract List<M> getMsgList();
do something like
void abstract addMessages(Collection<M> messages);
Iterator<M> abstract getMessageIterator();
Thoughts?
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "Hudson (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13171792#comment-13171792 ]
Hudson commented on GIRAPH-80:
------------------------------
Integrated in Giraph-trunk-Commit #51 (See [https://builds.apache.org/job/Giraph-trunk-Commit/51/])
GIRAPH-80: Don't expose the list holding the messages in
BasicVertex. (ssc via aching)
aching : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1220343
Files :
* /incubator/giraph/trunk/CHANGELOG
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/comm/BasicRPCCommunications.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BasicVertexResolver.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/EdgeListVertex.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/Vertex.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/utils/ComparisonUtils.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/utils/MemoryUtils.java
* /incubator/giraph/trunk/src/test/java/org/apache/giraph/utils/ComparisonUtilsTest.java
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "Avery Ching (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13170398#comment-13170398 ]
Avery Ching commented on GIRAPH-80:
-----------------------------------
Guys, GIRAPH-104 has been committed, the svn commit email was sent at 12/14/11 11:06 AM. It is present both in svn and git.
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "Jake Mannix (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13149862#comment-13149862 ]
Jake Mannix commented on GIRAPH-80:
-----------------------------------
+1 to returning an Iterable
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "Claudio Martella (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13150372#comment-13150372 ]
Claudio Martella commented on GIRAPH-80:
----------------------------------------
I favor the Iterable<M> approach, as suggested by Jake, too.
I was going to file this same JIRA today, good job Sebastian :)
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13171498#comment-13171498 ]
jiraposter@reviews.apache.org commented on GIRAPH-80:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3203/
-----------------------------------------------------------
(Updated 2011-12-17 09:36:06.229668)
Review request for giraph.
Summary (updated)
-------
refactoring that gives BasicVertex this 3 new methods:
public abstract Iterable<M> getMessages()
returns an unmodifiable iterable allowing access to the current messages
public abstract void setMessages(Iterable<M> messages);
replacement for getMsgList().clear() followed by getMsgList().addAll(...);
public abstract void releaseResources();
after a vertex voted to halt, all references to messages it could still hold should be removed to enable earlier GC, instead of externally calling replacement for getMsgList().clear(), this method should be used
Local unit tests pass, unfortunately I wasn't yet able to run the tests on my hadoop cluster (still have problems because I can only access it via a socks proxy)
This addresses bug GIRAPH-80.
https://issues.apache.org/jira/browse/GIRAPH-80
Diffs
-----
/trunk/src/main/java/org/apache/giraph/comm/BasicRPCCommunications.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/BasicVertexResolver.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/EdgeListVertex.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/Vertex.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java 1215442
/trunk/src/main/java/org/apache/giraph/utils/ComparisonUtils.java PRE-CREATION
/trunk/src/main/java/org/apache/giraph/utils/MemoryUtils.java 1215442
/trunk/src/test/java/org/apache/giraph/utils/ComparisonUtilsTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/3203/diff
Testing
-------
Thanks,
Sebastian
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13171788#comment-13171788 ]
jiraposter@reviews.apache.org commented on GIRAPH-80:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3203/#review3967
-----------------------------------------------------------
Ship it!
+1. Thanks for the changes. I will commit and then open up a separate JIRA to make setMessages() package-private.
- Avery
On 2011-12-17 09:36:06, Sebastian Schelter wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/3203/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-12-17 09:36:06)
bq.
bq.
bq. Review request for giraph.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. refactoring that gives BasicVertex this 3 new methods:
bq.
bq. public abstract Iterable<M> getMessages()
bq.
bq. returns an unmodifiable iterable allowing access to the current messages
bq.
bq. public abstract void setMessages(Iterable<M> messages);
bq.
bq. replacement for getMsgList().clear() followed by getMsgList().addAll(...);
bq.
bq. public abstract void releaseResources();
bq.
bq. after a vertex voted to halt, all references to messages it could still hold should be removed to enable earlier GC, instead of externally calling replacement for getMsgList().clear(), this method should be used
bq.
bq. Local unit tests pass, unfortunately I wasn't yet able to run the tests on my hadoop cluster (still have problems because I can only access it via a socks proxy)
bq.
bq.
bq. This addresses bug GIRAPH-80.
bq. https://issues.apache.org/jira/browse/GIRAPH-80
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. /trunk/src/main/java/org/apache/giraph/comm/BasicRPCCommunications.java 1215442
bq. /trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java 1215442
bq. /trunk/src/main/java/org/apache/giraph/graph/BasicVertexResolver.java 1215442
bq. /trunk/src/main/java/org/apache/giraph/graph/EdgeListVertex.java 1215442
bq. /trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1215442
bq. /trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java 1215442
bq. /trunk/src/main/java/org/apache/giraph/graph/Vertex.java 1215442
bq. /trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java 1215442
bq. /trunk/src/main/java/org/apache/giraph/utils/ComparisonUtils.java PRE-CREATION
bq. /trunk/src/main/java/org/apache/giraph/utils/MemoryUtils.java 1215442
bq. /trunk/src/test/java/org/apache/giraph/utils/ComparisonUtilsTest.java PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/3203/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq.
bq. Thanks,
bq.
bq. Sebastian
bq.
bq.
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "Sebastian Schelter (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13150367#comment-13150367 ]
Sebastian Schelter commented on GIRAPH-80:
------------------------------------------
sendMsg() is used for sending out messages to other vertices. New messages for a particular vertex are currently added via getMsgList().clear() and getMsgList().add(...) This is bad style as the internal datastructure of the vertex is externally modified. It also prevents convenient usage of more memory efficient datastructures that don't implement java.util.List like those provided by mahout-collections.
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "Claudio Martella (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13170295#comment-13170295 ]
Claudio Martella commented on GIRAPH-80:
----------------------------------------
I guess the problem is that Avery got my +1 but didn't commit yet. Your two patches are definitely going to conflict "quite a lot".
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13170115#comment-13170115 ]
jiraposter@reviews.apache.org commented on GIRAPH-80:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3203/
-----------------------------------------------------------
Review request for giraph.
Summary
-------
refactoring that gives BasicVertex this 3 new methods:
public abstract Iterable<M> getMessages()
returns an unmodifiable iterable allowing access to the current messages
public abstract void setMessages(Iterable<M> messages);
replacement for getMsgList().clear() followed by getMsgList().addAll(...);
public abstract void releaseResources();
after a vertex voted to halt, all references to messages it could still hold should be removed to enable earlier GC, instead of externally calling replacement for getMsgList().clear(), this method should be used
Local unit tests pass, unfortunately I wasn't yet able to run the tests on my hadoop cluster (still have problems because I can only access it via a socks proxy)
This addresses bug GIRAPH-80.
https://issues.apache.org/jira/browse/GIRAPH-80
Diffs
-----
/trunk/src/main/java/org/apache/giraph/comm/BasicRPCCommunications.java 1214675
/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java 1214675
/trunk/src/main/java/org/apache/giraph/graph/BasicVertexResolver.java 1214675
/trunk/src/main/java/org/apache/giraph/graph/EdgeListVertex.java 1214675
/trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1214675
/trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java 1214675
/trunk/src/main/java/org/apache/giraph/graph/Vertex.java 1214675
/trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java 1214675
/trunk/src/main/java/org/apache/giraph/utils/ComparisonUtils.java PRE-CREATION
/trunk/src/main/java/org/apache/giraph/utils/MemoryUtils.java 1214675
/trunk/src/test/java/org/apache/giraph/utils/ComparisonUtilsTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/3203/diff
Testing
-------
Thanks,
Sebastian
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13171499#comment-13171499 ]
jiraposter@reviews.apache.org commented on GIRAPH-80:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3203/
-----------------------------------------------------------
(Updated 2011-12-17 09:35:51.778265)
Review request for giraph.
Changes
-------
Updated the patch to include proposed changes. Unfortunately setMessages(...) cannot be package-private as it is invoked by BasicRPCCommunications.
Ran tests in local and pseudo-distributed mode.
Summary
-------
refactoring that gives BasicVertex this 3 new methods:
public abstract Iterable<M> getMessages()
returns an unmodifiable iterable allowing access to the current messages
public abstract void setMessages(Iterable<M> messages);
replacement for getMsgList().clear() followed by getMsgList().addAll(...);
public abstract void releaseResources();
after a vertex voted to halt, all references to messages it could still hold should be removed to enable earlier GC, instead of externally calling replacement for getMsgList().clear(), this method should be used
Local unit tests pass, unfortunately I wasn't yet able to run the tests on my hadoop cluster (still have problems because I can only access it via a socks proxy)
This addresses bug GIRAPH-80.
https://issues.apache.org/jira/browse/GIRAPH-80
Diffs (updated)
-----
/trunk/src/main/java/org/apache/giraph/comm/BasicRPCCommunications.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/BasicVertexResolver.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/EdgeListVertex.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/LongDoubleFloatDoubleVertex.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/Vertex.java 1215442
/trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java 1215442
/trunk/src/main/java/org/apache/giraph/utils/ComparisonUtils.java PRE-CREATION
/trunk/src/main/java/org/apache/giraph/utils/MemoryUtils.java 1215442
/trunk/src/test/java/org/apache/giraph/utils/ComparisonUtilsTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/3203/diff
Testing
-------
Thanks,
Sebastian
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "Sebastian Schelter (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13170192#comment-13170192 ]
Sebastian Schelter commented on GIRAPH-80:
------------------------------------------
It should apply to current trunk, do you experience any problems?
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "Jakob Homan (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13149808#comment-13149808 ]
Jakob Homan commented on GIRAPH-80:
-----------------------------------
+1. We should avoid return whole lists in general. Iterators are more future-proof against the time when we'll be doing tricks to avoid running out of memory when working when huge, dense graphs. The fewer implementation details we leak, the easier our lives will be.
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "Jake Mannix (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13149887#comment-13149887 ]
Jake Mannix commented on GIRAPH-80:
-----------------------------------
Can we change both addMessages() and getMessages() to take/return Iterable<M>? That way it can be scanned multiple times without multiple calls to the vertex.
void abstract addMessages(Iterable<M> messages);
Iterable<M> abstract getMessages();
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "Sebastian Schelter (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13170299#comment-13170299 ]
Sebastian Schelter commented on GIRAPH-80:
------------------------------------------
No problem. I can rework them to fit GIRAPH-104 once it's committed.
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "Avery Ching (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13149891#comment-13149891 ]
Avery Ching commented on GIRAPH-80:
-----------------------------------
Seems reasonable to me. Having the option of foreach and iterator is nice.
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-80) Don't expose the list holding the
messages in BasicVertex
Posted by "Sebastian Schelter (Commented) (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-80?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13150012#comment-13150012 ]
Sebastian Schelter commented on GIRAPH-80:
------------------------------------------
I'd also favor using Iterable<M>
> Don't expose the list holding the messages in BasicVertex
> ---------------------------------------------------------
>
> Key: GIRAPH-80
> URL: https://issues.apache.org/jira/browse/GIRAPH-80
> Project: Giraph
> Issue Type: Improvement
> Affects Versions: 0.70.0
> Reporter: Sebastian Schelter
>
> I'm currently trying to implement my own memory efficient vertex (similar to LongDoubleFloatDoubleVertex) and ran into problems with getMsgList()
> This method returns a list pointing to the messages of the vertex and it is modified externally (BasicRPCCommunications calls clear() and addAll() e.g.). This makes it very hard to use something else than a java.util.List internally (LongDoubleFloatDoubleVertex "hacked" around this) and it is generally dangerous to have the internal state of an object be modified externally. It also makes the code harder to read and understand.
> I'd suggest to change the API to let a vertex handle the modifications itself internally (e.g. add something like pushMessages(...))
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira