This patch brings the source from 6->9. There is a different jira (https://issues.apache.org/jira/browse/HADOOP-9650) for updating to jetty8. If that patch is accepted, this will need to be updated to address moving from 8->9.

This patch uses glassfish in combination with jetty9 for jsp compilation.

be sure to hit "submit patch" so that you will get a jenkins run on this.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12610564/HADOOP-10075.patch
against trunk revision .

-1 patch. The patch command could not apply the patch.

Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3303//console

This message is automatically generated.

Thanks for looking at this. I think you will need to re-generate the patch, since it failed to apply on jenkins.

--- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/http/TestSSLHttpServer.java
+++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/http/TestSSLHttpServer.java
@@ -76,6 +76,7 @@ public void setup() throws Exception {
 
     conf.setInt(HttpServer.HTTP_MAX_THREADS, 10);
     conf.addResource(CONFIG_SITE_XML);
+    conf.addResource(conf.get("hadoop.ssl.server.conf","ssl-server.xml"));
     server = createServer("test", conf);
     server.addServlet("echo", "/echo", TestHttpServer.EchoServlet.class);
     server.start();

Why do we need this addition?

-        InetAddress.getByName(server.getConnectors()[0].getHost());
-      int port = server.getConnectors()[0].getPort();
+        InetAddress.getByName(((ServerConnector)server.getConnectors()[0]).getHost());
+      int port = ((ServerConnector)server.getConnectors()[0]).getPort();

I see a lot of new typecasts like this. Is it possible to avoid these? If not, could we have an accessor function that makes this easier to read? Thanks.

The patch applies cleanly against branch-2.2.0. Which branch should I target? branch-2.2? The nature of these changes will probably result in the patch failing to apply pretty consistently as development continues until/less it makes it into a source mainline.

conf.addResource(conf.get("hadoop.ssl.server.conf","ssl-server.xml"));
was needed for tests to succeed on Fedora. Jetty 8 revamped the ssl configuration and this change was part of that for the test to pass.

Jetty 9 completely revamped Connector and related classes and casts such as:
(ServerConnector)server.getConnectors()[0])
was the only way I could see to get at the needed data. If there's a cleaner way I'm all for it.

Please target trunk, as we do for (almost) all upstream patches. Then we will backport it into stable branches as is appropriate.

Jetty 8 revamped the ssl configuration and [conf.addResource] was part of that for the test to pass.

OK.

Can you be a little more clear about why the typecasts are needed? What other kinds of connectors might be returned by server#getConnectors? Have you considered making a utility routine to do this typecast?

Note that in general, nothing can be committed without a clean jenkins run. There's more information here: http://wiki.apache.org/hadoop/HowToContribute

Jetty 9 redid the Connector interface and set of classes. The Connector interface no longer contains any network related methods and those are now in the NetworkConnector interface, which extends the Connector interface.

server#getConnectors still returns an array of Connectors, but obviously those Connectors no longer have any networking data so I needed to cast to something that does. It might be better to cast to NetworkConnector instead of ServerConnector though.

I have no problem creating a utility routine if that would make things more readable. I'd prefer to batch any requested changes into a single update if possible. Any other concerns?

it looks fine aside from the things I mentioned.

Another thing: will this break HBase, which is currently using some of our jetty stuff?

Hi stack, could you invite some guys to double-check Colin P. McCabe's last comments? I'd like to help when i read but i am not familiar this area

Yes, HBase will need to change to jetty 9 as well. I'm working on HBase and will be providing a patch to them when done. The change needed for HBase to interact with Hadoop using jetty9 is pretty minor from the work I've done so far. Iiirc, it's a returned variable type change from a call into hadoop (Connector class issue again).

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12610564/HADOOP-10075.patch
against trunk revision .

-1 patch. The patch command could not apply the patch.

Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3828//console

This message is automatically generated.

Robert Rati, do you have a hbase jira/patch available? Thanks... Demai

I am running some unit tests with the attached patch applied and started seeing errors in many httpserver related UTs:
java.lang.SecurityException: Signers of 'javax.servlet.HttpConstraintElement' do not match signers of other classes in package

After find going through maven dependency tree, I found that both javax.servlet:servlet-api and org.eclipse.jetty.orbit:javax.servlet (through jetty-server) are included on the classpath. Wondering if I am missing something or the patch has not been tested with Hadoop unit tests yet.

maven dependency output:

[INFO] +- javax.servlet:servlet-api:jar:3.0-alpha-1:compile
[INFO] | +- org.eclipse.jetty.orbit:javax.servlet:jar:3.0.0.v201112011016:compile
[INFO] +- org.eclipse.jetty:jetty-servlet:jar:9.0.4.v20130625:compile
[INFO] | - (org.eclipse.jetty:jetty-servlet:jar:9.0.4.v20130625:compile - omitted for duplicate)
[INFO] +- org.glassfish.web:javax.servlet.jsp:jar:2.2.6:compile
[INFO] | - javax.servlet.jsp:javax.servlet.jsp-api:jar:2.2.1:compile
[INFO] +- org.apache.tomcat:tomcat-servlet-api:jar:7.0.37:compile
[INFO] | +- (javax.servlet:servlet-api:jar:3.0-alpha-1:provided - omitted for duplicate)
[INFO] | | +- org.eclipse.jetty.orbit:javax.servlet:jar:3.0.0.v201112011016:provided
[INFO] | +- org.eclipse.jetty:jetty-servlet:jar:9.0.4.v20130625:provided
[INFO] | | - (org.eclipse.jetty:jetty-servlet:jar:9.0.4.v20130625:provided - omitted for duplicate)
[INFO] | +- org.glassfish.web:javax.servlet.jsp:jar:2.2.6:provided
[INFO] | | - javax.servlet.jsp:javax.servlet.jsp-api:jar:2.2.1:provided
[INFO] | +- org.apache.tomcat:tomcat-servlet-api:jar:7.0.37:provided
[INFO] +- javax.servlet:servlet-api:jar:3.0-alpha-1:provided

IMO we'd be better off moving out of Jetty and into jersey as the server; this would eliminate jersey version problems altogether, and more importantly, jersey "quirks"

Hi folks! What's the status of this patch? It doesn't seem to apply on trunk. The desire to move to jersey shouldn't have blocked this patch. We can port over the server code to jersey even if we started using Jetty-9.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12610564/HADOOP-10075.patch
against trunk revision 9850e15.

-1 patch. The patch command could not apply the patch.

Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5525//console

This message is automatically generated.

Cancelling a patch since the patch looks stale. I think it's good timing to upgrade the version of jetty because http/2 is supported on jetty 9.3.0: https://projects.eclipse.org/projects/rt.jetty/reviews/9.3.0-release-review

Uploading WIP patch for pom.xml of trunk. A previous patch includes changes against HttpServer, not against HttpServer2, so we need to change the point.

Robert Rati, could you help us to create a patch for trunk?

I'm going to pick this up and get it moving forward again, unless anyone has any objections?

Jetty 6 was EoL in 2010 (6 years ago!), so we should really do this. Hadoop 3 is a great opportunity to finally get this done. I agree with Ravi Prakash: even if we later move to Jersey, that doesn't need to block upgrading Jetty.

I've started looking into the code, and I think it will be doable, just a little tedious to refactor things like HttpServer2 to the Jetty 9 APIs.

Robert Kanter I think it's okay to upgrade at server side, but I don't know we can do so at client-side. Please check HADOOP-13070.

Robert Kanter I looked over the result of grep -r jetty --include="*.java": it's used only in server-side excluding tests, so we can upgrade jetty with minimum pain on branch-3

I think it may get picked up downstream by things, which is why I was arguing for a move to Jersey instead. That way hadoop doesn't need to worry about downstream jetty use and version problems —only jersey ones

Isn't that one of the benefits of a new major version? With Hadoop 3, we should be (finally) able to upgrade Jetty, right? If a downstream component has a conflict here, then they can continue using Hadoop 2.x until they're ready to upgrade from Jetty 6 to 9 as well. Even if we replace Jetty with Jersey, that could still interfere with a downstream component using a different version of Jersey.

IMO we'd be better off moving out of Jetty and into jersey as the server; this would eliminate jersey version problems altogether, and more importantly, jersey "quirks"

For those of us less familiar with Jersey, could you please elaborate on this Steve? Or did you mean "this would eliminate jetty version problems altogether" ? Or does Jersey promise never to change its API ever?

In any case we can always make that happen later, so we shouldn't block the upgrade of an old and crufty Jetty if someone wants to do it.

Jersey has been great to work with: the first embedded servlet engine. It did have problems, ones we had got familiar with (DNs used to have to probe on startup to see if jersey was actually serving requests, if it hadn't started properly they'd terminate). Things like that kept us on an old jetty version for a long time.

Posted a 003 patch. Here's some notes on it:

1. The bulk of the changes are in HttpServer2 and pom files
2. There's a lot of misc changes in other files all over the place because package names and class names/usage were changed slightly.
3. I verified that https works.
4. I had a lot of trouble with the gzipped javascript files we have. I tried a lot of different configurations and looking around online. From what I can tell by looking at the Jetty code and using a debugger, it looks like Jetty won't serve .gz files unless the non-gz version is there. They have checks that the non-gz file exists before checking for the .gz file:
   https://github.com/eclipse/jetty.project/blob/jetty-9.3.x/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceCache.java#L235
   https://github.com/eclipse/jetty.project/blob/jetty-9.3.x/jetty-server/src/main/java/org/eclipse/jetty/server/ResourceContentFactory.java#L63
   Unfortunately, this means having both the gz and non-gz versions of the files (which is one of the reasons why the patch is so big). I did verify that if the request says it accepts gzip, you'll get the smaller gzipped version of the file:

   >> curl 'http://localhost:8088/static/jquery/jquery-1.8.2.min.js' --compressed -s -D - -o /dev/null
   HTTP/1.1 200 OK
   Date: Wed, 14 Sep 2016 20:46:50 GMT
   Vary: Accept-Encoding
   Last-Modified: Wed, 14 Sep 2016 18:58:48 GMT
   Content-Type: application/javascript
   Content-Encoding: gzip
   Accept-Ranges: bytes
   Content-Length: 33430
   Server: Jetty(9.3.11.v20160721)
   

   >> curl 'http://localhost:8088/static/jquery/jquery-1.8.2.min.js' -s -D - -o /dev/null
   HTTP/1.1 200 OK
   Date: Wed, 14 Sep 2016 20:46:51 GMT
   Vary: Accept-Encoding
   Last-Modified: Wed, 14 Sep 2016 18:58:48 GMT
   Content-Type: application/javascript
   Accept-Ranges: bytes
   Content-Length: 93436
   Server: Jetty(9.3.11.v20160721)
   

   If anyone has any better ideas on how to get gz to work without the extra files, let me know and I'll change it. That said, we're probably supposed to have both the gz and non-gz files so that we can serve the non-gz files if the browser doesn't support gz; otherwise, you get 404s instead. So perhaps this is the correct thing to do anyway, and we had it wrong before.

My suggestion would be jerzey

actually, there's one more workaround, have the .gz. unzipped somewhere on installation. I don't like having to have both unzip and gzip, as it will only lead to version inconsistencies over time

-1 overall

This message was automatically generated.

The 004 patch does the following:

• Fixed unit tests
• Fixed deprecated warnings
• Removed some missed Jetty 6 usage
• Removed js.gz and css.gz files, so only the original files are in the source codebase. A maven plugin is used to gzip these files into the target directories during build time. We now do this to all js and {{css} files, unlike before where we had some that were gzipped and some that were not.

Note that I used --binary when generating the patch because it deletes some gzip files.

-1 overall

This message was automatically generated.

The 005 patch fixes a bunch of unit tests.

There's an issue I need to still figure out with the tests in the hadoop-yarn-server-timelineservice-hbase-tests module. We're mixing Hadoop 2.5.1 and Hadoop 3 (trunk) in there (oy!) and it should be using Jetty 6, but something is causing it to try to use Jetty 9:

java.lang.NoClassDefFoundError: org/eclipse/jetty/servlet/ServletContextHandler

	at org.apache.hadoop.yarn.server.timelineservice.reader.TestTimelineReaderWebServicesHBaseStorage.init(TestTimelineReaderWebServicesHBaseStorage.java:372)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:483)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:24)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:160)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:117)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:42)
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:262)
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:84)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:483)
	at com.intellij.rt.execution.application.AppMain.main(AppMain.java:147)
Caused by: java.lang.ClassNotFoundException: org.eclipse.jetty.servlet.ServletContextHandler
	at java.net.URLClassLoader$1.run(URLClassLoader.java:372)
	at java.net.URLClassLoader$1.run(URLClassLoader.java:361)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.net.URLClassLoader.findClass(URLClassLoader.java:360)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:308)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	... 31 more

-1 overall

This message was automatically generated.

The 006 patch fixes some checkstyle and unit tests, including the hadoop-yarn-server-timelineservice-hbase-tests module. The problem is that we have ATS classes from Hadoop 3, mixed with Hadoop 2.5.1 for everything else. This means that the ATS is running with HttpServer2 from Hadoop 2.5.1, with Jetty 6, not Jetty 9. To deal with this, I made two changes:

1. Added some Jetty 9 dependencies to the module as there is some ATS code that directly uses it (e.g. logging)
2. Moved a single HttpServer2 call in an ATS class into it's own method so that a test could overwrite it to do nothing during the test. Otherwise, you get a MethodNotFoundException. This doesn't affect the normal operation of the class, just during this test.

-1 overall

This message was automatically generated.

Thanks for all your work Robert.

• Why did you have to deprecate RequestLoggerFilter.setStatus ?
• Is there precedent for trusting external plugins in Maven? Even though I'm sure com.github.phuonghuynh is a perfectly trustworthy repo, is there something else we can do? I would almost rather keep the 2 files (compressed and uncompressed)

Thanks for looking at the patch; I know it's a big one

• I had to deprecate RequestLoggerFilter.setStatus because it calls super.setStatus, which is deprecated. There is a one argument setStatus, which is not. See https://docs.oracle.com/javaee/7/api/javax/servlet/http/HttpServletResponseWrapper.html#setStatus-int-java.lang.String-
• I couldn't find anything about the safety/security of maven plugins. It is coming from the maven central repo, so it's probably safe as long as the author of the plugin doesn't upload something malicious there (I don't know if the repo does any checking on that sort of thing). I guess we have these options:
  1. Use this plugin
  2. Keep both compressed and uncompressed files
  3. Keep only uncompressed files
  4. Write our own plugin to do the same thing. It looks like we already have a "hadoop-maven-plugins" maven plugin that does a number of different things. We could add it there. I took a quick look and writing a simple plugin like this doesn't look too complicated.

The 007 patch:

• No longer uses the com.github.phuonghuynh plugin. I've written one in hadoop-maven-plugins that does something similar instead.

-1 overall

This message was automatically generated.

wow, this is getting a big patch. Bet you wish you hadn't volunteered for it now ...

Our baby has grown from 100KB to 1.27MB in three years. Thanks for volunteering to take care of our problem child, Robert Kanter.

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:

      // 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;
        }
      }

• Can you get away with a diamond operator here?

      for (Map.Entry<ServletContextHandler, Boolean> e
          : defaultContexts.entrySet()) {
         if (e.getValue()) {
           ...
         }
      }

• You have a few casts, like this:

      ServerConnector c = (ServerConnector) webServer.getConnectors()[index];

  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:

    @Produces({MediaType.APPLICATION_JSON + "; charset=utf-8"})

• Please add space around the operators here:

        http_config.setRequestHeaderSize(1024*64);
        http_config.setResponseHeaderSize(1024*64);

• What's the story here?

  -  @Test(timeout = 5000)
  +  @Test(timeout = 10000)

  Extending timeouts isn't usually the right answer...

• I think the spacing could be better here:

          Log.getLog().warn(
              "Job end notification couldn't parse configured proxy's port "
             + portConf + ". Not going to use a proxy");

• Might it be worthwhile defining ";charset=utf-8" as a constant?
• Since you're messing with

          Log.getLog().info(" == alloc " + allocatedContainerCount
              + " it left " + iterationsLeft)

  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.

      assertEquals(MediaType.APPLICATION_XML_TYPE + "; charset=utf-8",
          response.getType().toString());

• You should probably be more specific here:

            // Once ${hbase-compatible-hadoop.version} is changed to Hadoop 3,
            // we should be able to get rid of this.

  Get rid of what and how?

• TimelineReaderServer.setupOptions() should have a javadoc header.

I'm not a fan of ternary operators, but this patch is big enough that I'm willing to let it slide this time.

I am, if you get more into the java 8 streams APIs or scala, you may appreciate them more too. The biggest risk that you get the query and the two results mixed up

Thanks for the feedback Daniel Templeton.

The 008 patch:

• Adds javadoc explaining the @deprecated
• Adds a log message to that servlet mapping thing
• Makes cast styling of (ServerConnector) consistent
• Added spaces around the * operator
• Updated log messages
• Clarified the comment about Hadoop 3
• Added javadoc in TimelineReaderServer#setupOptions
• Rebased on latest trunk

And my responses to your remaining feedback:

• I couldn't put in a diamond operator there; it didn't like that.
• The only removed class is SslSelectChannelConnectorSecure, which was marked private, so I think we're okay removing it.
• I'm not sure I understand your comment about the space and the semicolon. I know I updated the @Produces a lot, but as far as I can tell, they're all identical. Can you give me some file names and lines where they differ?
• On the test timeout increase, I had to increase it because the server now takes longer to shutdown. I did some checking, and with Jetty 9, it takes ~3000ms, but with Jetty 6, it takes only ~100ms. Due to this, I had to increase the timeout. I'm not sure there's much we can do about this; it's mostly Jetty code and I didn't change much in the stop method.
• I looks like the way to get the utf-8 charset as a constant String is via StandardCharsets.UTF_8.name(). Any suggestions on an easy way to do that? There's over 600 instances of this in the patch.
• I agree that it's generally good to put messages into assert statements, but I don't think that's necessary when it's obvious what it's checking and what is wrong with it. We're clearly checking the response type here, and when it's incorrect, you get an obvious message like this:

  org.junit.ComparisonFailure: 
  Expected :application/json; charset=foo
  Actual   :application/json; charset=utf-8
  

  Adding a message like "Expected response type X but got response type Y" won't be any more helpful than this.

-1 overall

This message was automatically generated.

Robert Kanter - mind throwing this on RB or Github PR for easier review?

Thanks a lot for the massive amount of effort Robert and others.
The patch is deceptively big. A lot of the bulk is added because of renaming classes or adding charset or just the logging method. Thanks also for writing the maven plugin Robert.

Is there a reason you changed httpServer.addContext(uiWebAppContext, true); to httpServer.addHandlerAtFront(uiWebAppContext); in ApplicationHistoryServer?

The main changes are in HttpServer2 and I am going through them right now. Will hopefully get done soon.

Is there a reason you changed httpServer.addContext(uiWebAppContext, true); to httpServer.addHandlerAtFront(uiWebAppContext); in ApplicationHistoryServer?

The order of the handlers is important when Jetty is trying to figure out which Servlet to use. To get the AHS to work correctly there, I had to make sure that uiWebAppContext was first in the list, even though we add it later.

Thanks for taking a look Ravi, I know it's a discouraging patch to look at
I'm currently working on making the charset changes a constant (I had a good idea to make this a little easier), and I'll post an updated patch. I'll also post it on RB to make it easier to comment on; I had trouble finding some of the things Daniel was talking about in his feedback.

The 009 patch:

• Adds a constant for the "charset=utf-8" string. It doesn't do this in TestTimelineReaderWebServicesHBaseStorage because that module uses Hadoop Common 2.5, which doesn't have this new constant.

I also wasn't able to upload the patch to ReviewBoard.

-1 overall

This message was automatically generated.

Karthik Kambatla

I've put the 009 patch (after a rebase on the latest trunk) on github if that's easier to review:
https://github.com/rkanter/hadoop/commit/645818889bf756efadfe6b7392faeeb1af3db383

Thanks Robert!

• Why did you have to add ep.getPort() == -2 in HttpServer2.Builder.build()? The javadoc for URI never claims to return a -2. Is it bleeding in from ServerConnector.getLocalPort() somehow?
• HttpServer2.createDefaultChannelConnector() has started taking a server as an argument. Is that really necessary? Or are we always asking to pass in an instance of Server to this method that we already have access to?

My notes (and not necessarily questions, but if you have more info, it'd be great to document here):

• http://www.eclipse.org/jetty/documentation/current/architecture.html#basic-architecture and http://www.eclipse.org/jetty/documentation/current/embedding-jetty.html are good pages to read up.

•       conn.addFirstConnectionFactory(new SslConnectionFactory(sslContextFactory,
            HttpVersion.HTTP_1_1.asString()));
  

  http://archive.eclipse.org/jetty/9.0.0.M3/apidocs/org/eclipse/jetty/server/ssl/SslSelectChannelConnector.html suggests to use SelectChannelConnector . Couldn't find any documentation on how to do that.

• c.setLowResourceMaxIdleTime(10000); has been changed to c.setIdleTimeout(10000); This just means that timeout will occur after 10s whether or not there were more than LowResourcesConnections. I'm fine with this change.
• We haven't set c.setResolveNames(false);This thread claims that by default names will not be resolved, so I'm fine with this change. Tagging Min Shen who had put in original arguments.
• addNoCacheFilter(webAppContext); was changed to addNoCacheFilter(logContext); Was this a bug earlier?
• addInternalServlet() removes existing path bindings now. I don't see what else we could do about it. Fine by me.

Thanks Ravi Prakash for taking a look.
Here's my responses to your questions/notes:

• On your -2 port question: I was maybe a bit overzelous in updating -1 port checks to -2. I think this should only happen on ServerConnector.getLocalPort(). I'll change this to only check of -1.
• HttpServer2.createDefaultChannelConnector() is static, so it doesn't have access to the Server instance unless we pass it.
• The Javadoc says "use SelectChannelConnector with SslContextFactory". I also couldn't find anything on SelectChannelConnector, but I am using SslContextFactory with a ServerConnector. The fact that SelectChannelConnector isn't hyperlinked in the Javadoc and I can't find a class for it makes me think it's a typo or mistake. ServerConnector's Javadoc does say that it can work with SSL. I have also tested SSL and it works with the current patch. I don't remember where, but I did look at some examples when figuring out the SSL stuff, and it was done this way.
• On the addNoCacheFilter change: Yes, I believe this was a bug earlier; though I could be wrong.

Given that the -2 port change is trivial, I'll wait on more feedback before uploading a new patch version with that change.

Thanks Robert! I think I am done with my feedback. If all the tests that passed earlier, would pass with the new patch, I'm happy to +1 it.

Latest patch looks great. One more round of quibbles:

• Seems like maybe 1024 * 64 should be a constant since it appears several times. It's also still in there as 1024*64 a few times.
• In TestHttpServer, it would be better to use the constants instead of "text/plain;charset=utf-8". (That, by the way, is what I meant with you stripping the space after the semicolon. You stripped it here, but then added the space everywhere else.)
• In MiniKMS, you have

            ((ServerConnector)server.getConnectors()[0]).getHost() + ":" +
            ((ServerConnector)server.getConnectors()[0]).getLocalPort());

  I believe the convention is to have the + at the start of the next line. Same thing in JobEndNotifier.

• In ResourceGzMojo you should probably catch Exception instead of Throwable:

      } catch (Throwable t) {
        throw new MojoExecutionException(t.toString(), t);
      }

  and

          } catch (Throwable t) {
            this.throwable = t;
          }
        } catch (Throwable t) {
          this.throwable = t;
        }

The 010 patch:

• Addresses Ravi Prakash's comment about the -2 check
• Relevant CheckStyle warnings. Also, I think it's confused about indentation levels for lambda expressions.
• Replaces 1024 * 64 with a constant
• Uses constants in TestHttpServer for the types. However, for some reason, it keeps being without a space, even though I'm directly setting the type in the servlet with a space. Even stranger, if I directly set the type in the servlet to a different charset (e.g. utf-16), it does keep the space. So I'm not really sure what's going on here...
• Moved + signs as per Daniel Templeton's comments
• Rebased on latest trunk

Here's the changes in github if that's easier to look at:
https://github.com/rkanter/hadoop/commit/546a7e36a7eeacf4b68fe31a1a09b9badc6b7560

I didn't remove catching throwable in ResourceGzMojo. The other Hadoop maven plugins also do this, and I think the idea is that Maven knows how to nicely handle a MojoExecutionException and report that properly to the user.

-1 overall

This message was automatically generated.