dsmiley commented on code in PR #2960:
URL: https://github.com/apache/solr/pull/2960#discussion_r1926430030
##########
solr/core/src/java/org/apache/solr/search/QueryResult.java:
##########
@@ -22,6 +22,7 @@ public class QueryResult {
// Object for back compatibility so that we render true not "true" in json
private Object partialResults;
private Boolean segmentTerminatedEarly;
Review Comment:
thinking out loud: Perhaps we should do away with segmentTerminatedEarly as
overly specific
##########
solr/core/src/java/org/apache/solr/search/QueryCommand.java:
##########
@@ -194,7 +195,7 @@ public QueryCommand setNeedDocSet(boolean needDocSet) {
}
public boolean getTerminateEarly() {
- return (flags & SolrIndexSearcher.TERMINATE_EARLY) != 0;
+ return (flags & SolrIndexSearcher.TERMINATE_EARLY) != 0 || maxHits <
Integer.MAX_VALUE;
Review Comment:
When I see these other constants about TERMINATE_EARLY, this is what leads
me to think that maxHitsTerminateEarly is a good name.
##########
solr/core/src/java/org/apache/solr/response/SolrQueryResponse.java:
##########
@@ -68,6 +68,7 @@ public class SolrQueryResponse {
public static final String RESPONSE_HEADER_PARTIAL_RESULTS_DETAILS_KEY =
"partialResultsDetails";
public static final String RESPONSE_HEADER_SEGMENT_TERMINATED_EARLY_KEY =
"segmentTerminatedEarly";
+ public static final String RESPONSE_HEADER_TERMINATED_EARLY_KEY =
"terminatedEarly";
Review Comment:
Somewhere we need documentation to differentiate the meaning of this vs
partialResults. To me, this _implies_ partialResults at the least and it may
be redundant.
##########
solr/core/src/java/org/apache/solr/search/EarlyTerminatingCollector.java:
##########
@@ -29,11 +30,15 @@
*/
public class EarlyTerminatingCollector extends FilterCollector {
+ private final int chunkSize = 100; // Check across threads only at a chunk
size
+
private final int maxDocsToCollect;
private int numCollected = 0;
private int prevReaderCumulativeSize = 0;
private int currentReaderSize = 0;
+ private final AtomicInteger pendingDocsToCollect;
Review Comment:
LongAdder is likely a better choice for this use-case
##########
solr/core/src/java/org/apache/solr/search/MultiThreadedSearcher.java:
##########
@@ -255,7 +265,11 @@ public Object reduce(Collection collectors) throws
IOException {
for (Iterator var4 = collectors.iterator();
var4.hasNext();
maxScore = Math.max(maxScore, collector.getMaxScore())) {
- collector = (MaxScoreCollector) var4.next();
+ Collector next = (Collector) var4.next();
Review Comment:
could use a comment like "assume the next collector, except
EarlyTerminating, is the MaxScoreCollector".
While you are changing this code, please rename `var4` which is an
inexcusable variable name
##########
solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java:
##########
@@ -338,6 +339,11 @@ private Collector buildAndRunCollectorChain(
if (cmd.isQueryCancellable()) {
core.getCancellableQueryTracker().removeCancellableQuery(cmd.getQueryID());
}
+ if (collector instanceof final EarlyTerminatingCollector
earlyTerminatingCollector) {
Review Comment:
this check is brittle since it can only possibly true if the *last*
collector is earlyTerminatingCollector. We may add others or re-order at some
point. Also, can we just depend on the exception above and set
`qr.setTerminatedEarly(true);` there?
--
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]
