dsmiley commented on code in PR #4574:
URL: https://github.com/apache/solr/pull/4574#discussion_r3502761730
##########
changelog/unreleased/SOLR-18297-update-jetty-12.1.10.yml:
##########
@@ -0,0 +1,7 @@
+title: Upgrade Eclipse Jetty to 12.1.10. The maximum response header size now
defaults to 16KB (up from 8KB) and is configurable via the
solr.jetty.response.header.size.max system property.
Review Comment:
IMO the header matter is minutia that isn't warranted to mention here. It's
not like we decreased the header.
##########
solr/core/src/java/org/apache/solr/servlet/HttpSolrProxy.java:
##########
@@ -41,15 +41,19 @@
class HttpSolrProxy {
// TODO add X-Forwarded-For and with comma delimited
- private static final Set<HttpHeader> HOP_BY_HOP_HEADERS =
+ // Headers not to forward: hop-by-hop, plus Host and Content-Length which
the Jetty client
+ // re-derives for the upstream. Forwarding the originals makes Jetty 12.1
reject with a 400.
+ private static final Set<HttpHeader> SKIP_HEADERS =
EnumSet.of(
HttpHeader.CONNECTION,
HttpHeader.KEEP_ALIVE,
HttpHeader.PROXY_AUTHENTICATE,
HttpHeader.PROXY_AUTHORIZATION,
HttpHeader.TE,
HttpHeader.TRANSFER_ENCODING,
- HttpHeader.UPGRADE);
+ HttpHeader.UPGRADE,
+ HttpHeader.HOST,
Review Comment:
I'm a bit unsure what the impact of skipping HOST is.
##########
solr/server/etc/jetty.xml:
##########
@@ -188,40 +188,47 @@
<Set name="dumpAfterStart">false</Set>
<Set name="dumpBeforeStop">false</Set>
- <Call name="addBean">
- <Arg>
- <New id="DeploymentManager"
class="org.eclipse.jetty.deploy.DeploymentManager">
Review Comment:
I like this change; thanks.
##########
solr/ui/build.gradle.kts:
##########
Review Comment:
looks like you took this from my PR ;-)
##########
solr/core/src/java/org/apache/solr/servlet/HttpSolrProxy.java:
##########
@@ -41,15 +41,19 @@
class HttpSolrProxy {
// TODO add X-Forwarded-For and with comma delimited
- private static final Set<HttpHeader> HOP_BY_HOP_HEADERS =
+ // Headers not to forward: hop-by-hop, plus Host and Content-Length which
the Jetty client
+ // re-derives for the upstream. Forwarding the originals makes Jetty 12.1
reject with a 400.
+ private static final Set<HttpHeader> SKIP_HEADERS =
EnumSet.of(
HttpHeader.CONNECTION,
HttpHeader.KEEP_ALIVE,
HttpHeader.PROXY_AUTHENTICATE,
HttpHeader.PROXY_AUTHORIZATION,
HttpHeader.TE,
HttpHeader.TRANSFER_ENCODING,
- HttpHeader.UPGRADE);
+ HttpHeader.UPGRADE,
+ HttpHeader.HOST,
+ HttpHeader.CONTENT_LENGTH);
Review Comment:
do we mess with the content, thus making the length inaccurate?
##########
solr/core/src/java/org/apache/solr/jersey/container/JettyBridgeResponseWriter.java:
##########
@@ -58,10 +59,13 @@ public OutputStream writeResponseStatusAndHeaders(
final StatusType statusInfo = context.getStatusInfo();
httpServletResponse.setStatus(statusInfo.getStatusCode());
- if (contentLength != -1 && contentLength < Integer.MAX_VALUE) {
- httpServletResponse.setContentLength((int) contentLength);
- }
+ // Don't set Content-Length: Solr's QueryResponseWriters stream and can't
predict the exact
+ // length, so a declared value may undercount and Jetty 12.1 aborts with
"too much content
+ // written". Leaving it unset frames the response as chunked, as the v1
path already does.
Review Comment:
I view this as a loss, albeit minor. Just a guess; this scenario is
triggered with maybe filestore, since it serves files with known lengths
instead of dynamic content? If we know the precise length, why would it be
mangled at downstream layers?
##########
solr/server/etc/jetty.xml:
##########
@@ -188,40 +188,47 @@
<Set name="dumpAfterStart">false</Set>
<Set name="dumpBeforeStop">false</Set>
- <Call name="addBean">
- <Arg>
- <New id="DeploymentManager"
class="org.eclipse.jetty.deploy.DeploymentManager">
- <Set name="contexts">
- <Ref refid="Contexts" />
- </Set>
- <Call name="addAppProvider">
- <Arg>
- <New class="org.eclipse.jetty.deploy.providers.ContextProvider">
- <Set name="EnvironmentName">core</Set>
- <Set name="monitoredDirName"><Property name="jetty.base"
default="."/>/contexts</Set>
- <Set name="scanInterval">0</Set>
- </New>
- </Arg>
- </Call>
-
- <!-- Add a customize step to the deployment lifecycle -->
- <!-- uncomment and replace DebugBinding with your extended
AppLifeCycle.Binding class
- <Call name="insertLifeCycleNode">
- <Arg>deployed</Arg>
- <Arg>starting</Arg>
- <Arg>customise</Arg>
- </Call>
- <Call name="addLifeCycleBinding">
- <Arg>
- <New class="org.eclipse.jetty.deploy.bindings.DebugBinding">
- <Arg>customise</Arg>
- </New>
- </Arg>
- </Call>
- -->
+ <!-- =========================================================== -->
+ <!-- Deploy the Solr webapp. -->
+ <!-- Solr serves a single, statically-known webapp, so we add it -->
+ <!-- directly to the context collection rather than using Jetty's -->
+ <!-- directory-scanning deployer (the DeploymentManager / -->
+ <!-- ContextProvider API was removed in Jetty 12.1). -->
+ <!-- =========================================================== -->
+ <Ref refid="Contexts">
+ <Call name="addHandler">
+ <Arg>
+ <New class="org.eclipse.jetty.ee10.webapp.WebAppContext">
+ <Set name="contextPath">/solr</Set>
+ <Set name="war"><Property
name="jetty.base"/>/solr-webapp/webapp</Set>
Review Comment:
while we're at it, can we remove the pointless intermediate dir?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
