You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@giraph.apache.org by "Jake Mannix (JIRA)" <ji...@apache.org> on 2011/09/08 20:40:09 UTC
[jira] [Created] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Mutable static global state in Vertex.java should be refactored
---------------------------------------------------------------
Key: GIRAPH-27
URL: https://issues.apache.org/jira/browse/GIRAPH-27
Project: Giraph
Issue Type: Improvement
Components: graph
Affects Versions: 0.70.0
Reporter: Jake Mannix
Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Avery Ching (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100849#comment-13100849 ]
Avery Ching commented on GIRAPH-27:
-----------------------------------
I made some revisions to Jake's fix to
- Do not expose GraphState to application developers
- Fixing a few formatting issues
https://reviews.apache.org/r/1771/
I also passed unittests and ran the PageRankBenchmark on a Yahoo! cluster with 100 workers and 500k vertices. If Jake is okay with the changes then I'll commit it on his behalf.
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Dmitriy V. Ryaboy (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100852#comment-13100852 ]
Dmitriy V. Ryaboy commented on GIRAPH-27:
-----------------------------------------
Avery, looks like you forget to add GraphState to svn before generating the patch.
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "jiraposter@reviews.apache.org (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100857#comment-13100857 ]
jiraposter@reviews.apache.org commented on GIRAPH-27:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1771/
-----------------------------------------------------------
(Updated 2011-09-09 01:52:18.842526)
Review request for giraph.
Changes
-------
Dmitriy correctly pointed out I forgot to add GraphState.java to the diff.
Summary
-------
Based on Jake's submission https://issues.apache.org/jira/secure/attachment/12493654/GIRAPH-27.patch
Couple of small changes:
- Do not expose GraphState to application developers
- Fixing a few formatting issues
This addresses bug GIRAPH-27.
https://issues.apache.org/jira /browse/GIRAPH-27
Diffs (updated)
-----
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/bsp/CentralizedService.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/comm/BasicRPCCommunications.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspService.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspServiceWorker.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspUtils.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/GraphState.java PRE-CREATION
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/Vertex.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/VertexRange.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/test/java/org/apache/giraph/TestBspBasic.java 1166925
Diff: https://reviews.apache.org/r/1771/diff
Testing
-------
Unittest and page rank benchmark on Yahoo! grid with 10 workers.
Thanks,
Avery
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Jake Mannix (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100956#comment-13100956 ]
Jake Mannix commented on GIRAPH-27:
-----------------------------------
Maybe the right thing is yes, to have an AbstractVertex base class which implements BasicVertex, but also has a pair of package-protected final methods for get/setGraphState(), so that GraphMapper can use it, but that way subclasses in user-packages can't see it.
But then you'd need to require that users always subclassed that, or else they'd run into the issue I had, that it would compile, but not run.
Maybe the right approach is to turn BasicVertex into an abstract class, and have the package-protected method there, and get rid of the interface at all. Then all users would be required to subclass BasicVertex, instead of implement it. They already have to extend Vertex, so this just makes it clear what they have to extend.
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Jake Mannix (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100958#comment-13100958 ]
Jake Mannix commented on GIRAPH-27:
-----------------------------------
Wow, great minds think alike. I promise I didn't see your comment when I wrote mine (I'd need some mental help if I had).
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Hudson (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13101950#comment-13101950 ]
Hudson commented on GIRAPH-27:
------------------------------
Integrated in Giraph-trunk-Commit #3 (See [https://builds.apache.org/job/Giraph-trunk-Commit/3/])
GIRAPH-27: Mutable static global state in Vertex.java should be
refactored. jake.mannix via aching.
aching : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1167420
Files :
* /incubator/giraph/trunk/CHANGELOG
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/bsp/CentralizedService.java
* /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/BspService.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspServiceWorker.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspUtils.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/GraphState.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/MutableVertex.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/Vertex.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/VertexRange.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java
* /incubator/giraph/trunk/src/test/java/org/apache/giraph/TestBspBasic.java
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Jake Mannix (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13101026#comment-13101026 ]
Jake Mannix commented on GIRAPH-27:
-----------------------------------
+1 (non-committer) from me.
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Avery Ching (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100928#comment-13100928 ]
Avery Ching commented on GIRAPH-27:
-----------------------------------
That is actually intentional, since I need to have access to the get/setGraphState() internally and I removed the get/setGraphState() from BasicVertex. So rather than expose get/setGraphState() to the user (BasicVertex), I opted to to this. I suppose we could have another interface internally that extended BasicVertex to allow getting and setting the graph state if you're concerned about exposed the vertex to the internals. Let me know what you think.
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Avery Ching (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13101031#comment-13101031 ]
Avery Ching commented on GIRAPH-27:
-----------------------------------
Waiting on another committer to +1 before committing.
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Avery Ching (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100948#comment-13100948 ]
Avery Ching commented on GIRAPH-27:
-----------------------------------
One alternative is to change BasicVertex to an abstract class that implements get/setGraphState as package private methods. Users won't have access to get/setGraphState, while your primitive implementation would (since it's part of the same package). Thoughts? If you like it, I can submit a revised reviewboard request.
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Jake Mannix (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100992#comment-13100992 ]
Jake Mannix commented on GIRAPH-27:
-----------------------------------
Ok, looking great. Applied patch and merged into my primitive test, and found a couple of things:
* BasicVertex#getNumEdges(), getNumVertices(), and getSuperstep() are all delegating to GraphState, so they should be implemented in the base class, instead of left abstract, or else subclasses which aren't subclasses of Vertex will be duplicating these getters unncessesarily
* we're still not free from Vertex as a base: BspUtils#createVertex() and getVertexClass() still assume subclasses of Vertex. Similarly, VertexResolver#intantiateVertex() needs to return a *mutable* vertex. When is it allowed to subclass BasicVertex, and when must you subclass MutableVertex? If you're not mutable, you don't need to worry about being instantiated by the VertexResolver, so a class cast is acceptable?
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Hudson (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13102916#comment-13102916 ]
Hudson commented on GIRAPH-27:
------------------------------
Integrated in Giraph-trunk-Commit #4 (See [https://builds.apache.org/job/Giraph-trunk-Commit/4/])
GIRAPH-27: Fixed type parameter errors in Hudson (aching).
aching : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1169863
Files :
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/Vertex.java
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Dmitriy V. Ryaboy (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100666#comment-13100666 ]
Dmitriy V. Ryaboy commented on GIRAPH-27:
-----------------------------------------
non-committer +1
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Hyunsik Choi (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100811#comment-13100811 ]
Hyunsik Choi commented on GIRAPH-27:
------------------------------------
Looks great!
I agree on import reordering.
All unit tests are passed.
+1
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Jake Mannix (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100817#comment-13100817 ]
Jake Mannix commented on GIRAPH-27:
-----------------------------------
Can anyone try to run something like the SimplePageRank stuff on some real data on a real cluster, to make sure that cross-JVM boundaries are properly working? Hard to test the effect of this kind of change on that kind of thing in unit tests alone...
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Avery Ching (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100856#comment-13100856 ]
Avery Ching commented on GIRAPH-27:
-----------------------------------
Thanks. I just updated it.
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Avery Ching (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100829#comment-13100829 ]
Avery Ching commented on GIRAPH-27:
-----------------------------------
I'm revising it a little with some formatting changes, but overall it looks good. I'd like to submit a slight revision before commit.
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Hudson (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13106815#comment-13106815 ]
Hudson commented on GIRAPH-27:
------------------------------
Integrated in Giraph-trunk-Commit #7 (See [https://builds.apache.org/job/Giraph-trunk-Commit/7/])
GIRAPH-34: Failure of Vertex reflection for putVertexList from
GIRAPH-27. (aching)
aching : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1171776
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/examples/SimpleCheckpointVertex.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java
* /incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/Vertex.java
* /incubator/giraph/trunk/src/test/java/org/apache/giraph/TestVertexRangeBalancer.java
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Avery Ching (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13102986#comment-13102986 ]
Avery Ching commented on GIRAPH-27:
-----------------------------------
Oh, it's very annoying. This problem has bitten me many times. =)
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "jiraposter@reviews.apache.org (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13101020#comment-13101020 ]
jiraposter@reviews.apache.org commented on GIRAPH-27:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1771/
-----------------------------------------------------------
(Updated 2011-09-09 06:26:15.766329)
Review request for giraph.
Changes
-------
Moved as much of the getGraphState() related method implementations from Vertex to BasicVertex and MutableVertex. Other changes for primitive implementations can be done in another JIRA.
Summary
-------
Based on Jake's submission https://issues.apache.org/jira/secure/attachment/12493654/GIRAPH-27.patch
Couple of small changes:
- Do not expose GraphState to application developers
- Fixing a few formatting issues
This addresses bug GIRAPH-27.
https://issues.apache.org/jira /browse/GIRAPH-27
Diffs (updated)
-----
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/bsp/CentralizedService.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/comm/BasicRPCCommunications.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspService.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspServiceWorker.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspUtils.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/GraphState.java PRE-CREATION
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/MutableVertex.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/Vertex.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/VertexRange.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/test/java/org/apache/giraph/TestBspBasic.java 1166925
Diff: https://reviews.apache.org/r/1771/diff
Testing
-------
Unittest and page rank benchmark on Yahoo! grid with 10 workers.
Thanks,
Avery
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Avery Ching (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13101030#comment-13101030 ]
Avery Ching commented on GIRAPH-27:
-----------------------------------
Btw, with respect to import ordering, can you please voice your preferences on https://issues.apache.org/jira/browse/GIRAPH-21 ? Thanks!
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Jake Mannix (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100939#comment-13100939 ]
Jake Mannix commented on GIRAPH-27:
-----------------------------------
I'm down with not exposing GraphState to the users, but I'm concerned with basically requiring that users be subclasses of Vertex, and are not allowed to implement BasicVertex themselves, because if they did so, the framework would try to instantiate them as subclasses of Vertex, and fail with runtime exceptions (this is what happened to me in trying to make a primitive-specific alternative to Vertex, while keeping the API fixed).
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Jake Mannix (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jake Mannix updated GIRAPH-27:
------------------------------
Attachment: GIRAPH-27.patch
Code probably doesn't meet all giraph code conventions (as I've got Lucene / Mahout code conventions set in my IDE currently, I think)
Unit tests pass.
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Attachments: GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Jakob Homan (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13101602#comment-13101602 ]
Jakob Homan commented on GIRAPH-27:
-----------------------------------
+1. reviewboard on an iPad on hotel wifi sucks.
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Jake Mannix (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jake Mannix updated GIRAPH-27:
------------------------------
Attachment: GIRAPH-27.patch
javadocs, chaining setters, 4-space indent
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "jiraposter@reviews.apache.org (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100970#comment-13100970 ]
jiraposter@reviews.apache.org commented on GIRAPH-27:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1771/
-----------------------------------------------------------
(Updated 2011-09-09 04:44:32.531014)
Review request for giraph.
Changes
-------
Make BasicVertex and MutableVertex abstract classes. BasicVertex has package private methods for get/setGraphState().
Summary
-------
Based on Jake's submission https://issues.apache.org/jira/secure/attachment/12493654/GIRAPH-27.patch
Couple of small changes:
- Do not expose GraphState to application developers
- Fixing a few formatting issues
This addresses bug GIRAPH-27.
https://issues.apache.org/jira /browse/GIRAPH-27
Diffs (updated)
-----
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/bsp/CentralizedService.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/comm/BasicRPCCommunications.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspService.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspServiceWorker.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspUtils.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/GraphState.java PRE-CREATION
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/MutableVertex.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/Vertex.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/VertexRange.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/test/java/org/apache/giraph/TestBspBasic.java 1166925
Diff: https://reviews.apache.org/r/1771/diff
Testing
-------
Unittest and page rank benchmark on Yahoo! grid with 10 workers.
Thanks,
Avery
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Jake Mannix (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13101009#comment-13101009 ]
Jake Mannix commented on GIRAPH-27:
-----------------------------------
Ok, while I still hold that my previous comment is valid, it's probably addressable in another ticket, as it requires looking at a whole bunch of places (VertexResolver is one, but then CentralizedService#getRepresentativeVertex() is another, and VertexRange also needs to move from having Vertex in its Map, to MutableVertex).
While writing this comment, I tried out a refactoring on my branch which does allow for the existence of a MutableVertex subclass which is *not* a Vertex subclass, and in fact has, as a trial,
SimplePageRankVertex extends LongDoubleFloatDoubleVertex
and the unit tests all pass with this new class!
Probably needs tons of cleanup, but I can post it here if appropriate.
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Dmitriy V. Ryaboy (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100616#comment-13100616 ]
Dmitriy V. Ryaboy commented on GIRAPH-27:
-----------------------------------------
I took a brief look; it'd be great if we agreed on import ordering so that everyone's IDEs didn't reorder every time.
You do have some two-space padding in places; I believe Giraph conventions are 4 spaces.
public class GraphState needs a javadoc (what are I, V, E, and M? I know, but it'd be nice to have it in writing..) Said javadoc should probably include scary warnings about making sure that one doesn't wind up with multiple different states floating around in a job.
Can you make the GraphState setters chainable (return this instead of void)? That'll make creating them flow much nicer in GraphMapper.
Why remove the \<I\>, etc in calls to BspUtils methods in Vertex?
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Attachments: GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Avery Ching (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100830#comment-13100830 ]
Avery Ching commented on GIRAPH-27:
-----------------------------------
btw, I'll also run the page rank benchmark on a real cluster as well.
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Jake Mannix (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13102984#comment-13102984 ]
Jake Mannix commented on GIRAPH-27:
-----------------------------------
oh that's super annoying. I'll have to configure my IDE to make sure it is "bug-compatible" with Sun JRE. :\
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Jake Mannix (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13101011#comment-13101011 ]
Jake Mannix commented on GIRAPH-27:
-----------------------------------
Sounds great. Hopefully not too many changes from the current patch, because I've applied this patch to my branch and then did a bunch of what I described in my previous comment after that. And it would be sad to have to re-do that. :)
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Jake Mannix (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13102937#comment-13102937 ]
Jake Mannix commented on GIRAPH-27:
-----------------------------------
Why are you using redundant type parameters? Type inference gets you the type for free, and it compiles fine without it, and is unambiguous and more compact...
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Avery Ching (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13102970#comment-13102970 ]
Avery Ching commented on GIRAPH-27:
-----------------------------------
Jake, please see
https://builds.apache.org/job/Giraph-trunk-Commit/3/console
[ERROR] /x1/jenkins/jenkins-slave/workspace/Giraph-trunk-Commit/trunk/src/main/java/org/apache/giraph/graph/Vertex.java:[145,38] type parameters of <I>I cannot be determined; no unique maximal instance exists for type variable I with upper bounds I,org.apache.hadoop.io.WritableComparable
[ERROR] /x1/jenkins/jenkins-slave/workspace/Giraph-trunk-Commit/trunk/src/main/java/org/apache/giraph/graph/Vertex.java:[150,42] type parameters of <V>V cannot be determined; no unique maximal instance exists for type variable V with upper bounds V,org.apache.hadoop.io.Writable
[ERROR] /x1/jenkins/jenkins-slave/workspace/Giraph-trunk-Commit/trunk/src/main/java/org/apache/giraph/graph/Vertex.java:[163,43] type parameters of <M>M cannot be determined; no unique maximal instance exists for type variable M with upper bounds M,org.apache.hadoop.io.Writable
to see the errors without explicit types. This seems to be a known issue (http://stackoverflow.com/questions/2640060/inconsistency-between-sun-jre-javac-and-eclipse-java-compiler).
After the changes (build #4), it passed.
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "jiraposter@reviews.apache.org (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100848#comment-13100848 ]
jiraposter@reviews.apache.org commented on GIRAPH-27:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1771/
-----------------------------------------------------------
Review request for giraph.
Summary
-------
Based on Jake's submission https://issues.apache.org/jira/secure/attachment/12493654/GIRAPH-27.patch
Couple of small changes:
- Do not expose GraphState to application developers
- Fixing a few formatting issues
This addresses bug GIRAPH-27.
https://issues.apache.org/jira /browse/GIRAPH-27
Diffs
-----
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BasicVertex.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspService.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspServiceWorker.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/bsp/CentralizedService.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/comm/BasicRPCCommunications.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/BspUtils.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/GraphMapper.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/Vertex.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/VertexRange.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/main/java/org/apache/giraph/graph/VertexResolver.java 1166925
http://svn.apache.org/repos/asf/incubator/giraph/trunk/src/test/java/org/apache/giraph/TestBspBasic.java 1166925
Diff: https://reviews.apache.org/r/1771/diff
Testing
-------
Unittest and page rank benchmark on Yahoo! grid with 10 workers.
Thanks,
Avery
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Jake Mannix (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100639#comment-13100639 ]
Jake Mannix commented on GIRAPH-27:
-----------------------------------
4-space padding, but I code on a laptop! :( yeah, I guess I can fix that. Import ordering... oof. Imports should be a Set, not a List...
Javadocs sound good, and agreed on the scariness about moving state to a bean, which could possibly be instantiated multiple times, and nobody knows who's got what... either that, the static factory method (not avoiding statics there), or the elusive RecursiveReflectionFactoriesAllTheWayDownFactory.
Chainable setters sounds like a good call.
Removed the types in BspUtils because the type could be inferred, wasn't necessary.
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Attachments: GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Avery Ching (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13101010#comment-13101010 ]
Avery Ching commented on GIRAPH-27:
-----------------------------------
Let's do this Jake, I'll do some of the cleanup here and repost. If you agree, I'll commit. Then you can make any additional changes in another JIRA that is specific for your primitives implementation. What do you think? I can do some of it in the next 10-15 min.
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Commented] (GIRAPH-27) Mutable static global state in
Vertex.java should be refactored
Posted by "Jake Mannix (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/GIRAPH-27?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13100901#comment-13100901 ]
Jake Mannix commented on GIRAPH-27:
-----------------------------------
Awesome, thanks Avery. Looks good to me.
In looking over the diff in more detail in reviewboard, I notice that there are still a bunch of places where Vertex is referred to, but really BasicVertex (or at most MutableVertex) is all that's needed. But I'll open another ticket for those changes once this has been merged in.
> Mutable static global state in Vertex.java should be refactored
> ---------------------------------------------------------------
>
> Key: GIRAPH-27
> URL: https://issues.apache.org/jira/browse/GIRAPH-27
> Project: Giraph
> Issue Type: Improvement
> Components: graph
> Affects Versions: 0.70.0
> Reporter: Jake Mannix
> Assignee: Jake Mannix
> Attachments: GIRAPH-27.patch, GIRAPH-27.patch
>
>
> Vertex.java has a bunch of static methods for getting/setting global graph state (total number of vertices, edges, a reference to the GraphMapper, etc). Refactoring this into a GraphState object, which every Vertex can hold onto a reference to (yes, a tiny bit more memory per Vertex, but in comparison to what's already in there...)
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira