Adds read locks to LuceneSearchProvider. #52
Conversation
|
What would be great would be if we had some way of testing access to the indexer low levels so we could validate that all calls to them were only done in a readLocked context. It looks pretty good. Needs a CHANGELOG. |
| @@ -547,17 +554,23 @@ private Query getFilterQuery(Set<ApiFilter> filters, int perPage) { | |||
| .map(document -> document.get(idKey)) | |||
| .map(dimension::findDimensionRowByKeyValue) | |||
| .collect(Collectors.toCollection(TreeSet::new)); | |||
|
|
|||
| int documentCount; | |||
| lock.readLock().lock(); | |||
There was a problem hiding this comment.
Don't we need to get this lock much earlier in this method? We're getting indexSearcher up near the top of the method...
|
A few more thoughts:
|
archolewa
force-pushed
the
lucene-index-missing-read-lock
branch
from
6776484 to
84db9a9
Compare
|
@michael-mclawhorn You might be able to do something by taking a write lock in a separate thread and then inspecting the guts of the read-write lock. I did a little bit of digging, but it's not immediately clear to me how to do that. Alternatively, you could take a write lock and then verify that the method doesn't finish, but that would significantly increase our testing time. |
archolewa
force-pushed
the
lucene-index-missing-read-lock
branch
2 times, most recently
from
acdfe4b to
b7fc02b
Compare
| lock.readLock().lock(); | ||
| try { | ||
| numDocs = getIndexSearcher().getIndexReader().numDocs(); | ||
| numDocs = indexSearcher.getIndexReader().numDocs(); |
There was a problem hiding this comment.
Why are we no longer going through the getter?
| try { | ||
| IndexSearcher indexSearcher = getIndexSearcher(); |
There was a problem hiding this comment.
Why is this moving outside the try?
There was a problem hiding this comment.
And isn't the whole point of this lock so that the index searcher can get cycled while it's being held?
| int documentCount; | ||
| initializeIndexSearcher(); | ||
| try { | ||
| lock.readLock().lock(); |
There was a problem hiding this comment.
Move outside the try to match the other calls in this class?
There was a problem hiding this comment.
Correct. any lock must complete before the try
|
👍 |
|
👍 |
archolewa
force-pushed
the
lucene-index-missing-read-lock
branch
from
b6017c2 to
ec00f9d
Compare
The
LuceneSearchProvideris missing read locks before reading the lucene database. As a result, Bad Things can happen if you attempt to read from Lucene (i.e. resolving dimension filters) while loading dimension indices.