Add jersey-servlet to dep chain

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

+1 @author. The patch does not contain any @author tags.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

-1 javac. The patch appears to cause the build to fail.

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

This message is automatically generated.

updated patch.

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

+1 @author. The patch does not contain any @author tags.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/2602//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/2602//console

This message is automatically generated.

ready for review

Hi folks,

This patch is fairly benign, any chance of getting a review?

Cheers,
Tim

this seems OK, and jersey folks have good track of not breaking things between releases. Still same comment that I've just posted in HADOOP-9650.

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

+1 @author. The patch does not contain any @author tags.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3246//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3246//console

This message is automatically generated.

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

+1 @author. The patch does not contain any @author tags.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/3601//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/3601//console

This message is automatically generated.

marking as a branch-3 "java7+" issue

depends on the HADOOP-10530 switch to Java 7 in trunk

resubmitting, though I think jersey version have moved on more.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12610134/HADOOP-2.2.0-9613.patch
against trunk revision 6f9fe76.

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

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

This message is automatically generated.

Are you aware of any effort to move to Jersey 2.x ?

-1 overall

This message was automatically generated.

Now that we are on java 7, this should be revisited

JERSEY-2429 implies that Jersey 1.18+ is going to be needed for full java 8 support (and asm 5)

ravi : no, no jersey 2.x plans. it's mostly been left alone

Timothy St. Clair let me take over this issue, but please feel free to assign this back to you.

Attaching a patch for trunk. This patch includes the upgrade of grizzly's version since the upgrade of jersey's version causes dependency convergence error about grizzly-http-server and grizzly-http.

Steve Loughran could you take a look?

-1 overall

This message was automatically generated.

The patch only includes fixes against pom.xml, so javac warnings are not related and we don't need to add tests.

I'm thinking the javac warnings (deprecation) are related to the patch. ClientResponse#getClientResponseStatus has been deprecated since 1.18. https://jersey.java.net/nonav/apidocs/1.18/jersey/com/sun/jersey/api/client/ClientResponse.html

Akira Ajisaka oh, you're right. That's my mistake. Attached an updated patch to fix the javac warnings.

Copied from https://builds.apache.org/job/PreCommit-HADOOP-Build/6737/artifact/patchprocess/commentfile

-1 overall

This message was automatically generated.

The test failures are related. I'll fix test failures. Please wait a moment.

Updating a patch to fix failures of tests.

-1 overall

This message was automatically generated.

Cancelling the patch for the test failure. I'm facing an interesting bug - tests fail because of mismatching of content-type after upgrading jersey. I appreciate if someone shares the knowledge about this problem.

Found the reason why the tests fail - jersey-test-framework-grizzly2 has started to use reflection via Webappcontext since 1.12. GuiceServletConfig and Injector are not visible from Webappcontext if it's not static. As a result, DI fails.

To upgrade jersey version to 1.19, we need to do following tasks:
1. Upgrading grizzly to 2.2.16.
2. Updating all test cases which uses GuiceServletContextListener.

They're very tough tasks - Steve Loughran do you have any ideas? Should we upgrade them?

I will rewrite tests to pass tests.

Rewrote tests. I found a bug about Jersey - after upgrading from 1.12 to 1.13, the root element whose content is empty collection is changed from null to empty object({}). Related to following chage: https://java.net/jira/browse/JERSEY-1168

Attached patch fixed tests and assertions about JSONObject.NULL. Please note that the later changes are incompatible.

By right, to upgrade Jersey to 1.19 itself is an incompatible change.

This is the incompatible behaviour.

• assertEquals("jobs is not null", JSONObject.NULL, json.get("jobs"));
  + assertEquals("jobs is not empty",
  + new JSONObject().toString(), json.get("jobs").toString());

Steve Loughran, could you take a look at latest patch(004)? Currently, we target this to 3.0.0. If we decide to commit this only to 3.0, we can change the behaviour. If we plan to backport this, we need a workaround to keep the compatibility.

+1 overall

This message was automatically generated.

Found a test failure by my change locally about TestTimelineClient. Fixing this.

Allen Wittenauer could you tell me how to run whole tests which includes changes?

Attached 005 patch to address the problem.

-1 overall

This message was automatically generated.

Why so many duplicate guice servlet context classes?

paste: /testptch/hadoop/patchprocess/tmp.11.10378: No such file or directory
rm: cannot remove '/testptch/hadoop/patchprocess/tmp.11.10378': No such file or directory

Something weird happened during this test run which caused test-patch to be able to determine which modules were supposed to be processed. I'll try to duplicate it and see if it was a bug in test-patch or (more likely) something external.

OK, I'll also report it when I can reproduce the problem on my local. Thanks for your help.

Steve, thank you for taking a look.

Why so many duplicate guice servlet context classes?

It's for sharing an object of com.google.inject.Injector within test classes. The test cases which extends JerseyTest are initialized in its constructor, which accepts a guice servlet context class.

  public TestAMWebServices() {
    super(new WebAppDescriptor.Builder(
        "org.apache.hadoop.mapreduce.v2.app.webapp")
        .contextListenerClass(GuiceServletConfig.class) // <-- servlet context class
        .filterClass(com.google.inject.servlet.GuiceFilter.class)
        .contextPath("jersey-guice-filter").servletPath("/").build());
  }

The guice servlet context classes are used to initialize JerseyTest and servlet containers with guice's DI:

private Injector injector = Guice.createInjector(new ServletModule() {
  @Override
  protected void configureServlets() {
       appContext = new MockAppContext(0, 1, 1, 1);
       appContext.setBlacklistedNodes(Sets.newHashSet("badnode1", "badnode2"));
       bind(JAXBContextResolver.class);
       bind(AMWebServices.class);
       bind(GenericExceptionHandler.class);
 
       serve("/*").with(GuiceContainer.class);
  }
}
 
public class GuiceServletConfig extends GuiceServletContextListener {
  @Override
  protected Injector getInjector() {
    return injector;
  }
}

The latest patch fixes test failures by changes of the initialization sequence after upgrading jersey-test-framework-grizzly2 to 1.13 or later. It happens because grizzly2 has started to use reflection since 2.2.16 and the logic to create ServletModule in Webappcontext doesn't handle formal parameter of inner class.

I thought it's better to make these inner classes static, but it happens test failures because some test changes states of servlets and the states remain still after executing tests. That's why I changed GuiceServletConfig to normal class(not inner class) and be able to initialize its module for each test cases as follows.

  static {
    GuiceServletConfig.injector = Guice.createInjector(new WebServletModule());
  }

  @Before
  @Override
  public void setUp() throws Exception {
    super.setUp();
    GuiceServletConfig.injector = Guice.createInjector(new WebServletModule());
  }

]

[ERROR] /testptch/hadoop/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-hs/src/test/java/org/apache/hadoop/mapreduce/v2/hs/webapp/TestHsWebServicesJobsQuery.java:[92,5] cannot find symbol

This problem happens when a new class, in this case GuiceServletConfig, is added on separate jar but the test jar cannot find it. Should we create new separate jira to deal with this problem?

Didn't that happen specifically because the mvn install of yarn-common failed?

Looks like a good idea. I assume you are targetting this only at trunk / 3.0 based on the "target version" and the incompatibility discussion?

Attaching a patch for fixing a failure of TestTimelineClient.

You're right.

Thank you for the comment. Hmm, let me try again.

-1 overall

This message was automatically generated.

Allen Wittenauer do you have any inspiration from following error message? It looks failing to fetch hadoop-common/hadoop-auth packages.

[ERROR] /testptch/hadoop/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/client/api/impl/TimelineClientImpl.java:[50,56] package org.apache.hadoop.security.authentication.client does not exist
[ERROR] /testptch/hadoop/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/client/api/impl/TimelineClientImpl.java:[51,56] package org.apache.hadoop.security.authentication.client does not exist
[ERROR] /testptch/hadoop/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/client/api/impl/TimelineClientImpl.java:[108,11] cannot find symbol
symbol: class ConnectionConfigurator
location: class org.apache.hadoop.yarn.client.api.impl.TimelineClientImpl
[ERROR] /testptch/hadoop/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/client/api/impl/TimelineClientImpl.java:[506,18] cannot find symbol

I'd need to dig deeper but on first glance, it looks like a build time order dependency problem/incompatibility introduced by the TimeLine code. Yetus is very good about finding these problems that the previous test-patch hid. Effectively, hadoop-yarn-client requires hadoop-auth now, but the build system doesn't know that. These sorts of problems are typically hidden on local builds (and in the old test-patch) because everyone builds at root. If you don't build at root and nuke your maven local cache, like Yetus' environment usually is, these things tend to surface. Either the timeline code needs to remove this, the pom.xml needs to explicitly state this dependency, or Yetus needs to be told to build and install hadoop-auth before hadoop-yarn-client.

Thanks a lot, your advice helps me a lot. Let me test with the change against pom.xml of yarn-common, thought this change should be addressed on another jira.

This is going to hit client-side hard; it forces all clients to upgrade their jersey code, and, as jersey's ClientResponse class is clearly not-backwards compatible across versions: https://jersey.java.net/nonav/documentation/2.0/migration.html

Which means it is going to be traumatic.. Anyone who wants to move up to Hadoop 3 is going to have to commit to upgrade jersey, invest the migration effort knowing they'll never go back, etc.

It may well be better, but its the kind of thing that puts people off upgrading -and by doing it in Hadoop, we give people downstream some transitive reasons not to upgrade.

Is there anything we can do here?

-1 overall

This message was automatically generated.

Steve, In the context of JDK8 support, we can still use Jersey 1.19, which supports JDK 8 with compatibility for client-side API. In the patch, ClientResponse.getClientResponseStatus, which is a deprecated method, is replaced with reposense.getStatusInfo().getStatusCode to remove the usages of deprecated methods, but getClientResponseStatus still remain.
https://jersey.java.net/nonav/apidocs/1.19/jersey/com/sun/jersey/api/client/ClientResponse.html

About a incompatibility of return value when JSON object is null, we need a workaround to keep compatibility.

• assertEquals("jobs is not null", JSONObject.NULL, json.get("jobs"));
  + assertEquals("jobs is not empty",
  + new JSONObject().toString(), json.get("jobs").toString());

Rebased the patch on current trunk.

I found that 008 patch is not enough to fix - replacing all response.getClientResponseStatus() with getStatusInfo().getStatusCode().

GitHub user oza opened a pull request:

https://github.com/apache/hadoop/pull/76

HADOOP-9613. [JDK8] Update jersey version to latest 1.x release.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/oza/hadoop HADOOP-9613

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/hadoop/pull/76.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #76

commit ee3f4ae41ca0af654ff2a61d478e5edeb986b06e
Author: Tsuyoshi Ozawa <ozawa@apache.org>
Date: 2016-02-15T16:02:09Z

HADOOP-9613. [JDK8] Update jersey version to latest 1.x release.

Github user oza commented on the pull request:

https://github.com/apache/hadoop/pull/76#issuecomment-184275448

This PR is for a review.

Github user oza commented on a diff in the pull request:

https://github.com/apache/hadoop/pull/76#discussion_r52916706

— Diff: hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-hs/src/test/java/org/apache/hadoop/mapreduce/v2/hs/webapp/TestHsWebServicesJobsQuery.java —
@@ -136,7 +130,8 @@ public void testJobsQueryStateNone() throws JSONException, Exception {
assertEquals(MediaType.APPLICATION_JSON_TYPE, response.getType());
JSONObject json = response.getEntity(JSONObject.class);
assertEquals("incorrect number of elements", 1, json.length());

• assertEquals("jobs is not null", JSONObject.NULL, json.get("jobs"));
  + assertEquals("jobs is not empty",
  • • End diff –

I found one incompatible change about Jersey - after upgrading from 1.12 to 1.13, the root element whose content is empty collection is changed from null to empty object({}). Related to following change: https://java.net/jira/browse/JERSEY-1168

This change of lines fixes tests and assertions about JSONObject.NULL to address the change above.

Found that TestNMWebServices has not been fixed yet. v010 patch addresses it.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

-1 overall

This message was automatically generated.

Updated tests which are added recently.

there's a lot of test asserts of the form

assertEquals(Status.NOT_FOUND.getStatusCode(), response.getStatusInfo().getStatusCode()))

Why not have some assertResponseStatusCode(expected, response) check