Bump up logback, janino, jetty #562

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
5 participants
@lalloni
Contributor

lalloni commented Feb 26, 2013

While combining an embedded neo4j-server 1.8.1 + logback 1.0.9 in our app we found incompatible dependency of both against janino's differing versions, hence we updated both logback and janino versions (taking into account the new janino's groupId), naking the dependency from logback-access 1.0.9 to jetty 7.x or 8.x, so we worked out the update of neo4j-server to use jetty 9 instead of 6.

This comment has been minimized.

Show comment Hide comment
@ghost

ghost Feb 26, 2013

Can one of the admins verify this patch?

ghost commented Feb 26, 2013

Can one of the admins verify this patch?

@peterneubauer

This comment has been minimized.

Show comment Hide comment
@peterneubauer

peterneubauer Feb 28, 2013

Contributor

@plalloni could you please send in the CLA before merging? http://docs.neo4j.org/chunked/snapshot/cla.html#_how_to_sign

Thank you!

Contributor

peterneubauer commented Feb 28, 2013

@plalloni could you please send in the CLA before merging? http://docs.neo4j.org/chunked/snapshot/cla.html#_how_to_sign

Thank you!

@lalloni

This comment has been minimized.

Show comment Hide comment
@lalloni

lalloni Feb 28, 2013

Contributor
Contributor

lalloni commented Feb 28, 2013

@peterneubauer

This comment has been minimized.

Show comment Hide comment
@peterneubauer

peterneubauer Feb 28, 2013

Contributor

@plalloni I cannot build your branch, I get

[INFO] Neo4j Server API .................................. SUCCESS [0.640s]
[INFO] Neo4j Server ...................................... FAILURE [1.473s]
[INFO] Neo4j Server Examples ............................. SKIPPED
[INFO] Neo4j Community Build ............................. SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 1:33.751s
[INFO] Finished at: Thu Feb 28 17:01:17 CET 2013
[INFO] Final Memory: 46M/123M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.0:compile (default-compile) on project neo4j-server: Compilation failure
[ERROR] /Users/peter/neo/neo4j/community/server/src/main/java/org/neo4j/server/web/Jetty6WebServer.java:[395,26] cannot access org.eclipse.jetty.server.RequestLog
[ERROR] class file for org.eclipse.jetty.server.RequestLog not found

Contributor

peterneubauer commented Feb 28, 2013

@plalloni I cannot build your branch, I get

[INFO] Neo4j Server API .................................. SUCCESS [0.640s]
[INFO] Neo4j Server ...................................... FAILURE [1.473s]
[INFO] Neo4j Server Examples ............................. SKIPPED
[INFO] Neo4j Community Build ............................. SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 1:33.751s
[INFO] Finished at: Thu Feb 28 17:01:17 CET 2013
[INFO] Final Memory: 46M/123M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.0:compile (default-compile) on project neo4j-server: Compilation failure
[ERROR] /Users/peter/neo/neo4j/community/server/src/main/java/org/neo4j/server/web/Jetty6WebServer.java:[395,26] cannot access org.eclipse.jetty.server.RequestLog
[ERROR] class file for org.eclipse.jetty.server.RequestLog not found

@peterneubauer

This comment has been minimized.

Show comment Hide comment
@peterneubauer

peterneubauer Mar 1, 2013

Contributor

Is this building ok for you @plalloni ?

Contributor

peterneubauer commented Mar 1, 2013

Is this building ok for you @plalloni ?

@lalloni

This comment has been minimized.

Show comment Hide comment
@lalloni

lalloni Mar 1, 2013

Contributor

@peterneubauer it was building ok when I tried, but let me check again and get back

Contributor

lalloni commented Mar 1, 2013

@peterneubauer it was building ok when I tried, but let me check again and get back

@lalloni

This comment has been minimized.

Show comment Hide comment
@lalloni

lalloni Mar 1, 2013

Contributor

@peterneubauer after a "mvn clean" I've got the error you posted above from "mvn compile", I'll look into it

Contributor

lalloni commented Mar 1, 2013

@peterneubauer after a "mvn clean" I've got the error you posted above from "mvn compile", I'll look into it

@lalloni

This comment has been minimized.

Show comment Hide comment
@lalloni

lalloni Mar 1, 2013

Contributor

@peterneubauer well, now I know a reason for not upgrading logback in neo4j... the last version of logback-access compatible with jetty 6.x is the one currently used by neo4j (0.9.30) after that one you have to go to jetty 7.x or 8.x

Is there any good reason for sticking to jetty 6 in neo4j-server? (other than the possible lot of work needed to implement the upgrade)

Contributor

lalloni commented Mar 1, 2013

@peterneubauer well, now I know a reason for not upgrading logback in neo4j... the last version of logback-access compatible with jetty 6.x is the one currently used by neo4j (0.9.30) after that one you have to go to jetty 7.x or 8.x

Is there any good reason for sticking to jetty 6 in neo4j-server? (other than the possible lot of work needed to implement the upgrade)

@lalloni

This comment has been minimized.

Show comment Hide comment
@lalloni

lalloni Mar 5, 2013

Contributor

@peterneubauer Now it compiles and tests successfuly "most of the time" in my WC, but I didn't commit the license files I had to "cp" from target to project's roots because I don't currently know what's expected about that.

Also, I said "most of the time" because sometimes I'm having failures while run the selenium tests from cli but they run successfuly under my IDE...

Contributor

lalloni commented Mar 5, 2013

@peterneubauer Now it compiles and tests successfuly "most of the time" in my WC, but I didn't commit the license files I had to "cp" from target to project's roots because I don't currently know what's expected about that.

Also, I said "most of the time" because sometimes I'm having failures while run the selenium tests from cli but they run successfuly under my IDE...

@jakewins

This comment has been minimized.

Show comment Hide comment
@jakewins

jakewins Mar 6, 2013

Contributor

@rickardoberg @jimwebber Any opinions on bumping Logback and Jetty versions? I'm fine bumping jetty, don't know about changes in logback.

Perhaps merge this into 2.0 instead of 1.9, since it will break people depending on older versions? Seems like a reasonable place to bump versions..

Contributor

jakewins commented Mar 6, 2013

@rickardoberg @jimwebber Any opinions on bumping Logback and Jetty versions? I'm fine bumping jetty, don't know about changes in logback.

Perhaps merge this into 2.0 instead of 1.9, since it will break people depending on older versions? Seems like a reasonable place to bump versions..

@jimwebber

This comment has been minimized.

Show comment Hide comment
@jimwebber

jimwebber Mar 6, 2013

Owner

I would love 2.0GA to run atop Jetty 9. Their new mechanically sympathetic architecture should give our server a "free" performance boost.

Owner

jimwebber commented Mar 6, 2013

I would love 2.0GA to run atop Jetty 9. Their new mechanically sympathetic architecture should give our server a "free" performance boost.

@jakewins

This comment has been minimized.

Show comment Hide comment
@jakewins

jakewins Mar 6, 2013

Contributor

@plalloni would you be interested in bumping the jetty version to 9? If not, I think we will decline this contribution, and instead add it to the team backlog to bump jetty to version 9.

Contributor

jakewins commented Mar 6, 2013

@plalloni would you be interested in bumping the jetty version to 9? If not, I think we will decline this contribution, and instead add it to the team backlog to bump jetty to version 9.

@lalloni

This comment has been minimized.

Show comment Hide comment
@lalloni

lalloni Mar 6, 2013

Contributor

@jakewins I would gladly take a shot at bumping to jetty 9 in any case, but I'm not sure how that would help me to get a happy (production) classpath containing neo4j 1.9+, latest logback & latest unfiltered-jetty.

Also, latest logback-access only declares to be compatible with jetty 7 & 8, whether it's compatible with jetty 9 is TBD.

How about getting jetty 7.x for neo4j 1.9.x and jetty 9.x for neo4j 2.x?

Contributor

lalloni commented Mar 6, 2013

@jakewins I would gladly take a shot at bumping to jetty 9 in any case, but I'm not sure how that would help me to get a happy (production) classpath containing neo4j 1.9+, latest logback & latest unfiltered-jetty.

Also, latest logback-access only declares to be compatible with jetty 7 & 8, whether it's compatible with jetty 9 is TBD.

How about getting jetty 7.x for neo4j 1.9.x and jetty 9.x for neo4j 2.x?

@lalloni

This comment has been minimized.

Show comment Hide comment
@lalloni

lalloni Mar 11, 2013

Contributor

Ok, I've committed for preview the (mostly working) migration to jetty 9.

Still have 4 failing tests to fix though, will do ASAIC.

BTW, logback-access works fine with jetty 9.

Contributor

lalloni commented Mar 11, 2013

Ok, I've committed for preview the (mostly working) migration to jetty 9.

Still have 4 failing tests to fix though, will do ASAIC.

BTW, logback-access works fine with jetty 9.

@lalloni

This comment has been minimized.

Show comment Hide comment
@lalloni

lalloni Mar 13, 2013

Contributor

Well, after some analysis I must ask for a little help/advice for completing this.

In the four still failing tests left there is one (JettyThreadLimittest.shouldHaveSensibleJettyThreadPoolSize) which fails when run from mvn but ends ok when run inside eclipse, and I don't see why.

Other two seem to be cases of jersey failing the request and not setting up the response content type as previously expected (application/json), but since I didn't change jersey version, I don't really understand why is happening now or how could be fixed other than fixing the response entity media type from NeoServletContainer.

The tests failing that way are:

  • AutoIndexFunctionalTests.Relationship_AutoIndex_is_not_removable
  • AutoIndexFunctionalTests.AutoIndex_is_not_removable

In both cases the server answers correctly with a 405 status but fails to set the content type as application/json setting application/html instead. ¿What should I do about this?

The last one failing is HttpsEnabledTest.serverShouldSupportSsl which fails when verifying the status code after making a GET to the / through SSL. The server answers through ssl (which is the behavior being tested) with a status of 303 (see other) forwarding the request to /webadmin/... which AFAIK seems correct... but the test requires an answer with status of 200. In this case I think I should change the test to ignore status code since the motivation is checking that ssl works, not how the rest service behaves. ¿Am I right?

Contributor

lalloni commented Mar 13, 2013

Well, after some analysis I must ask for a little help/advice for completing this.

In the four still failing tests left there is one (JettyThreadLimittest.shouldHaveSensibleJettyThreadPoolSize) which fails when run from mvn but ends ok when run inside eclipse, and I don't see why.

Other two seem to be cases of jersey failing the request and not setting up the response content type as previously expected (application/json), but since I didn't change jersey version, I don't really understand why is happening now or how could be fixed other than fixing the response entity media type from NeoServletContainer.

The tests failing that way are:

  • AutoIndexFunctionalTests.Relationship_AutoIndex_is_not_removable
  • AutoIndexFunctionalTests.AutoIndex_is_not_removable

In both cases the server answers correctly with a 405 status but fails to set the content type as application/json setting application/html instead. ¿What should I do about this?

The last one failing is HttpsEnabledTest.serverShouldSupportSsl which fails when verifying the status code after making a GET to the / through SSL. The server answers through ssl (which is the behavior being tested) with a status of 303 (see other) forwarding the request to /webadmin/... which AFAIK seems correct... but the test requires an answer with status of 200. In this case I think I should change the test to ignore status code since the motivation is checking that ssl works, not how the rest service behaves. ¿Am I right?

@lalloni

This comment has been minimized.

Show comment Hide comment
@lalloni

lalloni Mar 15, 2013

Contributor

@peterneubauer @jakewins I wonder whether you have seen my previous comment or not.

Contributor

lalloni commented Mar 15, 2013

@peterneubauer @jakewins I wonder whether you have seen my previous comment or not.

@jakewins

This comment has been minimized.

Show comment Hide comment
@jakewins

jakewins Mar 21, 2013

Contributor

I'll go ahead and check this out tomorrow and see if I can track down the cause of that

Contributor

jakewins commented Mar 21, 2013

I'll go ahead and check this out tomorrow and see if I can track down the cause of that

@jakewins

This comment has been minimized.

Show comment Hide comment
@jakewins

jakewins Mar 22, 2013

Contributor

Sorry, got caught up with start clauses in cypher today, it will have to wait until monday :(

Contributor

jakewins commented Mar 22, 2013

Sorry, got caught up with start clauses in cypher today, it will have to wait until monday :(

@jakewins

This comment has been minimized.

Show comment Hide comment
@jakewins

jakewins Mar 25, 2013

Contributor

@plalloni we went over this initially today, and it looks really good. We'll be going through the failing tests and fixing what's left to fix. Thanks so much for taking the time to work on this.

We have two aspects left that we will look over: One is that we will need to drop java 6 support entirely for the Neo4j server, the other is that jetty 9 is still not GA. The java 6 support will be discussed internally this week. For jetty 9 GA, the plan is this: We won't merge this in now. Instead, we will fix the tests and update it to be included in neo4j 2.0, and then we will branch it. We'll keep it updated in a branch until jetty 9 is released, at which point we will merge it in.

On your point of updating jetty to jetty 7 for 1.9, I'm afraid we can't do that, the 1.9 release is already very delayed and in a late point of it's release cycle, so we don't want to introduce those kinds of changes at this stage. Would you be able to get your stuff running using a fork of 1.9?

Contributor

jakewins commented Mar 25, 2013

@plalloni we went over this initially today, and it looks really good. We'll be going through the failing tests and fixing what's left to fix. Thanks so much for taking the time to work on this.

We have two aspects left that we will look over: One is that we will need to drop java 6 support entirely for the Neo4j server, the other is that jetty 9 is still not GA. The java 6 support will be discussed internally this week. For jetty 9 GA, the plan is this: We won't merge this in now. Instead, we will fix the tests and update it to be included in neo4j 2.0, and then we will branch it. We'll keep it updated in a branch until jetty 9 is released, at which point we will merge it in.

On your point of updating jetty to jetty 7 for 1.9, I'm afraid we can't do that, the 1.9 release is already very delayed and in a late point of it's release cycle, so we don't want to introduce those kinds of changes at this stage. Would you be able to get your stuff running using a fork of 1.9?

@jakewins

This comment has been minimized.

Show comment Hide comment
@jakewins

jakewins Mar 27, 2013

Contributor

I've squashed this into one commit, and moved it over to a branch on our repo here:

https://github.com/neo4j/neo4j/tree/2.0-jetty-9

I'll close this PR now, and we'll make sure this branch gets merged in when jetty releases 9.0 as a GA. Thanks so much again @plalloni for your work on this :)

Contributor

jakewins commented Mar 27, 2013

I've squashed this into one commit, and moved it over to a branch on our repo here:

https://github.com/neo4j/neo4j/tree/2.0-jetty-9

I'll close this PR now, and we'll make sure this branch gets merged in when jetty releases 9.0 as a GA. Thanks so much again @plalloni for your work on this :)

@jakewins jakewins closed this Mar 27, 2013

@lalloni

This comment has been minimized.

Show comment Hide comment
@lalloni

lalloni Mar 27, 2013

Contributor

@jakewins good to know this is merged, thanks. I still have to figure out what to do with our system given neo4j is the last standing mountain in the path to migration to scala 2.10... we might need to leave the embedded approach to save the situation... not something desirable in terms of performance of course...

Contributor

lalloni commented Mar 27, 2013

@jakewins good to know this is merged, thanks. I still have to figure out what to do with our system given neo4j is the last standing mountain in the path to migration to scala 2.10... we might need to leave the embedded approach to save the situation... not something desirable in terms of performance of course...

@jexp

This comment has been minimized.

Show comment Hide comment
@jexp

jexp Mar 27, 2013

Member

Hi @plalloni scala 2.10 shouldn't be an issue as we support it as of Neo4j 1.9.M04 I think.

Member

jexp commented Mar 27, 2013

Hi @plalloni scala 2.10 shouldn't be an issue as we support it as of Neo4j 1.9.M04 I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment