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 Mon, 03 Sep 2012 18:16:35 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!
>

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?


- 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