rnewson commented on code in PR #4791:
URL: https://github.com/apache/couchdb/pull/4791#discussion_r1346260502
##########
nouveau/src/main/java/org/apache/couchdb/nouveau/core/IndexManager.java:
##########
@@ -97,7 +97,7 @@ public <R> R with(final String name, final IndexLoader
loader, final IndexFuncti
throw (IOException) e.getCause();
}
- if (index.tryAcquire(1, TimeUnit.SECONDS)) {
+ if (index.tryAcquire()) {
Review Comment:
the original was an attempt at backpressure (and not sure it worked tbh,
it's hard to induce pressure here).
the commit that changes this code makes a more fundamental change in
close(), where the permits are acquired before closing and then deliberately
never released again. so if tryAcquire returns false it will always return
false (if we can exclude the possibility of 2^31-1 concurrent requests for that
index) so waiting for up to a second is pointless.
The reason we have a while loop here is for the following scenario;
1) index becomes idle
2) cache expiration triggers, triggering the eviction code which closes the
index and removes it from the map
3) a new query for the same index comes in at just the worst moment
we want the query at 3) to succeed, it just needs to wait for the index to
close (which means blocking for in-flight requests to finish), be removed from
the index, and opened again.
this PR also changes when we evict, by moving from maxSize to maxWeight,
specifically setting a weight of 0 for active indexes so they won't be closed),
so the odds of this happening are quite low, but I don't think actually zero.
I've tried, across the commits, to simplify things, to make it easier to see
how and where concurrency and cache state changes are happening. I think
there's more to be done too but these seem important first steps.
--
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]
