Re: [PR] Jetty12 + EE10 [solr]
risdenk commented on PR #2876: URL: https://github.com/apache/solr/pull/2876#issuecomment-2835917162 For this PR need to make sure we handle https://issues.apache.org/jira/browse/SOLR-17744 as well. Not sure if anything needs to change with Jetty 12 - @hossman implied there might be a better way with Jetty 12 using the mod itself instead of making one for Jetty 10. -- 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]
Re: [PR] Jetty12 + EE10 [solr]
risdenk commented on code in PR #2876:
URL: https://github.com/apache/solr/pull/2876#discussion_r2045506012
##
gradle/testing/randomization/policies/solr-tests.policy:
##
@@ -240,8 +241,15 @@ grant {
permission java.io.FilePermission "${aws.configFile}", "read,readlink";
permission java.io.FilePermission "${user.home}${/}.aws${/}-",
"read,readlink";
+ // GCS
Review Comment:
@iamsanjay is this still needed?
--
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]
Re: [PR] Jetty12 + EE10 [solr]
risdenk commented on code in PR #2876: URL: https://github.com/apache/solr/pull/2876#discussion_r2045190029 ## gradle/libs.versions.toml: ## @@ -13,8 +13,8 @@ # See the License for the specific language governing permissions and # limitations under the License. [versions] -adobe-testing-s3mock = "2.17.0" -amazon-awssdk = "2.26.19" +adobe-testing-s3mock = "3.9.1" +amazon-awssdk = "2.28.11" Review Comment: Does the AWS sdk need to be updated in this MR? Is this just to align with s3mock? -- 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]
Re: [PR] Jetty12 + EE10 [solr]
dsmiley commented on PR #2876: URL: https://github.com/apache/solr/pull/2876#issuecomment-2800016996 This is a huge PR; I'd like to extract from this PR the "HttpServletRequest avoidance" topic to a separate PR/commit (could be same issue) so as to reduce the scope of this PR to be simpler. I'll do this. -- 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]
Re: [PR] Jetty12 + EE10 [solr]
epugh commented on PR #2876: URL: https://github.com/apache/solr/pull/2876#issuecomment-2799947369 Thanks @dsmiley for some next level debugging. Does this mean we are really close on this landing? -- 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]
Re: [PR] Jetty12 + EE10 [solr]
dsmiley commented on PR #2876: URL: https://github.com/apache/solr/pull/2876#issuecomment-2799731147 Finally I found the root cause of `PackageToolTest.testPackageTool`! Lots of debugging today. It probably doesn't have to do with this PR but the new versions of Jetty seem to tickle it to happening often but not every time. Not sure if it ever happened before; maybe not if the test has been reliable. I filed https://issues.apache.org/jira/browse/SOLR-17740 describing the problem; I will work on a fix. In the mean time, it's okay to add a temporary ignore on that test to unblock this PR, assuming it's only that test which fails. (?) But it'll hopefully be fixed very soon so we can just wait. -- 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]
Re: [PR] Jetty12 + EE10 [solr]
risdenk commented on code in PR #2876:
URL: https://github.com/apache/solr/pull/2876#discussion_r2031137588
##
gradle/libs.versions.toml:
##
@@ -13,8 +13,8 @@
# See the License for the specific language governing permissions and
# limitations under the License.
[versions]
-adobe-testing-s3mock = "2.17.0"
Review Comment:
Does this PR need to be merged with main? I would expect much fewer changes
in dependencies outside of Jetty. It looks like there are other lingering
changes that might also need to be up to date with main.
##
gradle/libs.versions.toml:
##
@@ -80,8 +80,8 @@ decompose = "3.2.2"
diffplug-spotless = "6.5.2"
dropwizard-metrics = "4.2.26"
eclipse-ecj = "3.39.0"
-eclipse-jetty = "10.0.22"
-eclipse-jettytoolchain = "4.0.6"
+eclipse-jetty = "12.0.10"
Review Comment:
Should probably go to 12.0.19 -
https://github.com/jetty/jetty.project/releases/tag/jetty-12.0.19
##
gradle/testing/randomization/policies/solr-tests.policy:
##
@@ -240,8 +241,15 @@ grant {
permission java.io.FilePermission "${aws.configFile}", "read,readlink";
permission java.io.FilePermission "${user.home}${/}.aws${/}-",
"read,readlink";
+ // GCS
Review Comment:
Are these changes supposed to be part of Jetty 12?
##
solr/CHANGES.txt:
##
@@ -119,7 +119,8 @@ Deprecation Removals
Dependency Upgrades
-
-(No changes)
+
+* SOLR-16984: Upgraded internal Jetty server from Jetty 10 to Jetty 12,
including migration from javax.* to jakarta.* namespaces for Servlet and
related APIs. (Sanjay Dutt, David Smiley)
Review Comment:
I'm not fully convinced other dependencies need to be upgraded, but if this
needs to upgrade other dependencies (spring, aws, etc) they should probably be
called out as well
##
gradle/libs.versions.toml:
##
@@ -295,7 +296,7 @@ bouncycastle-bcprov = { module =
"org.bouncycastle:bcprov-jdk18on", version.ref
carrot2-core = { module = "org.carrot2:carrot2-core", version.ref =
"carrot2-core" }
carrotsearch-hppc = { module = "com.carrotsearch:hppc", version.ref =
"carrotsearch-hppc" }
carrotsearch-randomizedtesting-runner = { module =
"com.carrotsearch.randomizedtesting:randomizedtesting-runner", version.ref =
"carrotsearch-randomizedtesting" }
-# @keep transitive dependency for version alignment
Review Comment:
Looks like this changed back again?
##
solr/modules/s3-repository/build.gradle:
##
@@ -21,7 +21,7 @@ description = 'S3 Repository'
dependencies {
Review Comment:
Are all the changes to this s3 repository build.gradle required?
##
solr/core/src/java/org/apache/solr/handler/admin/ShowFileRequestHandler.java:
##
@@ -100,9 +100,14 @@ public class ShowFileRequestHandler extends
RequestHandlerBase implements Permis
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
static {
-KNOWN_MIME_TYPES = new HashSet<>(MimeTypes.getKnownMimeTypes());
+KNOWN_MIME_TYPES = new HashSet<>();
+for (MimeTypes.Type type : MimeTypes.Type.values()) {
+ KNOWN_MIME_TYPES.add(type.toString());
+}
KNOWN_MIME_TYPES.add("text/xml");
KNOWN_MIME_TYPES.add("text/javascript");
+KNOWN_MIME_TYPES.add("text/csv");
Review Comment:
This should be added as a code comment most likely.
--
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]
Re: [PR] Jetty12 + EE10 [solr]
epugh commented on PR #2876: URL: https://github.com/apache/solr/pull/2876#issuecomment-2792429099 I was working on something else yesterday afternoon, and I went and manually ran the steps in the bats test https://github.com/apache/solr/blob/main/solr/packaging/test/test_packages.bats#L64 (which is not part of the test suite due to dependency on third party github links) and saw the same error! I wish I had reported it faster. -- 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]
Re: [PR] Jetty12 + EE10 [solr]
dsmiley commented on PR #2876: URL: https://github.com/apache/solr/pull/2876#issuecomment-2791587430 I spent some time analyzing that test failure for `PackageToolTest.testPackageTool`. The seed is pertinent but not the other args. A gradle executed invocation produces the error almost always but strangely inconsistently via IntelliJ test runner (same seed) -- very strange! A possible timing issue maybe. I discovered that there's an error that's not being reported by the client (client says null but didn't output the error). It's the error message generated by ClusterFiles.validate about an encryption signature that doesn't match. I set a breakpoint on two machines at this point nearby and I see that this branch (showing the problem) gets a buffer length 6733 but a successful run gets length 6734. At a glance, the data is the same except that the longer payload (that works) has an extra leading byte of decimal 80. I didn't look into that further (time for me to quit). I may look again tomorrow night. -- 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]
Re: [PR] Jetty12 + EE10 [solr]
iamsanjay commented on PR #2876: URL: https://github.com/apache/solr/pull/2876#issuecomment-2788499312 Test case is failing. Not [flaky](https://develocity.apache.org/scans/tests?search.rootProjectNames=solr-root&tests.container=org.apache.solr.cli.PackageToolTest&tests.test=testPackageTool)! ``` ./gradlew :solr:core:test --tests "org.apache.solr.cli.PackageToolTest.testPackageTool" "-Ptests.jvmargs=-XX:TieredStopAtLevel=1 -XX:+UseParallelGC -XX:ActiveProcessorCount=1 -XX:ReservedCodeCacheSize=120m" -Ptests.seed=B8415E6AAC736FCD -Ptests.timeoutSuite=60! -Ptests.useSecurityManager=true -Ptests.file.encoding=ISO-8859-1 ``` -- 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]
Re: [PR] Jetty12 + EE10 [solr]
risdenk commented on code in PR #2876:
URL: https://github.com/apache/solr/pull/2876#discussion_r2033086854
##
gradle/libs.versions.toml:
##
@@ -311,33 +312,38 @@ decompose-decompose = { module =
"com.arkivanov.decompose:decompose", version.re
decompose-extensions-compose = { module =
"com.arkivanov.decompose:extensions-compose", version.ref = "decompose" }
dropwizard-metrics-core = { module = "io.dropwizard.metrics:metrics-core",
version.ref = "dropwizard-metrics" }
dropwizard-metrics-graphite = { module =
"io.dropwizard.metrics:metrics-graphite", version.ref = "dropwizard-metrics" }
-dropwizard-metrics-jetty10 = { module =
"io.dropwizard.metrics:metrics-jetty10", version.ref = "dropwizard-metrics" }
+dropwizard-metrics-jetty12 = { module =
"io.dropwizard.metrics:metrics-jetty12", version.ref = "dropwizard-metrics" }
+dropwizard-metrics-jetty12-ee10 = { module =
"io.dropwizard.metrics:metrics-jetty12-ee10", version.ref =
"dropwizard-metrics" }
dropwizard-metrics-jmx = { module = "io.dropwizard.metrics:metrics-jmx",
version.ref = "dropwizard-metrics" }
dropwizard-metrics-jvm = { module = "io.dropwizard.metrics:metrics-jvm",
version.ref = "dropwizard-metrics" }
-dropwizard-metrics-servlets = { module =
"io.dropwizard.metrics:metrics-servlets", version.ref = "dropwizard-metrics" }
+dropwizard-metrics-servlets = { module =
"io.dropwizard.metrics:metrics-jakarta-servlets", version.ref =
"dropwizard-metrics" }
Review Comment:
Do we need
https://mvnrepository.com/artifact/io.dropwizard.metrics/metrics-jakarta-servlet
instead here?
I'm confused why after 60b6ae3a914ca561ee2ccce92d770b708b78e3a3 we have
Jetty 9.4 dependencies included again.
##
gradle/libs.versions.toml:
##
@@ -311,33 +312,38 @@ decompose-decompose = { module =
"com.arkivanov.decompose:decompose", version.re
decompose-extensions-compose = { module =
"com.arkivanov.decompose:extensions-compose", version.ref = "decompose" }
dropwizard-metrics-core = { module = "io.dropwizard.metrics:metrics-core",
version.ref = "dropwizard-metrics" }
dropwizard-metrics-graphite = { module =
"io.dropwizard.metrics:metrics-graphite", version.ref = "dropwizard-metrics" }
-dropwizard-metrics-jetty10 = { module =
"io.dropwizard.metrics:metrics-jetty10", version.ref = "dropwizard-metrics" }
+dropwizard-metrics-jetty12 = { module =
"io.dropwizard.metrics:metrics-jetty12", version.ref = "dropwizard-metrics" }
+dropwizard-metrics-jetty12-ee10 = { module =
"io.dropwizard.metrics:metrics-jetty12-ee10", version.ref =
"dropwizard-metrics" }
dropwizard-metrics-jmx = { module = "io.dropwizard.metrics:metrics-jmx",
version.ref = "dropwizard-metrics" }
dropwizard-metrics-jvm = { module = "io.dropwizard.metrics:metrics-jvm",
version.ref = "dropwizard-metrics" }
-dropwizard-metrics-servlets = { module =
"io.dropwizard.metrics:metrics-servlets", version.ref = "dropwizard-metrics" }
+dropwizard-metrics-servlets = { module =
"io.dropwizard.metrics:metrics-jakarta-servlets", version.ref =
"dropwizard-metrics" }
Review Comment:
Do we need
https://mvnrepository.com/artifact/io.dropwizard.metrics/metrics-jakarta-servlet
instead here?
I'm confused why after 60b6ae3a914ca561ee2ccce92d770b708b78e3a3 we have
Jetty 9.4 dependencies included again.
--
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]
Re: [PR] Jetty12 + EE10 [solr]
iamsanjay commented on PR #2876: URL: https://github.com/apache/solr/pull/2876#issuecomment-2782169134 https://jakarta.ee/specifications/servlet/6.0/jakarta-servlet-spec-6.0 > ### 3.14. Lifetime of the Request Object > Each request object is valid only within the scope of a servlet’s service method, or within the scope of a filter’s doFilter method, unless the asynchronous processing is enabled for the component and the startAsync method is invoked on the request object. In the case where asynchronous processing occurs, the request object remains valid until complete is invoked on the AsyncContext. Containers commonly recycle request objects in order to avoid the performance overhead of request object creation. The developer must be aware that maintaining references to request objects for which startAsync has not been called outside the scope described above is not recommended as it may have indeterminate results. > > In case of upgrade, the above is still true. I came across another PR that might be related: [Jetty Issue #12518](https://github.com/jetty/jetty.project/issues/12518). It seems to align with this behavior. I also found similar language in the Servlet 3.0 specification, which suggests this isn't entirely new — just perhaps enforced more strictly now. That said, it's likely we won't find a single link that fully explains this behavior in isolation. Instead, it seems to be the result of multiple changes introduced during the Jetty 12 ramp-up, especially with their transition to Servlet 6.0 and Jakarta EE 10 compliance. -- 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]
Re: [PR] Jetty12 + EE10 [solr]
dsmiley commented on PR #2876: URL: https://github.com/apache/solr/pull/2876#issuecomment-2781097910 I suspect it may be difficult to remove those methods... but maybe. We've probably done enough for now; tests pass, after all. Can you point to a bit of Jetty or other documentation somewhere that explains this new behavior of Jetty? Ideally the WARNING you added would have such a link for the reader to learn more, but I'd like to learn more _now_ too. It's a really worrisome / troublesome change; pretty surprising IMO. -- 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]
Re: [PR] Jetty12 + EE10 [solr]
iamsanjay commented on PR #2876: URL: https://github.com/apache/solr/pull/2876#issuecomment-2781008247 Thanks! But we should avoid propagating the HttpServletRequest further into the codebase? Ideally, it shouldn’t be part of SolrQueryRequest or exposed beyond Jetty once the request has been processed—that's what I was going for with the earlier suggestion. > Also, can we remove the getHttpSolrCall() method from SolrQueryRequest and drop getReq() from HttpSolrCall as part of that cleanup? It doesn’t have to be part of this change, but maybe something we can consider in a follow-up. -- 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]
Re: [PR] Jetty12 + EE10 [solr]
dsmiley commented on PR #2876: URL: https://github.com/apache/solr/pull/2876#issuecomment-2780963337 I'll update CancellableQueryTracker.java and HttpSolrCall.getUserAgentSolrVersion within the hour roughly -- 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]
Re: [PR] Jetty12 + EE10 [solr]
iamsanjay commented on PR #2876:
URL: https://github.com/apache/solr/pull/2876#issuecomment-2780686392
Can we remove the `HttpSolrCall` getter from `SolrQueryRequest` along with
removing `getReq()` from `HttpSolrCall`?
Few more places.
HttpSolrCall.java
```
public SolrVersion getUserAgentSolrVersion() {
String header = req.getHeader("User-Agent");
if (header == null || !header.startsWith("Solr")) {
return null;
}
try {
return SolrVersion.valueOf(header.substring(header.lastIndexOf(' ') +
1));
} catch (Exception e) {
// unexpected but let's not freak out
assert false : e.toString();
return null;
}
}
```
CancellableQueryTracker.java
`activeQueriesGenerated.put(queryID,
req.getHttpSolrCall().getQueryString());`
--
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]
Re: [PR] Jetty12 + EE10 [solr]
dsmiley commented on PR #2876: URL: https://github.com/apache/solr/pull/2876#issuecomment-2777480066 > ServletApiRequest.getRequest() will return a non-null value once the request crosses an async boundary (e.g., background threads, ExecutorService, etc.) This wasn't clear to me so I dug a little further. So, the standard servlet `HttpServletRequest`, implemented by Jetty, is `ServletApiRequest`... and many of it's methods call an internal `getRequest` that could return null, throwing an NPE. Wow... that sounds so brittle! Basically any HttpServletRequest usage across Solr outside of the front door might throw NPE?! Ouch! Your fix for the tracing issue makes sense -- +1 to that. But it seems like there's a bigger risk. Any Solr code can call `SolrQueryRequest.getHttpSolrCall().getReq()` or `org.apache.solr.request.SolrRequestInfo#getUserPrincipal`. The second could be early resolved I guess but the getReq might need a big fat warning. -- 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]
Re: [PR] Jetty12 + EE10 [solr]
igiguere commented on code in PR #2876:
URL: https://github.com/apache/solr/pull/2876#discussion_r2027025386
##
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##
@@ -411,6 +410,7 @@ public void contextInitialized(ServletContextEvent event) {
// Default servlet as a fall-through
root.addServlet(Servlet404.class, "/");
+ chain = root;
}
chain = injectJettyHandlers(chain);
Review Comment:
... and now 'chain' becomes what? With itself injected into itself?
--
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]
Re: [PR] Jetty12 + EE10 [solr]
igiguere commented on code in PR #2876:
URL: https://github.com/apache/solr/pull/2876#discussion_r2027024323
##
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##
@@ -411,6 +410,7 @@ public void contextInitialized(ServletContextEvent event) {
// Default servlet as a fall-through
root.addServlet(Servlet404.class, "/");
+ chain = root;
Review Comment:
I'm confused. Why initialize 'chain' as Handler.Wrapper only to override
that here, and make 'chain' a ServletContextHandler?
##
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##
@@ -411,6 +410,7 @@ public void contextInitialized(ServletContextEvent event) {
// Default servlet as a fall-through
root.addServlet(Servlet404.class, "/");
+ chain = root;
}
chain = injectJettyHandlers(chain);
Review Comment:
... and now 'chain' becomes what?
--
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]
Re: [PR] Jetty12 + EE10 [solr]
iamsanjay commented on PR #2876:
URL: https://github.com/apache/solr/pull/2876#issuecomment-2774697417
We're hitting a NullPointerException in async code when calling
TraceUtils.getSpan(req) after upgrading to Jetty 12. The root cause is that
Jetty 12 no longer guarantees that ServletApiRequest.getRequest() will return a
non-null value once the request crosses an async boundary (e.g., background
threads, ExecutorService, etc.).
In Solr, we set the tracing Span on the HttpServletRequest early in the
request lifecycle (in traceHttpRequestExecution2()), and later retrieve it
using TraceUtils.getSpan(req). This worked fine in Jetty 9–11, but in Jetty 12,
calling getSpan(req) from async code can throw NPEs because the internal Jetty
Request is gone.
### Fix
To avoid this, we now cache the span inside HttpSolrCall during its
construction, which happens synchronously and before any async processing
starts. This ensures we capture the span while the Jetty request is still valid:
```
public HttpSolrCall(HttpServletRequest req, ...) {
this.span =
Optional.ofNullable(TraceUtils.getSpan(req)).orElse(Span.getInvalid());
}
```
All downstream code — including async paths like processAndWait() or
CoreAdminHandler — now reads the cached Span from the HttpSolrCall instance
rather than trying to access it from the request again.
This avoids the NPE and keeps tracing safe and Jetty 12–compliant without
needing to propagate servlet context manually.
(Powered by ChatGPT! 🤖) It would be great if someone else also confirms this
theory — hahaha!
--
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]
Re: [PR] Jetty12 + EE10 [solr]
dsmiley commented on code in PR #2876:
URL: https://github.com/apache/solr/pull/2876#discussion_r1932792887
##
solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java:
##
@@ -414,16 +415,16 @@ public void contextInitialized(ServletContextEvent event)
{
// Default servlet as a fall-through
root.addServlet(Servlet404.class, "/");
- chain = root;
+ chain.setHandler(root);
Review Comment:
it's be clearer to do this at the front of this block.
--
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]
Re: [PR] Jetty12 + EE10 [solr]
dsmiley commented on code in PR #2876:
URL: https://github.com/apache/solr/pull/2876#discussion_r1931628768
##
solr/core/src/test/org/apache/solr/servlet/HttpSolrCallCloudTest.java:
##
@@ -93,40 +114,66 @@ private void assertCoreChosen(int numCores, TestRequest
testRequest) throws Unav
assertEquals(numCores, coreNames.size());
}
- private static class TestResponse extends Response {
+ public static class TestRequest implements HttpServletRequest {
Review Comment:
I hate mocks but as I look at this, mocking seems perfect so you don't have
to implement everything. Or maybe there's a base class as there was before
that you can extend. (Same feedback for another source file you did this in)
##
solr/core/src/test/org/apache/solr/cloud/CloudExitableDirectoryReaderTest.java:
##
@@ -106,7 +106,7 @@ public static void setupCluster() throws Exception {
((Metered)
metricRegistry
.getMetrics()
-
.get("org.eclipse.jetty.servlet.ServletContextHandler.2xx-responses"))
+
.get("org.eclipse.jetty.server.Handler$Wrapper.2xx-responses"))
Review Comment:
This metrics name seems poor. I wonder if it's due to us/Solr (thus we have
easy control) or is this name chosen by Jetty / DropWizard.
##
solr/modules/s3-repository/build.gradle:
##
@@ -20,7 +20,9 @@ apply plugin: 'java-library'
description = 'S3 Repository'
dependencies {
- api project(':solr:core')
+ api(project(':solr:core')) {
+exclude group: 'org.ow2.asm', module: '*'
Review Comment:
them solr:core shouldn't be publishing that dependency in the first place.
At least see if that can be done.
##
solr/cross-dc-manager/src/java/org/apache/solr/crossdc/manager/consumer/Consumer.java:
##
@@ -21,8 +21,8 @@
import static
org.apache.solr.crossdc.common.KafkaCrossDcConf.ZK_CONNECT_STRING;
import com.codahale.metrics.SharedMetricRegistries;
-import com.codahale.metrics.servlets.MetricsServlet;
-import com.codahale.metrics.servlets.ThreadDumpServlet;
+import io.dropwizard.metrics.servlets.MetricsServlet;
Review Comment:
it's suspicious to see we import com.codahale & io.dropwizard. Maybe okay
but I wasn't expecting.
##
solr/server/etc/jetty.xml:
##
@@ -214,14 +212,16 @@
+
