gerlowskija commented on code in PR #2666:
URL: https://github.com/apache/solr/pull/2666#discussion_r1777144114
##########
solr/core/src/java/org/apache/solr/handler/component/ParallelHttpShardHandler.java:
##########
@@ -41,57 +39,78 @@
@NotThreadSafe
public class ParallelHttpShardHandler extends HttpShardHandler {
+ @SuppressWarnings("unused")
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
private final ExecutorService commExecutor;
+ /*
+ * Unlike the basic HttpShardHandler, this class allows us to exit submit
before
+ * pending is incremented and the responseFutureMap is updated. If the
runnables that
Review Comment:
[Q] Taking the "this class allows us to exit submit before pending is
incremented" behavior as a given, the use of these additional AtomicInt's makes
sense. And the descriptive comment here is lovely. +1!
But why does the PR introduce this complication in the first place? Is it
required by SOLR-17158? Is it fixing a concurrency bug that pre-exists
SOLR-17158? It's hard to chime in without the context and rationale, but it
feels like there should be a simpler way to do this that doesn't require 3
AtomicInt's, a Map of Futures, and a lock to go with it?
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
