[
https://issues.apache.org/jira/browse/YARN-8448?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16582321#comment-16582321
]
Szilard Nemeth edited comment on YARN-8448 at 8/17/18 8:16 PM:
---------------------------------------------------------------
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
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 block
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
was (Author: snemeth):
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
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: [email protected]
For additional commands, e-mail: [email protected]
