[ https://issues.apache.org/jira/browse/YARN-8448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16582321#comment-16582321 ]
Szilard Nemeth commented on YARN-8448: -------------------------------------- Hey [~rkanter]! Here are my review comments: - hadoop project/pom.xml: Please use a variable for the bouncycastle version instead of using 1.59 twice. - There are unused imports in: ResourceManager, TestWebAppProxyServlet - I don't really see any usage of the introduced method {{ org.apache.hadoop.security.ssl.KeyStoreTestUtil#setAllowAllSSL(javax.net.ssl.HttpsURLConnection) }} Actually there are two definitions of {{setAllowAllSSL}}, they both public and I can't see any usage of those methods. - {{KeyStoreTestUtil}}: Throws clause for {{CertificateException}} can be removed since {{checkClientTrusted}} and {{checkServerTrusted}} never throws this exception - {{ProxyCAManager}}: LOG is unused, rmContext is unused. Did you intend to use rmContext in recover? I would assume this as there is a TODO there. - {{container-executor.c}}: The first block you added to {{create_script_paths}} uses {{exit_code = OUT_OF_MEMORY;}} if the keystore/truststore file destinations could not be created. Is this intentional? - {{container-executor.c}}: Still in {{create_script_paths}}, the code black that try to open keystore/truststore files, the exit codes seem to be bad here: {{exit_code = INVALID_ARGUMENT_NUMBER;}} Should be instead: {{COULD_NOT_CREATE_KEYSTORE_FILE or COULD_NOT_CREATE_TRUSTSTORE_FILE}} - {{org.apache.hadoop.yarn.server.resourcemanager.TestApplicationMasterLauncher#testSetupTokens: }} This method could be private. Moreover, I would extract the code setting up {{proxyCA}} with the mocked methods to a new method, for the sake of readability. - {{org.apache.hadoop.yarn.server.resourcemanager.TestApplicationMasterLauncher.MyAMLauncher#createAndSetAMRMToken: }} Type argument AMRMTokenIdentifierfor Token can be removed - {{org.apache.hadoop.yarn.server.webproxy.ProxyCA#createCACertAndKeyPair: }} I think the {{to}} Date could be a private static final field of this class as it is a fixed date. - {{org.apache.hadoop.yarn.server.webproxy.ProxyCA#createCACertAndKeyPair }}: When {{createCert}} is invoked, is it intentional that you used the same string for the issuer and the subject? - {{ProxyCA{{, in method {{createTrustManager}}: {{checkClientTrusted}} does not throw a {{CertificateException}} so you can remove it from the signature > AM HTTPS Support > ---------------- > > Key: YARN-8448 > URL: https://issues.apache.org/jira/browse/YARN-8448 > Project: Hadoop YARN > Issue Type: Sub-task > Reporter: Robert Kanter > Assignee: Robert Kanter > Priority: Major > Attachments: YARN-8448.001.patch, YARN-8448.002.patch, > YARN-8448.003.patch, YARN-8448.004.patch, YARN-8448.005.patch > > -- This message was sent by Atlassian JIRA (v7.6.3#76005) --------------------------------------------------------------------- To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org