Do not apply memory optimized serach for old indices. #2918
Conversation
0ctopus13prime
requested review from
VijayanB,
Vikasht34,
heemin32,
jmazanec15,
junqiu-lei,
luyuncheng,
martin-gaievski,
naveentatikonda,
navneet1v,
ryanbogan,
shatejas and
vamshin
as code owners
0ctopus13prime
force-pushed
the
loc-block-old-index
branch
2 times, most recently
from
c330cef to
74dfc7e
Compare
0ctopus13prime
force-pushed
the
loc-block-old-index
branch
7 times, most recently
from
5f203c7 to
847c533
Compare
0ctopus13prime
force-pushed
the
loc-block-old-index
branch
3 times, most recently
from
61eb4e5 to
004a9d0
Compare
| ) { | ||
| // Since LuceneOnFaiss logic sits in newer codec, we don't support LuceneOnFaiss for older codec whose version < 2.19 | ||
| if (indexCreatedVersion.before(Version.V_2_19_0)) { | ||
| return false; |
There was a problem hiding this comment.
should this throw an exception? The idea is that if user tries to turn on the setting for indices before 2.19 it shouldn't be allowed rather than silently falling back to default behavior
There was a problem hiding this comment.
Currently, there's no way to block user from enabling memory optimized search as in the event listener, we can't look up mapping from IndicesService. And the setting sits at index level, requires user to close, update and reopen it again. If we throw an exception, then all traffics will be hitting 400, then to recover, user should close the index again, and turn it off, and reopen again where loading all bytes will happen. At the worst case, if there's no sufficient memory space, then whole cluster can be ruined. 😓
The best way is to block user from enabling it if the target index is not supported, but technically we can't do it right now.
There was a problem hiding this comment.
@navneet1v Thoughts on throwing vs silently falling back to graph loading?
I am okay with silently falling back for now, please add a debug log and update the documentation for memory optimized search
There was a problem hiding this comment.
There were some setting we depreacated in 3.0 and when I was trying in 3.1 , it was throwinng as undefined setting !! So if we have launched memoryoptimzed from 2.19 , would user able to set the setting since there is no updated code which has memoryoptimzed?
There was a problem hiding this comment.
@0ctopus13prime I think the better experience here will be to throw the exception when the setting is getting set at the index. But since we cannot do this, as an application if somehow user has set this setting on old indices we should throw exception during query to ensure that user know they are doing something incorrect, rather than silently falling back.
@0ctopus13prime another thing, what is the reason for enabling this setting for indices >=2.19 ? it should be 2.17 because from 2.17 we started using KNNVectorField of Lucene.
There was a problem hiding this comment.
@navneet1v
But isn't it better to fallback to default rather than throwing an error?
Once it's configured, then all traffic would end up getting 400 if we throw an exception there. 🤔
Would it be better if we could give a chance to user to pick window for closing index and reopen it?
There was a problem hiding this comment.
But isn't it better to fallback to default rather than throwing an error?
No its not better to fallback since it gives a wrong impression to users and operators that the feature is working. If we can throw exception during turning on of this setting that will be great. But looks like it is not possible. So atleast on search path we should throw exceptions.
| ) { | ||
| // Since LuceneOnFaiss logic sits in newer codec, we don't support LuceneOnFaiss for older codec whose version < 2.19 | ||
| if (indexCreatedVersion.before(Version.V_2_19_0)) { | ||
| return false; |
There was a problem hiding this comment.
There were some setting we depreacated in 3.0 and when I was trying in 3.1 , it was throwinng as undefined setting !! So if we have launched memoryoptimzed from 2.19 , would user able to set the setting since there is no updated code which has memoryoptimzed?
There was a problem hiding this comment.
@0ctopus13prime another thing, what is the reason for enabling this setting for indices >=2.19 ? it should be 2.17 because from 2.17 we started using KNNVectorField of Lucene.
0ctopus13prime
force-pushed
the
loc-block-old-index
branch
from
004a9d0 to
c443d5a
Compare
It must be a something 😅, it was not working for some reason, but it seems working now with 2.17. |
Signed-off-by: Dooyong Kim <[email protected]>
0ctopus13prime
force-pushed
the
loc-block-old-index
branch
from
3ea1ec7 to
ebe486f
Compare
Signed-off-by: Doo Yong Kim <[email protected]>
4 participants
Description
For old indices created before 2.17, will use its old codec where returning null as VectorReader leading NPE issue when memory optimized search is enabled.
This fix PR is adding index created check to return
falsefor isMemoryOptimizedSearchEnabled so that the flow can fallback to default logic which will then delegate C++ to load vectors into off-heap.For indices created after 2.18+, will use the codec has LuceneOnFaiss implementation, will delegate it to search on Faiss index.
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
#2917
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.