----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6753/#review10720 -----------------------------------------------------------
This looks like a nice refactoring to handle different response for the master and workers. Looks like a great start to adding direct communication to the master. I have a few minor things below, but I think we shouldn't be removing the types out of the classes and relying on warning suppression. I believe the suppresion exists is because of Eclipse showing warnings (Intellij doesn't). I'd prefer to keep the types there because if we need them later on down the line, it is going to be a mess to add the types in (due to crazy weird dependencies). http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/MasterRequest.java <https://reviews.apache.org/r/6753/#comment23136> Shouldn't we make this an interface in this case (no implementation of any sort)? http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/NettyClient.java <https://reviews.apache.org/r/6753/#comment23137> We should remove the suppresion here instead I think and keep the types. http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/NettyServer.java <https://reviews.apache.org/r/6753/#comment23138> Again, we should remove the suppression here and keep the types. http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/WorkerRequestServerHandler.java <https://reviews.apache.org/r/6753/#comment23139> extra line. - Avery Ching On Aug. 23, 2012, 6:41 p.m., Maja Kabiljo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6753/ > ----------------------------------------------------------- > > (Updated Aug. 23, 2012, 6:41 p.m.) > > > Review request for giraph. > > > Description > ------- > > For GIRAPH-273 first thing we need is to open Netty communication on master, > make connections to workers and make connections from workers to master. > > Since it's already significant amount of code I'm opening a separate issue > for it. > > This doesn't send any messages for now, but I tested it with some dummy > messages and it works. The patch adds all the client/server classes we had > for worker for master, and does some redesign so common parts could be > reused. Again there are some useNetty checks which we'll be able to remove > soon. > > > This addresses bug GIRAPH-313. > https://issues.apache.org/jira/browse/GIRAPH-313 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/bsp/CentralizedServiceWorker.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/MasterClient.java > PRE-CREATION > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/MasterClientServer.java > PRE-CREATION > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/MasterRequest.java > PRE-CREATION > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/MasterRequestServerHandler.java > PRE-CREATION > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/MasterServer.java > PRE-CREATION > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/NettyClient.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/NettyMasterClient.java > PRE-CREATION > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/NettyMasterClientServer.java > PRE-CREATION > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/NettyMasterServer.java > PRE-CREATION > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/NettyServer.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/NettyWorkerClient.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/NettyWorkerServer.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/RequestDecoder.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/RequestEncoder.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/RequestInfo.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/RequestRegistry.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/RequestServerHandler.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/SendPartitionMessagesRequest.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/SendPartitionMutationsRequest.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/SendVertexRequest.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/WorkerRequest.java > PRE-CREATION > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/WorkerRequestServerHandler.java > PRE-CREATION > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/comm/WritableRequest.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/BspService.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/BspServiceMaster.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/main/java/org/apache/giraph/graph/BspServiceWorker.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/giraph/comm/ConnectionTest.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/giraph/comm/RequestFailureTest.java > 1375970 > > http://svn.apache.org/repos/asf/giraph/trunk/src/test/java/org/apache/giraph/comm/RequestTest.java > 1375970 > > Diff: https://reviews.apache.org/r/6753/diff/ > > > Testing > ------- > > Passes mvn verify and pseudo distributed tests. > > > Thanks, > > Maja Kabiljo > >
