SOLR-16158: Remove server jars from solr-core #1179
Conversation
- Move SolrJettyRunner to test framework. Solr-core now no longer has jetty server deps. - server/lib/ext now contains the following jars: - logging - metrics - Shared server & core dependencies - The web-app is now setup so that "server" jars can be used by the Solr webapp. We no longer have to duplicate these jetty "server" jars between server/lib and WEB-INF/lib. - The solr-core & solr-server dependencies now just work, and shouldn't require special setup.
| <Call name="addServerClassMatcher"> | ||
| <Arg> | ||
| <New id="removeServerClasses" class="org.eclipse.jetty.webapp.ClassMatcher"> |
There was a problem hiding this comment.
Yea agree - would be good to add a comment to why this is needed?
There was a problem hiding this comment.
Yeah will definitely add a comment. But here's the gist:
https://www.eclipse.org/jetty/documentation/jetty-9/index.html#jetty-classloading
Jetty protects access to certain "server" classes. You can see the defaults if you scroll down to the "Setting Server Classes" section in the documentation link above. (Annoyingly the same documentation doesn't exist for Jetty 10 or 11, although the functionality is the same, other than a method signature) This means that the webapp cannot use these classes unless they are also provided in the WEB-INF/lib directory as well, meaning that we have to duplicate these jars using the default settings.
However when we add this set of serverClassMatchers, which all start with -, it removes these classes from the protected "server" class definition. This allows us to use the same jars between the jetty server and our webapp.
The documentation doesn't actually give any good reasons why these classes should be protected, or maybe I just missed it. So we can always go back to duplicating the jars, but I think this is probably a better way forward.
Obviously we need some pretty extensive testing to make sure that the setParentLoaderPriority(true) option doesn't mess with any of our class loader stuff. So far the integration tests, docker tests and unit tests all work so far. But we will definitely need to do some more.
There was a problem hiding this comment.
Thanks for that explanation; please add a summary as a comment in the XML.
I definitely prefer a solution (like this) that avoids duplicate JARs; I didn't know we were doing that (yuck!).
RE classpath separation (Jetty vs web app), note there is a nasty line in bin/solr when SOLR_SSL_ENABLED:
SOLR_JETTY_CONFIG+=("--module=https" "--lib=$DEFAULT_SERVER_DIR/solr-webapp/webapp/WEB-INF/lib/*")
I recall the original introduction of this was by @markrmiller when I did git history analysis some time ago. I feel we should either not do that, or just give up on the separation -- this is a single-web app server so why separate?
There was a problem hiding this comment.
RE classpath separation (Jetty vs web app), note there is a nasty line in bin/solr when SOLR_SSL_ENABLED:
I have a sneaking suspicion that isn't necessary, at least anymore. We should try to tackle that in a separate PR though. Would like to not have to test SSL at the same time 😅
There was a problem hiding this comment.
The doc exist here for jetty 10: https://www.eclipse.org/jetty/documentation/jetty-10/old_docs/index.html#jetty-classloading The note for old docs is "The old Jetty documentation refers to old Jetty versions, and it is currently being ported to the current Jetty version."
| <Call name="addServerClassMatcher"> | ||
| <Arg> | ||
| <New id="removeServerClasses" class="org.eclipse.jetty.webapp.ClassMatcher"> |
There was a problem hiding this comment.
Yea agree - would be good to add a comment to why this is needed?
.../core/src/test/org/apache/solr/client/solrj/embedded/TestEmbeddedSolrServerAdminHandler.java
Outdated
Show resolved
Hide resolved
.../core/src/test/org/apache/solr/client/solrj/embedded/TestEmbeddedSolrServerConstructors.java
Outdated
Show resolved
Hide resolved
solr/core/src/test/org/apache/solr/client/solrj/embedded/TestEmbeddedSolrServerSchemaAPI.java
Outdated
Show resolved
Hide resolved
solr/core/src/test/org/apache/solr/client/solrj/embedded/TestJettySolrRunner.java
Outdated
Show resolved
Hide resolved
Move TestJettySolrRunner to the test-framework module.
|
Note: the jetty-server jar is in lib/ext even though Solr core/SolrJ should not need it. We can hunt that down in a separate issue though if need be. |
|
The CHANGES.txt text speaks of duplicated JARs but does not mention any. If this was actually a problem in 9.1 (I forget?) then okay -- could you list at least one there? If that was not a released problem (e.g. happened after 9.1) then don't mention it; keep it in the commit notes, though. |
|
Ok listed 2, and expanded a bit in both sections. |
- Move SolrJettyRunner to test framework. Solr-core now no longer has jetty server deps. - server/lib/ext now contains the following jars: - logging - metrics - Shared server & core dependencies - The web-app is now setup so that "server" jars can be used by the Solr webapp. We no longer have to duplicate these jetty "server" jars between server/lib and WEB-INF/lib. - The solr-core & solr-server dependencies now just work, and shouldn't require special setup. (cherry picked from commit a18f5b3)
https://issues.apache.org/jira/browse/SOLR-16158
This is a slight back-compat concern, but I'm not sure the SolrJettyRunner is used widely enough for it to warrant keeping this from 9x
This is also a slight back-compat concern, but we are already changing our jetty xml files for the jetty 10 upgrade, so this change is likely ok too.
Most of the files are just changing the location of SolrJettyRunner. If you want to see the actual changes here, then just look at the
*/build.gradleandsolr/server/contexts/solr-jetty-context.xmlfiles.