giraph-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Eli Reisman" <initialcont...@gmail.com>
Subject Re: Review Request: GIRAPH-211: Add secure authentication to Netty IPC
Date Tue, 04 Sep 2012 18:00:51 GMT


> On Aug. 30, 2012, 3:34 a.m., Eugene Koontz wrote:
> >
> 
> Maja Kabiljo wrote:
>     Eugene, this seems like a lot of great work, I'm grateful I was advised against trying
to solve this issue when I first started :-)
>     
>     When I run "mvn -Phadoop_2.0.1 clean test" I get: 
>     The requested profile "hadoop_2.0.1" could not be activated because it does not exist
>     And with other profiles this code doesn't compile.
>     
>     When merging with GIRAPH-313 you reverted a few things which were changed there.
I removed ServerData from RequestServerHandler and made separate handlers for worker and master
with the idea that they will have different kinds of data and therefore different methods
which can be called on it. Since right now master communication is not used yet you didn't
have any problems with the tests, and it seems to me this addition won't work well with it.
I suggest putting secretManager somewhere else, or we could make ServerData abstract and keep
the secretManager there and then make two implementations of it, one for worker with current
stuff. Also you should return to calling processRequest instead of request.doRequest in RequestServerHandler.
Or if you have some better suggestion on how to have different requests work on different
data.
> 
> Eli Reisman wrote:
>     Yeah it looks like the patch might have gone a bit stale. This is great work though,
I am learning a lot about SASL and our security model just reading it! I'll be happy to come
back and try to review this properly when the patch is rebased.
>     
>     As for the doRequest/process request thing, I meant to ask about that change before.
Seems like its a good pattern for WritableRequests to meet a common doRequest-style interface
and let whatever any subclass of WritableRequest needs to do happen in there (like the Command
pattern.) It does avoid the unneeded generics and avoids putting the responsibility on the
Handler's processRequest() to figure out what each request needs to do, what type it is, and
how to do it. Letting the Handler just call a known API like doRequest (or call it processRequest)
implemented at the Request side and supplied with the objects it needs to have side effects
on keeps each code path separate in its own WritableRequest object. As long as there's a big
rebase to do here anyway, maybe it wouldn't be bad if couple things like that could get reinstated
in some sensible way?
>     
>     Either way, great work. This is crucial for getting rid of the old RPC and I'll be
excited to see this in the codebase!
>
> 
> Eli Reisman wrote:
>     Never mind, I see doRequest is still in there in the subclasses of the Handlers.
So processRequest is like this because of the new master communication channels, and the master
one doesn't get a ServerData as input? Fair enough. So Maja: you're saying the trouble for
this patch is that the master requests don't' get access to the SeverData but the current
version of the patch expects one to be there in a request?
>     
>
> 
> Maja Kabiljo wrote:
>     Eli, I just mentioned this because Eugene was saying he merged with 313.
>     
>     If I understand this patch correctly, it's expected that we have ServerData object
in order to be able to do secure communication, and on master side it's set to null (and we
also shouldn't have ServerData object in its current state on master because it has stuff
master shouldn't have). 
>     
>     As for processRequest/doRequest, honestly, I'm not very happy with the way I solved
it. Reasons being that even though we separated the data we have on worker and on master,
for worker requests which don't deal with graph data we'll still have to have generics so
we would be able to have doRequest(ServerData<I,V,E,M>). I couldn't figure out how to
solve that in a nice way, and still have RequestHandler treating all requests in the same
way. It's like we want to have several different doRequest signatures. I'm really interested
in your thoughts about this.
>     (Sorry for spamming this patch, probably we should move this discussion elsewhere)

I see. Yeah there's no happy solution there if we need to change the doRequest() signature.
You did a great job on the message storage, I have been trying them out lately. If it keeps
all the WritableRequest signatures the same, maybe it would be OK to pass "null" to doRequest
for a MasterRequest, as ugly as that is, and just document it?

In regards to getting this patch to work, is there anything we can do to help Eugene get the
data he needs integrated into the messaging as we have it now, without ServerData being uniformly
available? I feel like you're (Maja) probably in the best position to know what the tradeoffs
are in that regard.


- Eli


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


On Aug. 30, 2012, 1:50 a.m., Eugene Koontz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6609/
> -----------------------------------------------------------
> 
> (Updated Aug. 30, 2012, 1:50 a.m.)
> 
> 
> Review request for giraph.
> 
> 
> Description
> -------
> 
> Same patch as: https://issues.apache.org/jira/secure/attachment/12543007/GIRAPH-211.patch
> 
> Current limitations:
> 
> -Not tested on other than hadoop 2.0.1-SNAPSHOT
> -Needs useAuth=true switch to turn on/off (default = off(?))
> 
> How to compile and test:
> 
> mvn -Phadoop_2.0.1 clean test
> 
> Works with the following test:
> 
> $HADOOP_RUNTIME/bin/hadoop jar $GIRAPH_DIR/target/giraph-0.2-SNAPSHOT-for-hadoop-2.0.1-SNAPSHOT-jar-with-dependencies.jar
org.apache.giraph.benchmark.PageRankBenchmark -Dgiraph.useNetty=true -e 2 -s 10 -v -V 2 -w
2
> 
> 
> This addresses bug GIRAPH-211.
>     https://issues.apache.org/jira/browse/GIRAPH-211
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/giraph/comm/BasicRPCCommunications.java bc53b83 
>   src/main/java/org/apache/giraph/comm/ServerData.java 68a2044 
>   src/main/java/org/apache/giraph/comm/WorkerClient.java c3ec4fe 
>   src/main/java/org/apache/giraph/comm/netty/NettyClient.java 428b877 
>   src/main/java/org/apache/giraph/comm/netty/NettyServer.java 5904908 
>   src/main/java/org/apache/giraph/comm/netty/NettyWorkerClient.java 72e79a9 
>   src/main/java/org/apache/giraph/comm/netty/NettyWorkerClientServer.java b06e0f6 
>   src/main/java/org/apache/giraph/comm/netty/SaslNettyClient.java PRE-CREATION 
>   src/main/java/org/apache/giraph/comm/netty/SaslNettyServer.java PRE-CREATION 
>   src/main/java/org/apache/giraph/comm/netty/handler/MasterRequestServerHandler.java
719515d 
>   src/main/java/org/apache/giraph/comm/netty/handler/RequestServerHandler.java 6f0fc20

>   src/main/java/org/apache/giraph/comm/netty/handler/ResponseClientHandler.java c1c2574

>   src/main/java/org/apache/giraph/comm/netty/handler/WorkerRequestServerHandler.java
e8b6c54 
>   src/main/java/org/apache/giraph/comm/requests/NullReply.java PRE-CREATION 
>   src/main/java/org/apache/giraph/comm/requests/RequestType.java 4fbf692 
>   src/main/java/org/apache/giraph/comm/requests/ResponseEncoder.java PRE-CREATION 
>   src/main/java/org/apache/giraph/comm/requests/SaslComplete.java PRE-CREATION 
>   src/main/java/org/apache/giraph/comm/requests/SaslTokenMessage.java PRE-CREATION 
>   src/main/java/org/apache/giraph/comm/requests/SendPartitionCurrentMessagesRequest.java
7cae1e2 
>   src/main/java/org/apache/giraph/comm/requests/SendPartitionMessagesRequest.java 6c7ebb6

>   src/main/java/org/apache/giraph/comm/requests/SendPartitionMutationsRequest.java d3553c1

>   src/main/java/org/apache/giraph/comm/requests/SendVertexRequest.java 2004db4 
>   src/main/java/org/apache/giraph/comm/requests/WritableRequest.java a68b26a 
> 
> Diff: https://reviews.apache.org/r/6609/diff/
> 
> 
> Testing
> -------
> 
> Tested only on Hadoop 2.0.1-SNAPSHOT : needs testing on other Hadoops.
> 
> 
> Thanks,
> 
> Eugene Koontz
> 
>


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