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