giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maja Kabiljo" <majakabi...@fb.com>
Subject Re: Review Request: Open Netty client and server on master
Date Fri, 24 Aug 2012 09:21:52 GMT


> On Aug. 24, 2012, 7:51 a.m., Avery Ching wrote:
> > 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).
> 
> Maja Kabiljo wrote:
>     Avery, thank you for the review.
>     
>     The way I see it, netty client and netty server should be classes which look at requests
just as writable objects, and independent of the current algorithm completely. So if we get
to a point that we need the types back we are doing something wrong :-) I've taken a look
at which warnings do we get. For NettyClient there is only one with RequestEncoder, and that
class also doesn't need to be parametrized anymore. For NettyServer we only have the part
with registering requests, and that can be done in a different way, we don't necessarily need
the instances of requests in order to register them. What do you think?

Also, I see we have @SuppressWarnings("rawtypes") all around the codebase mostly because of
two things:
- I extends WritableComparable without a type
- Mapper.Context without types
We can eliminate those warnings easily.


- Maja


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6753/#review10720
-----------------------------------------------------------


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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message