Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

lazy load efSearch #52

Merged
merged 3 commits into from
Feb 16, 2020
Merged

Conversation

vamshin
Copy link
Member

@vamshin vamshin commented Feb 16, 2020

Issue #, if available:
#51

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Member

@jmazanec15 jmazanec15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just have a couple questions. Overall looks good to me

@@ -236,4 +236,8 @@ public void onFileDeleted(Path indexFilePath) {
getInstance().cache.invalidate(indexFilePath.toString());
}
};

private String[] getQueryParams(String indexName) {
return new String[] {"efSearch=" + KNNSettings.getEfSearchParam(indexName)};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain more why there is only 1 query parameter being passed in? Why are M and efConstruction not gotten?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

efSearch is required only during searches. Other algorithm params are needed for indexing.

@@ -87,12 +87,6 @@ public long getIndexSize() {
public void close() {
Lock writeLock = readWriteLock.writeLock();
writeLock.lock();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this no longer needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Bad merge. Will add it back.

Copy link
Member

@jmazanec15 jmazanec15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks!

@vamshin vamshin merged commit dee3bad into opendistro-for-elasticsearch:master Feb 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants