[jira] [Comment Edited] (GIRAPH-211) Add secure authentication to Netty IPC

["Avery Ching (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/GIRAPH-211?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13459279#comment-13459279 ] 

Avery Ching edited comment on GIRAPH-211 at 9/20/12 12:31 PM:
--------------------------------------------------------------

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

{code}
/** Whether to use SASL with DIGEST and Hadoop Job Tokens to authenticate	
 * and authorize Netty BSP Clients to Servers. */
{code}

to 

{code}
/** 
 * Whether to use SASL with DIGEST and Hadoop Job Tokens to authenticate	
 * and authorize Netty BSP Clients to Servers. 
 */
{code}

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!

                
      was (Author: aching):
    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!

                  
> Add secure authentication to Netty IPC
> --------------------------------------
>
>                 Key: GIRAPH-211
>                 URL: https://issues.apache.org/jira/browse/GIRAPH-211
>             Project: Giraph
>          Issue Type: Improvement
>            Reporter: Eugene Koontz
>            Assignee: Eugene Koontz
>             Fix For: 0.2.0
>
>         Attachments: GIRAPH-211.patch, GIRAPH-211.patch, GIRAPH-211.patch, GIRAPH-211.patch, GIRAPH-211.patch, GIRAPH-211.patch, GIRAPH-211.patch, GIRAPH-211-proposal.txt
>
>
> Gianmarco De Francisci Morales asked on the user list:
> bq. I am getting the exception in the subject when running my giraph program
> bq. on a cluster with Kerberos authentication.
> This leads to the idea of having Kerberos authentication supported within GIRAPH. Hopefully it would use our fast GIRAPH-37 IPC, but could also interoperate with Hadoop security.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira