> 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? > >
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) - Maja ----------------------------------------------------------- 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 > >
