Tune default values for ef_search and ef_construction for better indexing and search performance

[navneet1v]

Description

Changed the default values for ef_search and ef_constrction for enabling better indexing and search latency for vector search

Issues Resolved

#1354

BWC Testing

Manual Testing

Manual Testing was performed by running ./gradlew run on 2.11 branch. After this this script was executed with 2.11 arg.

Once it is completed, I ran the ./gradlew run --preserve-data with updated code. This ensured that old indices are still available and shards are initialized. After this I ran the same script with 2.12 as the parameter.

Automated Testing

Added new BWC test for rolling and restart upgrade to validate index creation and indexing of documents.

In both the testings validated(via logs) that for all the indices the older values of hyper parameters is intact.

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed as per the DCO using --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.

[codecov]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (083ea2b) 85.08% compared to head (bc9a6cf) 85.04%.

Files	Patch %	Lines
...earch/knn/index/util/IndexHyperParametersUtil.java	73.33%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1353      +/-   ##
============================================
- Coverage     85.08%   85.04%   -0.05%     
- Complexity     1242     1251       +9     
============================================
  Files           161      162       +1     
  Lines          5069     5101      +32     
  Branches        473      477       +4     
============================================
+ Hits           4313     4338      +25     
- Misses          551      557       +6     
- Partials        205      206       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[heemin32]

Do we have test result on recall and latency after the change?

[navneet1v]

Do we have test result on recall and latency after the change?

Yes I have the whole results. I will be updating them on the github issue.

[navneet1v]

Do we have test result on recall and latency after the change?

@heemin32 Please check the github issue. All the performance benchmarks are updated: #1354

[jmazanec15]

Nice job. This is a very elegant solution. Have a couple questions/comments about some of the implementation details.

[navneet1v]

Keeping this PR open, as me and @jmazanec15 are doing some deep-dive to understand do we need to serialize indexCreatedVersion here:

k-NN/src/main/java/org/opensearch/knn/index/MethodComponentContext.java

Lines 59 to 70 in 63b8f40

	public MethodComponentContext(StreamInput in) throws IOException {
	this.name = in.readString();
	// Due to backwards compatibility issue, parameters could be null. To prevent any null pointer exceptions,
	// do not read if their are no bytes left is null. Make sure this is in sync with the fellow read method. For
	// more information, refer to https://github.com/opensearch-project/k-NN/issues/353.
	if (in.available() > 0) {
	this.parameters = in.readMap(StreamInput::readString, new ParameterMapValueReader());
	} else {
	this.parameters = null;
	}
	}

. Once we have some consensus on that I will move forward with the PR.

[navneet1v]

Added more integration tests for HNSWPQ where hyper parameters are not set.

[jmazanec15]

LGTM

[ryanbogan]

LGTM!