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
