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

Wei-Chiu Chuang commented on HADOOP-10768:
------------------------------------------

Here's my review comments. Some of them are already mentioned previously. 
[~dapengsun] would you please update the patch if you have time available?

 
{quote}do you know why there's a ~50% degradation? That's concerning and may 
severely impede performance (to the point I can't use it :() 
{quote}
While that's the case, it is still 4~5x  performant than existing 
implementation, and I guess I'm still happy about that. The amount of memory 
allocation is concerning. I'll try to find time to profile it today.

 
 * CryptoInputStream
 the new readFully() method is a public method but lacks javadoc.
 is it different from the existing readFully() methods? Can you reuse the 
existing readFully() method?
 * Client
 Why are we catching exceptions in Client.shouldAuthenticateOverKrb()? That 
seems unnecessary. If not catching exception causes a problem, please have a 
test case for it.
 * SaslUtil
 ** negotiateCipherOption()
 can you please throw a non IOException? I’m not in favor of making every 
method throwing a generic IOExceptions. Similarly, update the method signature 
for the code path (getCipherOption, processSaslToken)
 This method is very similar to DataTransferSaslUtil#negotiateCipherOption() 
except for the configuration keys.
 I see no reason to duplicate the code, especially it involves some 
coding/decoding, which is not that easy to comprehend.
 ** Also, every time this method is called, it returns a new List<>. I feel 
like this is too much of a cost. Can we reduce the memory footprint?

 * SaslRpcClient
 ** saslConnect()
 LOG.debug(
 "Get SASL RPC CipherOption from Conf" + cipherOptions);
 —> missing a space after Conf
 the method is well over 100 lines in length now. Some code refactor will 
greatly improve readability.
 ** handleSaslCipherOptions()
 throw new SaslException(e.getMessage(), e);
 the exception message should be more descriptive. It could be something like 
“Unable to initialize SaslCryptoCodec”

> Optimize Hadoop RPC encryption performance
> ------------------------------------------
>
>                 Key: HADOOP-10768
>                 URL: https://issues.apache.org/jira/browse/HADOOP-10768
>             Project: Hadoop Common
>          Issue Type: Improvement
>          Components: performance, security
>    Affects Versions: 3.0.0-alpha1
>            Reporter: Yi Liu
>            Assignee: Dapeng Sun
>            Priority: Major
>         Attachments: HADOOP-10768.001.patch, HADOOP-10768.002.patch, 
> HADOOP-10768.003.patch, HADOOP-10768.004.patch, HADOOP-10768.005.patch, 
> HADOOP-10768.006.patch, HADOOP-10768.007.patch, HADOOP-10768.008.patch, 
> HADOOP-10768.009.patch, Optimize Hadoop RPC encryption performance.pdf
>
>
> Hadoop RPC encryption is enabled by setting {{hadoop.rpc.protection}} to 
> "privacy". It utilized SASL {{GSSAPI}} and {{DIGEST-MD5}} mechanisms for 
> secure authentication and data protection. Even {{GSSAPI}} supports using 
> AES, but without AES-NI support by default, so the encryption is slow and 
> will become bottleneck.
> After discuss with [~atm], [~tucu00] and [~umamaheswararao], we can do the 
> same optimization as in HDFS-6606. Use AES-NI with more than *20x* speedup.
> On the other hand, RPC message is small, but RPC is frequent and there may be 
> lots of RPC calls in one connection, we needs to setup benchmark to see real 
> improvement and then make a trade-off. 



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org

Reply via email to