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