magibney commented on a change in pull request #592:
URL: https://github.com/apache/solr/pull/592#discussion_r801237620
##########
File path: solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
##########
@@ -854,8 +868,58 @@ private DocSet getAndCacheDocSet(Query query) throws
IOException {
return filterCache.computeIfAbsent(query, q -> getDocSetNC(q, null));
}
- private static Query matchAllDocsQuery = new MatchAllDocsQuery();
+ private static final Query MATCH_ALL_DOCS_QUERY = new MatchAllDocsQuery();
private volatile BitDocSet liveDocs;
+ private final FutureTask<BitDocSet> liveDocsFuture = new
FutureTask<>(this::computeLiveDocs);
+
+ private BitDocSet computeLiveDocs() {
Review comment:
wrt low-level, I realized we needed at least to _check_ for the
MatchAllDocsQuery special case in SolrIndexSearcher (in order to siphon that
case off before consulting filterCache) -- and having realized that, I
basically just ran with [this suggestion/todo
comment](https://github.com/apache/solr/commit/1e63b32731bedf108aaeeb5d0a04d671f5663102#diff-99978700f1c69d6a5f6c2190f89c98cfe20c441161db5a183ec002e15cb1be28R857).
I'm ok with losing the special-case low-level code (though it basically
makes liveDocSet computation into a glorified array copy -- though to be fair I
haven't profiled it). Tangentially: it strikes me that some of this code could
be useful in partially restoring cache (e.g. filterCache) entries per-segment
sometime down the line ...
That said, although I initially went ahead an moved the code to DocSetUtil
as you suggested, I actually ended up moving it back, for reasons explained in
the commit message for e368bdd7008894939276f3b94891979cdc4f88b7:
```
In practice it felt like an awkward (and perhaps somewhat dangerous?)
fit to have `computeLiveDocs` live in `DocSetUtil`. The potential for
deadlock was real; and the `liveDocs` concept is really very intimately
associated with the SolrIndexSearcher per se; putting its computation
in DocSetUtil might tempt people to call it there, when they should
really just get liveDocs via `SolrIndexSearcher.getLiveDocSet()`
```
I'm curious to know what you think. I'm find to move it back into DocSetUtil
if you think that's better (or for that matter remove it entirely if you
prefer).
--
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]
