hadoop-common-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Daniel Templeton (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HADOOP-10075) Update jetty dependency to version 9
Date Fri, 07 Oct 2016 23:51:20 GMT

    [ 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: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org


Mime
View raw message