----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6609/#review12009 -----------------------------------------------------------
Thanks Eugene for continuing this. I think you may have missed my comments from the JIRA as many of them are not addressed here. Also this appears to not be rebased on trunk. Here is what I wrote earlier (some maybe not apply anymore): https://issues.apache.org/jira/browse/GIRAPH-211?focusedCommentId=13459279&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13459279 -------------------------------------- Eugene, this looks nice! I still see some checkstyle stuff but you're working through it I guess. Overall, the design is good. If we think about improvements we can make them later on. Questions/Comments: For consistency, can you please convert multi-line comments like /** Whether to use SASL with DIGEST and Hadoop Job Tokens to authenticate * and authorize Netty BSP Clients to Servers. */ to /** * Whether to use SASL with DIGEST and Hadoop Job Tokens to authenticate * and authorize Netty BSP Clients to Servers. */ Can we get rename Authorize to AuthorizeServerHandler or something else more descriptive? NettyClient.java 372: Please wrap the LOG.info() with if (LOG.isInfoEnabled()). 545-557: Can't we just go through the regular netty request part of the code? We don't need to have -2 here and can just submit the destWorkerId? SASL_COMPLETE -> SASL_COMPLETE_REQUEST? SaslTokenMessage.java can we call is SaslTokenMessageRequest? SaslComplete.java can we call it SaslCompleteRequest to match the other names? SaslComplete.java 29-34: Why not get rid of these? SaslTokenMessage.java: 86: Extra line Thanks again, this was a lot of work! src/main/java/org/apache/giraph/comm/ServerData.java <https://reviews.apache.org/r/6609/#comment25597> Please wrap all debug/info level logging i.e. "if (LOG.isDebug...". src/main/java/org/apache/giraph/comm/ServerData.java <https://reviews.apache.org/r/6609/#comment25598> if (LOG.isDebug..., src/main/java/org/apache/giraph/comm/ServerData.java <https://reviews.apache.org/r/6609/#comment25599> if (LOG.isDebug..., src/main/java/org/apache/giraph/comm/ServerData.java <https://reviews.apache.org/r/6609/#comment25600> if (LOG.isDebug..., src/main/java/org/apache/giraph/comm/ServerData.java <https://reviews.apache.org/r/6609/#comment25601> if (LOG.isDebug..., src/main/java/org/apache/giraph/comm/ServerData.java <https://reviews.apache.org/r/6609/#comment25602> if (LOG.isDebug..., src/main/java/org/apache/giraph/comm/ServerData.java <https://reviews.apache.org/r/6609/#comment25603> Is this okay? Or should we exit everything? src/main/java/org/apache/giraph/comm/ServerData.java <https://reviews.apache.org/r/6609/#comment25604> if (LOG.isDebug... src/main/java/org/apache/giraph/comm/netty/NettyClient.java <https://reviews.apache.org/r/6609/#comment25605> Is this TODO something that need to be addressed now? src/main/java/org/apache/giraph/comm/netty/NettyClient.java <https://reviews.apache.org/r/6609/#comment25612> Can't we just go through the regular netty request part of the code? We don't need to have -2 here and can just submit the destWorkerId? src/main/java/org/apache/giraph/comm/netty/handler/ResponseClientHandler.java <https://reviews.apache.org/r/6609/#comment25607> Please wrap and for consistency with rest of code, can you use "decode:" here? src/main/java/org/apache/giraph/comm/netty/handler/ResponseClientHandler.java <https://reviews.apache.org/r/6609/#comment25606> For consistency with rest of code, can you use "decode:" here? src/main/java/org/apache/giraph/comm/netty/handler/ResponseClientHandler.java <https://reviews.apache.org/r/6609/#comment25608> Can you wrap here and for consistency with rest of code, can you use "decode:" here? src/main/java/org/apache/giraph/comm/netty/handler/ResponseClientHandler.java <https://reviews.apache.org/r/6609/#comment25609> Please wrap and for consistency with rest of code, can you use "decode:" here? src/main/java/org/apache/giraph/comm/requests/RequestType.java <https://reviews.apache.org/r/6609/#comment25610> For consistency, can we name this SASL_COMPLETE_REQUEST? src/main/java/org/apache/giraph/graph/BspServiceWorker.java <https://reviews.apache.org/r/6609/#comment25611> This needs to be rebased, we don't use GiraphJob for configuration parameters anymore, see GiraphConfiguration. - Avery Ching On Sept. 27, 2012, 6:54 p.m., Eugene Koontz wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6609/ > ----------------------------------------------------------- > > (Updated Sept. 27, 2012, 6:54 p.m.) > > > Review request for giraph. > > > Description > ------- > > Current limitations: > > -Not tested on other than hadoop trunk > > How to use: > > use > > -Dgiraph.useNetty=true -Dgiraph.authenticate=true > > on your job's command line to use this feature. > > How to compile and test: > > mvn -Phadoop_trunk clean test > > Works with the following test (after applying patch to trunk): > > $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 > -Dgiraph.authenticate=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/WorkerClient.java c3ec4fe > src/main/java/org/apache/giraph/comm/netty/NettyClient.java e6a8508 > 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/Authorize.java > PRE-CREATION > src/main/java/org/apache/giraph/comm/netty/handler/ResponseEncoder.java > PRE-CREATION > src/main/java/org/apache/giraph/comm/netty/handler/SaslClientHandler.java > PRE-CREATION > src/main/java/org/apache/giraph/comm/netty/handler/SaslServerHandler.java > PRE-CREATION > src/main/java/org/apache/giraph/comm/requests/RequestType.java 4fbf692 > 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/graph/BspServiceWorker.java d926d1c > src/main/java/org/apache/giraph/graph/GiraphJob.java 342be46 > > Diff: https://reviews.apache.org/r/6609/diff/ > > > Testing > ------- > > Tested only on Hadoop Trunk : needs testing on other Hadoops. > > > Thanks, > > Eugene Koontz > >
