[
https://issues.apache.org/jira/browse/HADOOP-10075?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15556658#comment-15556658
]
Daniel Templeton commented on HADOOP-10075:
-------------------------------------------
Damn, that's a patch. Thanks for pushing that boulder up the mountain. Let's
get it reviewed before it rolls back down again.
Taking a first pass at reviewing. Here are some initial comments:
* {{RequestLoggerFilter}} is an example, so it would be nice to add some
javadoc with a {{@deprecated}} tag to explain why and what should be used
instead. I think that's generally true for all the classes you deprecated.
* It would be good to log something here: {code} // Jetty doesn't like the
same path spec mapping to different servlets, so
// if there's already a mapping for this pathSpec, remove it and assume that
// the newest one is the one we want
final ServletMapping[] servletMappings =
webAppContext.getServletHandler().getServletMappings();
for (int i = 0; i < servletMappings.length; i++) {
if (servletMappings[i].containsPathSpec(pathSpec)) {
ServletMapping[] newServletMappings =
ArrayUtil.removeFromArray(servletMappings, servletMappings[i]);
webAppContext.getServletHandler()
.setServletMappings(newServletMappings);
break;
}
}{code}
* Can you get away with a diamond operator here?{code} for
(Map.Entry<ServletContextHandler, Boolean> e
: defaultContexts.entrySet()) {
if (e.getValue()) {
...
}
}{code}
* You have a few casts, like this: {code} ServerConnector c =
(ServerConnector) webServer.getConnectors()[index];{code} I don't think you're
supposed to have the space between the parens around the type and the target.
* I'm not a fan of ternary operators, but this patch is big enough that I'm
willing to let it slide this time. :)
* Is it safe to remove class without letting them sit around deprecated for a
release?
* You took out the space before the semicolon everywhere else, so you shouldn't
add it here: {code} @Produces({MediaType.APPLICATION_JSON + ";
charset=utf-8"}){code}
* Please add space around the operators here:{code}
http_config.setRequestHeaderSize(1024*64);
http_config.setResponseHeaderSize(1024*64);{code}
* What's the story here? {code}- @Test(timeout = 5000)
+ @Test(timeout = 10000){code} Extending timeouts isn't usually the right
answer...
* I think the spacing could be better here:{code} Log.getLog().warn(
"Job end notification couldn't parse configured proxy's port "
+ portConf + ". Not going to use a proxy");{code}
* Might it be worthwhile defining {{";charset=utf-8"}} as a constant?
* Since you're messing with {code} Log.getLog().info(" == alloc " +
allocatedContainerCount
+ " it left " + iterationsLeft){code} would you mind making that
log message actually make sense?
* At great risk to my personal safety, I will suggest that it would be nice to
add messages to the asserts you're touching in the test classes, e.g.{code}
assertEquals(MediaType.APPLICATION_XML_TYPE + "; charset=utf-8",
response.getType().toString());{code}
* You should probably be more specific here:{code} // Once
${hbase-compatible-hadoop.version} is changed to Hadoop 3,
// we should be able to get rid of this.{code} Get rid of what and
how?
* {{TimelineReaderServer.setupOptions()}} should have a javadoc header.
> Update jetty dependency to version 9
> ------------------------------------
>
> Key: HADOOP-10075
> URL: https://issues.apache.org/jira/browse/HADOOP-10075
> Project: Hadoop Common
> Issue Type: Improvement
> Affects Versions: 2.2.0, 2.6.0
> Reporter: Robert Rati
> Assignee: Robert Kanter
> Attachments: HADOOP-10075-002-wip.patch, HADOOP-10075.003.patch,
> HADOOP-10075.004.patch, HADOOP-10075.005.patch, HADOOP-10075.006.patch,
> HADOOP-10075.007.patch, HADOOP-10075.patch
>
>
> Jetty6 is no longer maintained. Update the dependency to jetty9.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
