You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "markrmiller (via GitHub)" <gi...@apache.org> on 2026/05/16 04:36:41 UTC
[PR] Switch HttpShardHandler shard fan-out to virtual threads, remove ParallelHttpShardHandler [solr]
markrmiller opened a new pull request, #4431:
URL: https://github.com/apache/solr/pull/4431
*Experimental Branch* Not for contribution review.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Switch HttpShardHandler shard fan-out to virtual threads, remove ParallelHttpShardHandler [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller closed pull request #4431: Switch HttpShardHandler shard fan-out to virtual threads, remove ParallelHttpShardHandler
URL: https://github.com/apache/solr/pull/4431
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Switch HttpShardHandler shard fan-out to virtual threads, remove ParallelHttpShardHandler [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #4431:
URL: https://github.com/apache/solr/pull/4431#discussion_r3254798735
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java:
##########
@@ -265,40 +278,108 @@ protected void makeShardRequest(
SimpleSolrResponse ssr,
ShardResponse srsp,
long startTimeNS) {
- CompletableFuture<LBSolrClient.Rsp> future = this.lbClient.requestAsync(lbReq);
- // Synchronize on canceled, so that we know precisely whether to add it to the responseFutureMap
- // or not.
- synchronized (canceled) {
- if (canceled.get() && !future.isDone()) {
- future.cancel(true);
- return;
- } else {
- responseFutureMap.put(srsp, future);
+ // Capture submitter context now so the shard task (running on a virtual thread) sees the
+ // submitter's MDC + every registered InheritableThreadLocalProvider (SolrRequestInfo for PKI,
+ // OTel trace Context, etc.), matching what MDCAwareThreadPoolExecutor does for pool threads.
+ // Virtual threads aren't pooled so no restore is needed.
+ final Map<String, String> submitterMdc = MDC.getCopyOfContextMap();
+ final List<ExecutorUtil.InheritableThreadLocalProvider> providers =
+ ExecutorUtil.getThreadLocalProviders();
+ final List<AtomicReference<Object>> providerCtx;
+ if (providers.isEmpty()) {
+ providerCtx = List.of();
+ } else {
+ providerCtx = new ArrayList<>(providers.size());
+ for (ExecutorUtil.InheritableThreadLocalProvider p : providers) {
+ AtomicReference<Object> ref = new AtomicReference<>();
+ p.store(ref);
+ providerCtx.add(ref);
}
}
- // Add the callback explicitly after adding the future to the map, because the callback relies
- // on the map already having the future.
- future.whenComplete(
- (LBSolrClient.Rsp rsp, Throwable throwable) -> {
- if (rsp != null) {
- ssr.nl = rsp.getResponse();
- srsp.setShardAddress(rsp.getServer());
- } else if (throwable != null) {
- srsp.setException(throwable);
- if (throwable instanceof SolrException) {
- srsp.setResponseCode(((SolrException) throwable).code());
- }
+
+ // Build + dispatch the request on the virtual thread (parallel CPU work) and block on the
+ // returned CompletableFuture there. cancelAll() interrupts the virtual thread; the catch
+ // below translates that into a graceful jetty-future cancel, which is how Jetty wants to be
+ // aborted (vs. interrupting a synchronous send mid-flight).
+ Runnable shardTask =
+ () -> {
+ ExecutorUtil.setServerThreadFlag(Boolean.TRUE);
+ if (submitterMdc != null) {
+ MDC.setContextMap(submitterMdc);
+ }
+ for (int i = 0; i < providers.size(); i++) {
+ providers.get(i).set(providerCtx.get(i));
}
- ssr.elapsedTime =
- TimeUnit.MILLISECONDS.convert(System.nanoTime() - startTimeNS, TimeUnit.NANOSECONDS);
- // Synchronize on cancelled so this code and cancelAll() cannot happen at the same time
- synchronized (canceled) {
- // We don't want to add responses after the requests have been canceled
- if (responseFutureMap.containsKey(srsp)) {
- responses.add(HttpShardHandler.this.transformResponse(sreq, srsp, shard));
+ CompletableFuture<LBSolrClient.Rsp> jettyFuture = null;
+ try {
+ try {
+ // doPrivileged needed because the request setup reads "solr.*" system properties
+ // and otherwise fails under SecurityManager when invoked from a virtual thread.
+ @SuppressWarnings("removal")
+ CompletableFuture<LBSolrClient.Rsp> tmp =
+ AccessController.doPrivileged(
+ (PrivilegedExceptionAction<CompletableFuture<LBSolrClient.Rsp>>)
+ () -> lbClient.requestAsync(lbReq));
+ jettyFuture = tmp;
+ LBSolrClient.Rsp rsp = jettyFuture.get();
+ ssr.nl = rsp.getResponse();
+ srsp.setShardAddress(rsp.getServer());
+ } catch (CancellationException ce) {
+ // jettyFuture was cancelled; leave srsp without an exception/response.
+ } catch (InterruptedException ie) {
+ Thread.currentThread().interrupt();
+ if (jettyFuture != null) {
+ jettyFuture.cancel(true);
+ }
+ } catch (ExecutionException ee) {
+ Throwable cause = ee.getCause() != null ? ee.getCause() : ee;
+ srsp.setException(cause);
+ if (cause instanceof SolrException se) {
+ srsp.setResponseCode(se.code());
+ }
+ } catch (PrivilegedActionException pae) {
+ Throwable cause = pae.getCause() != null ? pae.getCause() : pae;
+ srsp.setException(cause);
+ if (cause instanceof SolrException se) {
+ srsp.setResponseCode(se.code());
+ }
+ }
+ } finally {
+ ssr.elapsedTime =
+ TimeUnit.MILLISECONDS.convert(
+ System.nanoTime() - startTimeNS, TimeUnit.NANOSECONDS);
+ // Synchronize on canceled so this code and cancelAll() cannot happen at the same time
+ synchronized (canceled) {
+ // We don't want to add responses after the requests have been canceled
+ if (pending.containsKey(srsp)) {
+ responses.add(HttpShardHandler.this.transformResponse(sreq, srsp, shard));
+ }
}
+ for (int i = 0; i < providers.size(); i++) {
+ providers.get(i).clean(providerCtx.get(i));
+ }
+ MDC.clear();
Review Comment:
as this is a virtual thread which aren't pool'ed, why do this cleanup?
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java:
##########
@@ -265,40 +278,108 @@ protected void makeShardRequest(
SimpleSolrResponse ssr,
ShardResponse srsp,
long startTimeNS) {
- CompletableFuture<LBSolrClient.Rsp> future = this.lbClient.requestAsync(lbReq);
- // Synchronize on canceled, so that we know precisely whether to add it to the responseFutureMap
- // or not.
- synchronized (canceled) {
- if (canceled.get() && !future.isDone()) {
- future.cancel(true);
- return;
- } else {
- responseFutureMap.put(srsp, future);
+ // Capture submitter context now so the shard task (running on a virtual thread) sees the
+ // submitter's MDC + every registered InheritableThreadLocalProvider (SolrRequestInfo for PKI,
+ // OTel trace Context, etc.), matching what MDCAwareThreadPoolExecutor does for pool threads.
+ // Virtual threads aren't pooled so no restore is needed.
+ final Map<String, String> submitterMdc = MDC.getCopyOfContextMap();
+ final List<ExecutorUtil.InheritableThreadLocalProvider> providers =
+ ExecutorUtil.getThreadLocalProviders();
+ final List<AtomicReference<Object>> providerCtx;
+ if (providers.isEmpty()) {
+ providerCtx = List.of();
+ } else {
+ providerCtx = new ArrayList<>(providers.size());
+ for (ExecutorUtil.InheritableThreadLocalProvider p : providers) {
+ AtomicReference<Object> ref = new AtomicReference<>();
+ p.store(ref);
+ providerCtx.add(ref);
}
}
- // Add the callback explicitly after adding the future to the map, because the callback relies
- // on the map already having the future.
- future.whenComplete(
- (LBSolrClient.Rsp rsp, Throwable throwable) -> {
- if (rsp != null) {
- ssr.nl = rsp.getResponse();
- srsp.setShardAddress(rsp.getServer());
- } else if (throwable != null) {
- srsp.setException(throwable);
- if (throwable instanceof SolrException) {
- srsp.setResponseCode(((SolrException) throwable).code());
- }
+
+ // Build + dispatch the request on the virtual thread (parallel CPU work) and block on the
+ // returned CompletableFuture there. cancelAll() interrupts the virtual thread; the catch
+ // below translates that into a graceful jetty-future cancel, which is how Jetty wants to be
+ // aborted (vs. interrupting a synchronous send mid-flight).
+ Runnable shardTask =
+ () -> {
+ ExecutorUtil.setServerThreadFlag(Boolean.TRUE);
+ if (submitterMdc != null) {
+ MDC.setContextMap(submitterMdc);
+ }
+ for (int i = 0; i < providers.size(); i++) {
+ providers.get(i).set(providerCtx.get(i));
}
- ssr.elapsedTime =
- TimeUnit.MILLISECONDS.convert(System.nanoTime() - startTimeNS, TimeUnit.NANOSECONDS);
- // Synchronize on cancelled so this code and cancelAll() cannot happen at the same time
- synchronized (canceled) {
- // We don't want to add responses after the requests have been canceled
- if (responseFutureMap.containsKey(srsp)) {
- responses.add(HttpShardHandler.this.transformResponse(sreq, srsp, shard));
+ CompletableFuture<LBSolrClient.Rsp> jettyFuture = null;
+ try {
+ try {
+ // doPrivileged needed because the request setup reads "solr.*" system properties
+ // and otherwise fails under SecurityManager when invoked from a virtual thread.
+ @SuppressWarnings("removal")
+ CompletableFuture<LBSolrClient.Rsp> tmp =
Review Comment:
why declare tmp instead of directly setting jettyFuture?
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java:
##########
@@ -265,40 +278,108 @@ protected void makeShardRequest(
SimpleSolrResponse ssr,
ShardResponse srsp,
long startTimeNS) {
- CompletableFuture<LBSolrClient.Rsp> future = this.lbClient.requestAsync(lbReq);
- // Synchronize on canceled, so that we know precisely whether to add it to the responseFutureMap
- // or not.
- synchronized (canceled) {
- if (canceled.get() && !future.isDone()) {
- future.cancel(true);
- return;
- } else {
- responseFutureMap.put(srsp, future);
+ // Capture submitter context now so the shard task (running on a virtual thread) sees the
+ // submitter's MDC + every registered InheritableThreadLocalProvider (SolrRequestInfo for PKI,
+ // OTel trace Context, etc.), matching what MDCAwareThreadPoolExecutor does for pool threads.
+ // Virtual threads aren't pooled so no restore is needed.
+ final Map<String, String> submitterMdc = MDC.getCopyOfContextMap();
+ final List<ExecutorUtil.InheritableThreadLocalProvider> providers =
+ ExecutorUtil.getThreadLocalProviders();
+ final List<AtomicReference<Object>> providerCtx;
+ if (providers.isEmpty()) {
+ providerCtx = List.of();
+ } else {
+ providerCtx = new ArrayList<>(providers.size());
+ for (ExecutorUtil.InheritableThreadLocalProvider p : providers) {
+ AtomicReference<Object> ref = new AtomicReference<>();
+ p.store(ref);
+ providerCtx.add(ref);
}
}
- // Add the callback explicitly after adding the future to the map, because the callback relies
- // on the map already having the future.
- future.whenComplete(
- (LBSolrClient.Rsp rsp, Throwable throwable) -> {
- if (rsp != null) {
- ssr.nl = rsp.getResponse();
- srsp.setShardAddress(rsp.getServer());
- } else if (throwable != null) {
- srsp.setException(throwable);
- if (throwable instanceof SolrException) {
- srsp.setResponseCode(((SolrException) throwable).code());
- }
+
+ // Build + dispatch the request on the virtual thread (parallel CPU work) and block on the
+ // returned CompletableFuture there. cancelAll() interrupts the virtual thread; the catch
+ // below translates that into a graceful jetty-future cancel, which is how Jetty wants to be
+ // aborted (vs. interrupting a synchronous send mid-flight).
+ Runnable shardTask =
+ () -> {
+ ExecutorUtil.setServerThreadFlag(Boolean.TRUE);
+ if (submitterMdc != null) {
+ MDC.setContextMap(submitterMdc);
+ }
+ for (int i = 0; i < providers.size(); i++) {
+ providers.get(i).set(providerCtx.get(i));
}
- ssr.elapsedTime =
- TimeUnit.MILLISECONDS.convert(System.nanoTime() - startTimeNS, TimeUnit.NANOSECONDS);
- // Synchronize on cancelled so this code and cancelAll() cannot happen at the same time
- synchronized (canceled) {
- // We don't want to add responses after the requests have been canceled
- if (responseFutureMap.containsKey(srsp)) {
- responses.add(HttpShardHandler.this.transformResponse(sreq, srsp, shard));
+ CompletableFuture<LBSolrClient.Rsp> jettyFuture = null;
+ try {
+ try {
+ // doPrivileged needed because the request setup reads "solr.*" system properties
+ // and otherwise fails under SecurityManager when invoked from a virtual thread.
+ @SuppressWarnings("removal")
+ CompletableFuture<LBSolrClient.Rsp> tmp =
+ AccessController.doPrivileged(
+ (PrivilegedExceptionAction<CompletableFuture<LBSolrClient.Rsp>>)
Review Comment:
ugh; sad, for something so trivial. Why is this *not* needed seemingly everywhere else?
Any way, we'll likely be removing such things soon.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Switch HttpShardHandler shard fan-out to virtual threads, remove ParallelHttpShardHandler [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on PR #4431:
URL: https://github.com/apache/solr/pull/4431#issuecomment-4479959981
Please see the description: Experimental Branch Not for contribution review.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Switch HttpShardHandler shard fan-out to virtual threads, remove ParallelHttpShardHandler [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4431:
URL: https://github.com/apache/solr/pull/4431#discussion_r3486352061
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##########
@@ -84,6 +85,13 @@ public class HttpShardHandlerFactory extends ShardHandlerFactory
// This executor is initialized in the init method
protected ExecutorService commExecutor;
Review Comment:
Because they are different executors for different purposes.
The commExecutor is used by the jetty httpclient as the async callback/worker executor. It's the non blocking I/O executor.
The shardExecutor is the new virtual thread per task executor that does the blocking scatter/gather. Each virtual thread blocks on jettyFuture.get() while jetty's own executor drives completion.
If you point the LB to the shardExecutor, you will get the httpclient using virtual threads for callbacks and such, which requires a Solrj change (the LBAsyncSolrClient and HttpJettySolrClient.withExecutor(E) plumbing). Which is the larger Solrj rework you were suspecting.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Switch HttpShardHandler shard fan-out to virtual threads, remove ParallelHttpShardHandler [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #4431:
URL: https://github.com/apache/solr/pull/4431#discussion_r3351565257
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java:
##########
@@ -84,6 +85,13 @@ public class HttpShardHandlerFactory extends ShardHandlerFactory
// This executor is initialized in the init method
protected ExecutorService commExecutor;
Review Comment:
Should the loadBalancer be using the shardExecutor instead of this commExecutor? Or will that require a bigger change in SolrJ to better support virtual threads?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Switch HttpShardHandler shard fan-out to virtual threads, remove ParallelHttpShardHandler [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on PR #4431:
URL: https://github.com/apache/solr/pull/4431#issuecomment-4479978403
And to get ahead of "why create the PR then", it was the simplest way to show something to Houston in the most consumable manner.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Switch HttpShardHandler shard fan-out to virtual threads, remove ParallelHttpShardHandler [solr]
Posted by "markrmiller (via GitHub)" <gi...@apache.org>.
markrmiller commented on code in PR #4431:
URL: https://github.com/apache/solr/pull/4431#discussion_r3486321482
##########
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandler.java:
##########
@@ -265,40 +278,108 @@ protected void makeShardRequest(
SimpleSolrResponse ssr,
ShardResponse srsp,
long startTimeNS) {
- CompletableFuture<LBSolrClient.Rsp> future = this.lbClient.requestAsync(lbReq);
- // Synchronize on canceled, so that we know precisely whether to add it to the responseFutureMap
- // or not.
- synchronized (canceled) {
- if (canceled.get() && !future.isDone()) {
- future.cancel(true);
- return;
- } else {
- responseFutureMap.put(srsp, future);
+ // Capture submitter context now so the shard task (running on a virtual thread) sees the
+ // submitter's MDC + every registered InheritableThreadLocalProvider (SolrRequestInfo for PKI,
+ // OTel trace Context, etc.), matching what MDCAwareThreadPoolExecutor does for pool threads.
+ // Virtual threads aren't pooled so no restore is needed.
+ final Map<String, String> submitterMdc = MDC.getCopyOfContextMap();
+ final List<ExecutorUtil.InheritableThreadLocalProvider> providers =
+ ExecutorUtil.getThreadLocalProviders();
+ final List<AtomicReference<Object>> providerCtx;
+ if (providers.isEmpty()) {
+ providerCtx = List.of();
+ } else {
+ providerCtx = new ArrayList<>(providers.size());
+ for (ExecutorUtil.InheritableThreadLocalProvider p : providers) {
+ AtomicReference<Object> ref = new AtomicReference<>();
+ p.store(ref);
+ providerCtx.add(ref);
}
}
- // Add the callback explicitly after adding the future to the map, because the callback relies
- // on the map already having the future.
- future.whenComplete(
- (LBSolrClient.Rsp rsp, Throwable throwable) -> {
- if (rsp != null) {
- ssr.nl = rsp.getResponse();
- srsp.setShardAddress(rsp.getServer());
- } else if (throwable != null) {
- srsp.setException(throwable);
- if (throwable instanceof SolrException) {
- srsp.setResponseCode(((SolrException) throwable).code());
- }
+
+ // Build + dispatch the request on the virtual thread (parallel CPU work) and block on the
+ // returned CompletableFuture there. cancelAll() interrupts the virtual thread; the catch
+ // below translates that into a graceful jetty-future cancel, which is how Jetty wants to be
+ // aborted (vs. interrupting a synchronous send mid-flight).
+ Runnable shardTask =
+ () -> {
+ ExecutorUtil.setServerThreadFlag(Boolean.TRUE);
+ if (submitterMdc != null) {
+ MDC.setContextMap(submitterMdc);
+ }
+ for (int i = 0; i < providers.size(); i++) {
+ providers.get(i).set(providerCtx.get(i));
}
- ssr.elapsedTime =
- TimeUnit.MILLISECONDS.convert(System.nanoTime() - startTimeNS, TimeUnit.NANOSECONDS);
- // Synchronize on cancelled so this code and cancelAll() cannot happen at the same time
- synchronized (canceled) {
- // We don't want to add responses after the requests have been canceled
- if (responseFutureMap.containsKey(srsp)) {
- responses.add(HttpShardHandler.this.transformResponse(sreq, srsp, shard));
+ CompletableFuture<LBSolrClient.Rsp> jettyFuture = null;
+ try {
+ try {
+ // doPrivileged needed because the request setup reads "solr.*" system properties
+ // and otherwise fails under SecurityManager when invoked from a virtual thread.
+ @SuppressWarnings("removal")
+ CompletableFuture<LBSolrClient.Rsp> tmp =
+ AccessController.doPrivileged(
+ (PrivilegedExceptionAction<CompletableFuture<LBSolrClient.Rsp>>)
Review Comment:
DocumentObjectBinder.collectInfo does the same doPrivileged dance for the same reason. It's required wherever code touches a SecurityManager-protected resource (here, reading solr.* system properties during request setup) from a context whose inherited AccessControlContext is more restrictive - which is the case on the per-shard virtual thread. Most other Solr code reads those props on the request thread that already holds the permission, so it doesn't need it. The need for it goes away when SecurityManager goes away.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org