gus-asf commented on code in PR #2666: URL: https://github.com/apache/solr/pull/2666#discussion_r1775211297
########## solr/core/src/java/org/apache/solr/handler/component/ParallelHttpShardHandler.java: ########## @@ -45,53 +48,84 @@ public class ParallelHttpShardHandler extends HttpShardHandler { private final ExecutorService commExecutor; + AtomicInteger attemptStart = new AtomicInteger(0); + AtomicInteger attemptCount = new AtomicInteger(0); + public ParallelHttpShardHandler(ParallelHttpShardHandlerFactory httpShardHandlerFactory) { super(httpShardHandlerFactory); this.commExecutor = httpShardHandlerFactory.commExecutor; } + @Override + protected boolean responsesPending() { + // ensure we can't exit while loop in HttpShardHandler.take(boolean) until we've completed + // as many Runnable actions as we created. + return super.responsesPending() || attemptStart.get() > attemptCount.get(); + } + @Override public void submit(ShardRequest sreq, String shard, ModifiableSolrParams params) { + attemptStart.incrementAndGet(); // do this outside of the callable for thread safety reasons final List<String> urls = getURLs(shard); final var lbReq = prepareLBRequest(sreq, shard, params, urls); final var srsp = prepareShardResponse(sreq, shard); final var ssr = new SimpleSolrResponse(); srsp.setSolrResponse(ssr); - pending.incrementAndGet(); if (urls.isEmpty()) { recordNoUrlShardResponse(srsp, shard); return; Review Comment: Ah good point! In fact it's probably best not to increment attemptStart until the Runnable is given to the executor so that any failure prior to that doesn't cause issues. ########## solr/core/src/java/org/apache/solr/handler/component/ParallelHttpShardHandler.java: ########## @@ -45,53 +48,84 @@ public class ParallelHttpShardHandler extends HttpShardHandler { private final ExecutorService commExecutor; + AtomicInteger attemptStart = new AtomicInteger(0); + AtomicInteger attemptCount = new AtomicInteger(0); + public ParallelHttpShardHandler(ParallelHttpShardHandlerFactory httpShardHandlerFactory) { super(httpShardHandlerFactory); this.commExecutor = httpShardHandlerFactory.commExecutor; } + @Override + protected boolean responsesPending() { + // ensure we can't exit while loop in HttpShardHandler.take(boolean) until we've completed + // as many Runnable actions as we created. + return super.responsesPending() || attemptStart.get() > attemptCount.get(); + } + @Override public void submit(ShardRequest sreq, String shard, ModifiableSolrParams params) { + attemptStart.incrementAndGet(); // do this outside of the callable for thread safety reasons final List<String> urls = getURLs(shard); final var lbReq = prepareLBRequest(sreq, shard, params, urls); final var srsp = prepareShardResponse(sreq, shard); final var ssr = new SimpleSolrResponse(); srsp.setSolrResponse(ssr); - pending.incrementAndGet(); if (urls.isEmpty()) { recordNoUrlShardResponse(srsp, shard); return; Review Comment: This also enables us to avoid code duplication too :) -- 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