----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12174/#review22530 -----------------------------------------------------------
Good start. A couple of things below: 1) I had a suggestion about refactoring below to make the code cleaner and simpler. 2) Can you please also add a test that actually executes the code in a program? See TestGraphPartitioner as an example of running the same job with different parameters (in your case - testing your change and without). 3) Can you please demonstrate some cases where this improves performance? I.e. PageRankBenchmark The refactoring will be a big change, so I'll re-review completely when you finish it. giraph-core/src/main/java/org/apache/giraph/comm/SendCache.java <https://reviews.apache.org/r/12174/#comment46222> This should be private with a getter giraph-core/src/main/java/org/apache/giraph/comm/SendMessageCache.java <https://reviews.apache.org/r/12174/#comment46224> Given that these are all only used for the "one to all" message format, you should subclass SendMessageCache to be SendMessageToAllCache and then choose it at runtime. giraph-core/src/main/java/org/apache/giraph/comm/SendMessageCache.java <https://reviews.apache.org/r/12174/#comment46218> /** T giraph-core/src/main/java/org/apache/giraph/comm/SendMessageCache.java <https://reviews.apache.org/r/12174/#comment46220> vertex giraph-core/src/main/java/org/apache/giraph/comm/SendMessageCache.java <https://reviews.apache.org/r/12174/#comment46225> Please indent correctly. giraph-core/src/main/java/org/apache/giraph/comm/SendMessageCache.java <https://reviews.apache.org/r/12174/#comment46226> indent giraph-core/src/main/java/org/apache/giraph/comm/SendMessageCache.java <https://reviews.apache.org/r/12174/#comment46227> indent. giraph-core/src/main/java/org/apache/giraph/comm/SendMessageCache.java <https://reviews.apache.org/r/12174/#comment46228> weather -> whether giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClientRequestProcessor.java <https://reviews.apache.org/r/12174/#comment46230> After refactoring you should be able to structure this a bit better. NettyWorkerClientRequestProcessor should be able to delegate much of the work to the cache to prepare the message and then you can get rid of the logic inside of sendMessageToAllEdges. giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClientRequestProcessor.java <https://reviews.apache.org/r/12174/#comment46229> // Count giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClientRequestProcessor.java <https://reviews.apache.org/r/12174/#comment46232> Refactoring should allow us to just override the current method. - Avery Ching On June 28, 2013, 7:45 p.m., Bingjing Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12174/ > ----------------------------------------------------------- > > (Updated June 28, 2013, 7:45 p.m.) > > > Review request for giraph. > > > Bugs: GIRAPH-701 > https://issues.apache.org/jira/browse/GIRAPH-701 > > > Repository: giraph-git > > > Description > ------- > > Add "one-to-all" message sending strategy. > Now when sendMessageToAllEdges is invoked, Giraph may apply "one-to-all" > message sending strategy. > To enable it, use conf.enableOneToAllMsgSending() > > > Diffs > ----- > > giraph-core/src/main/java/org/apache/giraph/comm/SendCache.java 92d0926 > giraph-core/src/main/java/org/apache/giraph/comm/SendMessageCache.java > 40023c2 > giraph-core/src/main/java/org/apache/giraph/comm/ServerData.java affc260 > > giraph-core/src/main/java/org/apache/giraph/comm/WorkerClientRequestProcessor.java > 89fb3e4 > > giraph-core/src/main/java/org/apache/giraph/comm/messages/OneMessagePerVertexStore.java > f18af5b > > giraph-core/src/main/java/org/apache/giraph/comm/netty/NettyWorkerClientRequestProcessor.java > d786db5 > giraph-core/src/main/java/org/apache/giraph/comm/requests/RequestType.java > 4129fb8 > > giraph-core/src/main/java/org/apache/giraph/comm/requests/SendWorkerOneToAllMessagesRequest.java > PRE-CREATION > giraph-core/src/main/java/org/apache/giraph/conf/GiraphConfiguration.java > 2d232a6 > giraph-core/src/main/java/org/apache/giraph/conf/GiraphConstants.java > c65b5f0 > giraph-core/src/main/java/org/apache/giraph/graph/Computation.java 87d5879 > > giraph-core/src/main/java/org/apache/giraph/utils/ByteArrayOneToAllMessages.java > PRE-CREATION > > giraph-core/src/main/java/org/apache/giraph/utils/ByteArrayVertexIdData.java > 9b3f165 > giraph-core/src/test/java/org/apache/giraph/comm/RequestTest.java c8c09df > giraph-core/src/test/java/org/apache/giraph/utils/MockUtils.java bc5b5e2 > > Diff: https://reviews.apache.org/r/12174/diff/ > > > Testing > ------- > > Did > mvn clean verify > mvn clean test > > > Thanks, > > Bingjing Zhang > >
