[ 
https://issues.apache.org/jira/browse/GIRAPH-37?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13128526#comment-13128526
 ] 

Dmitriy V. Ryaboy commented on GIRAPH-37:
-----------------------------------------

Jacob, I haven't worked through the actual communication logic, but thought I'd 
drop in a few minor code style comments (this is just based on reading, didn't 
try running it, busy week):

First, I would suggest namespacing the autogen stuff into a separate package 
(o.a.g.comm.finaglerpc.gen ?). That would make it easier to see what's our code 
and what is auto-generated.

The strings "giraph.rpc.impl" and "giraph.rpc.finagle.debug" should be public 
static final and documented.  In fact we might want to start 
GiraphConfigStrings or something as a central place to keep these, instead of 
spreading them in undiscoverable hard-coded strings all across the codebase 
a-la hadoop..  Same for static counter names like "FinagleRPC stats".

What do you mean by (Synchronized) in the documentation for 
FinagleRPCCommunications.transientInMessages and inVertexRangeMap? The maps 
that you are using are not inherently synchronized.  Do you want to init them 
with something like Collections.synchronizedMap(new HashMap()) ? Or did I miss 
your meaning?  Also, transientFoo is a bit confusing given that Java has a 
transient keyword and Foos aren't transient in that sense. inflightInMessages? 

You specify things in comments like nullable and visible for testing that are 
available as annotations in Guava, which is nicer.. not reason enough to bring 
in yet enough dependency, but something to keep in mind. 

MAX_VERTICES_PER_RPC -- we are going to want that to be configurable, right?

In flush(), you are doing a lot of string concatenation in a peer-sized loop. 
Some of that is redundant (all but the name of the peer), and all of it would 
be better off with a StringBuilder.

Protocol Version -- why not? Granted, thrift is mostly backwards-compatible, 
but it's a bit of safety that won't hurt. Could just add it to the thrift 
struct.


                
> Implement Netty-backed rpc solution
> -----------------------------------
>
>                 Key: GIRAPH-37
>                 URL: https://issues.apache.org/jira/browse/GIRAPH-37
>             Project: Giraph
>          Issue Type: New Feature
>            Reporter: Jakob Homan
>            Assignee: Jakob Homan
>         Attachments: GIRAPH-37-wip.patch
>
>
> GIRAPH-12 considered replacing the current Hadoop based rpc method with 
> Netty, but didn't went in another direction. I think there is still value in 
> this approach, and will also look at Finagle.

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

        

Reply via email to