New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove terms filter lookup cache. #9056
Remove terms filter lookup cache. #9056
Conversation
This is our only cache which is not 'exact' and might allow for stalled results. Additionally, a similar cache that we have and needs to perform lookups in other indices in order to run queries is the script index, and for this index we rely on the filesystem cache, so we should probably do the same with terms filters lookups.
final GetResponse getResponse = client.get(new GetRequest(lookup.getIndex(), lookup.getType(), lookup.getId()).preference("_local").routing(lookup.getRouting())).actionGet(); | ||
if (getResponse.isExists()) { | ||
List<Object> values = XContentMapValues.extractRawValues(lookup.getPath(), getResponse.getSourceAsMap()); | ||
terms.addAll(values); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm what do we do if isExists()
returns false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it might be a preexisting issue but if we execute this on a network thread we might deadlock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do we do if isExists() returns false?
If there is no such entry in the index, then we will call the terms filter parser on an empty list, and this generates a MatchNoDocsFilter (see AbstractFieldMapper.termsFilter).
I know it might be a preexisting issue but if we execute this on a network thread we might deadlock?
I did not change the way that the request is performed, so if there is an issue, it is pre-existing indeed. Any suggestion on how to fix the issue (I don't know much about how we deal with thread pools and network threads)? If it's not easy to fix, I'd lean towards opening another issue for it if you don't mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it might be ok to do this since we are only parsing it on a search thread. I wonder if we can add an assertion for the threadpool that it is not a nework thread ever? but opening a diff issue is good
left two comments - LGTM otherwise |
This is our only cache which is not 'exact' and might allow for stalled results.
Additionally, a similar cache that we have and needs to perform lookups in other
indices in order to run queries is the script index, and for this index we rely
on the filesystem cache, so we should probably do the same with terms filters
lookups.