[
https://issues.apache.org/jira/browse/HADOOP-13597?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15737413#comment-15737413
]
Xiao Chen commented on HADOOP-13597:
------------------------------------
Thanks John for revving and others for reviewing.
My comments below:
- {{HttpServer2}}: Don't understand the {{skipSecretProvider}}. Could you
explain and add some comments/javadocs? Is this about to the
AuthenticationFilter and it's related secret providers startup tricks?
- {{HttpServer2}}: Possible to have {{createHttpsChannelConnector}} call
{{createHttpChannelConnector}} first, to reduce some duplicate codes? The
{{httpConfig.setSecureScheme(HTTPS_SCHEME);}} line seems reasonable to be
inside the method too.
- I found the {{AccessLoggingConfiguration}} class naming confusing. Looking at
the class javadoc didn't help much either - I only figured out until looking at
the code usage. Can't find a good replacement in my vocabulary (appreciate if
anyone else has better naming), but we should state in javadoc: 1 this a
configuration object that logs each access. 2. it redacts sensitive
information. Actually, maybe this is better to be a composites a
{{Configuration}} rather than inherits? At least whoever uses it later don't
have to figure out which method is an Override etc. (BTW, currently missing
{{@Override}} annotations on all methods, and {{set}} seems to be missing a
{{super.set}}.)
- {{KMSWebServer}}: Nit - I think hadoop code mostly have static import on 1
line and ignore the 80-char rules.
- {{KMSWebServer}}: Totally theoretical, it may be good to also have the
{{isAlive}} method, and probably add a {{waitActive}}-ish method in the
MiniKMS, so interested tests can call that and reduce flaky tests due to start
up race.
- Searching for {{tomcat}}, see several nitty references still:
{{AuthenticationFiler}}'s var name {{isInitializedByTomcat}},
{{CommandsManual.md}} and {{SecureMode.md}}, KMS's doc {{index.md.vm}}, and
some code comments etc.
- For the passwords, agree with Robert on supportability. However I also see
similar code in {{DFSUtil}} ({{loadSslConfToHttpServerBuilder}} and
{{getPassword}}). Was these copied over? We should at least move that to a
common util, and avoid this level of duplication. This will probably leave us
not having to change {{Configuration}}, but adding a wrapper util. Or per
Wei-Chiu's suggestion, maybe not needed any more. Appreciate more javadocs here
too, as to why such method is needed.
- Didn't see answer to Allen's ask about unit tests. (Take a look at
hadoop-common-project/hadoop-common/src/test/scripts if you're wondering how
that's done).
- Nit: {{kms-site.xml}} is following other -site.xmls, to have a comment line
"put site-specific...", which is good. Please follow them closer to have this
line before the {{<configuration>}} element. :)
> Switch KMS from Tomcat to Jetty
> -------------------------------
>
> Key: HADOOP-13597
> URL: https://issues.apache.org/jira/browse/HADOOP-13597
> Project: Hadoop Common
> Issue Type: New Feature
> Components: kms
> Affects Versions: 2.6.0
> Reporter: John Zhuge
> Assignee: John Zhuge
> Attachments: HADOOP-13597.001.patch, HADOOP-13597.002.patch,
> HADOOP-13597.003.patch
>
>
> The Tomcat 6 we are using will reach EOL at the end of 2017. While there are
> other good options, I would propose switching to {{Jetty 9}} for the
> following reasons:
> * Easier migration. Both Tomcat and Jetty are based on {{Servlet
> Containers}}, so we don't have change client code that much. It would require
> more work to switch to {{JAX-RS}}.
> * Well established.
> * Good performance and scalability.
> Other alternatives:
> * Jersey + Grizzly
> * Tomcat 8
> Your opinions will be greatly appreciated.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
