[GitHub] [lucene-solr] mikemccand commented on a change in pull request #815: LUCENE-8213: Introduce Asynchronous Caching in LRUQueryCache
mikemccand commented on a change in pull request #815: LUCENE-8213: Introduce
Asynchronous Caching in LRUQueryCache
URL: https://github.com/apache/lucene-solr/pull/815#discussion_r322798227
##
File path: lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java
##
@@ -656,13 +713,20 @@ public long ramBytesUsed() {
// threads when IndexSearcher is created with threads
private final AtomicBoolean used;
+private Executor executor;
Review comment:
`final`?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on a change in pull request #815: LUCENE-8213: Introduce Asynchronous Caching in LRUQueryCache
mikemccand commented on a change in pull request #815: LUCENE-8213: Introduce
Asynchronous Caching in LRUQueryCache
URL: https://github.com/apache/lucene-solr/pull/815#discussion_r322797372
##
File path: lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java
##
@@ -367,6 +393,7 @@ public void clearQuery(Query query) {
if (singleton != null) {
onEviction(singleton);
}
+ inFlightAsyncLoadQueries.remove(query);
Review comment:
Let's get this under `finally` clause?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on a change in pull request #815: LUCENE-8213: Introduce Asynchronous Caching in LRUQueryCache
mikemccand commented on a change in pull request #815: LUCENE-8213: Introduce
Asynchronous Caching in LRUQueryCache
URL: https://github.com/apache/lucene-solr/pull/815#discussion_r322797612
##
File path: lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java
##
@@ -262,6 +283,11 @@ DocIdSet get(Query key, IndexReader.CacheHelper
cacheHelper) {
assert lock.isHeldByCurrentThread();
assert key instanceof BoostQuery == false;
assert key instanceof ConstantScoreQuery == false;
+
+if (inFlightAsyncLoadQueries.contains(key)) {
Review comment:
Can you add to javadocs for this method explaining when/why this exception
is thrown?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on a change in pull request #815: LUCENE-8213: Introduce Asynchronous Caching in LRUQueryCache
mikemccand commented on a change in pull request #815: LUCENE-8213: Introduce Asynchronous Caching in LRUQueryCache URL: https://github.com/apache/lucene-solr/pull/815#discussion_r322794044 ## File path: lucene/CHANGES.txt ## @@ -56,6 +56,9 @@ Improvements * LUCENE-8937: Avoid agressive stemming on numbers in the FrenchMinimalStemmer. (Adrien Gallou via Tomoko Uchida) +* LUCENE-8213: LRUQueryCache#doCache can now use passed in Executor to + asynchronously load heavy queries (Atri Sharma) Review comment: s/`load`/`cache`? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on a change in pull request #815: LUCENE-8213: Introduce Asynchronous Caching in LRUQueryCache
mikemccand commented on a change in pull request #815: LUCENE-8213: Introduce Asynchronous Caching in LRUQueryCache URL: https://github.com/apache/lucene-solr/pull/815#discussion_r322793756 ## File path: lucene/CHANGES.txt ## @@ -56,6 +56,9 @@ Improvements * LUCENE-8937: Avoid agressive stemming on numbers in the FrenchMinimalStemmer. (Adrien Gallou via Tomoko Uchida) +* LUCENE-8213: LRUQueryCache#doCache can now use passed in Executor to Review comment: Can you reword to state that it's the `Executor` passed to `IndexSearcher`'s constructor? 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on a change in pull request #815: LUCENE-8213: Introduce Asynchronous Caching in LRUQueryCache
mikemccand commented on a change in pull request #815: LUCENE-8213: Introduce
Asynchronous Caching in LRUQueryCache
URL: https://github.com/apache/lucene-solr/pull/815#discussion_r322794753
##
File path: lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java
##
@@ -89,12 +92,30 @@
*/
public class LRUQueryCache implements QueryCache, Accountable {
+ /** Throw this error when a query is asynchronously being loaded in the cache
+ * and a read for the same is requested
+ */
+ private static final class AsyncQueryLoadInProgressException extends
RuntimeException {
+
+/**
+ * Sole constructor.
+ */
+public AsyncQueryLoadInProgressException(String query) {
Review comment:
Hmm why does this take `String query` not `Query query`?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] mikemccand commented on a change in pull request #815: LUCENE-8213: Introduce Asynchronous Caching in LRUQueryCache
mikemccand commented on a change in pull request #815: LUCENE-8213: Introduce
Asynchronous Caching in LRUQueryCache
URL: https://github.com/apache/lucene-solr/pull/815#discussion_r322809172
##
File path: lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java
##
@@ -726,14 +790,35 @@ public ScorerSupplier scorerSupplier(LeafReaderContext
context) throws IOExcepti
DocIdSet docIdSet;
try {
docIdSet = get(in.getQuery(), cacheHelper);
+ } catch (AsyncQueryLoadInProgressException e) {
Review comment:
We could alternatively have this request wait for cache to populate and then
return that cached result. It'd be cheaper CPU overall, especially in
degenerate queries where the same query is often executed multiple times
concurrently, but I think more complicated here, so let's go forward w/ this
approach?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
