Enable concurrent intra merge for HNSW graphs #108164
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
elasticsearchmachine
added
the
needs:triage
mayya-sharipova
added
:Search Relevance/Vectors
|
Pinging @elastic/es-search (Team:Search) |
mayya-sharipova
force-pushed
the
hnsw-concurrent-merge
branch
from
3a2da0b to
81d8248
Compare
Lucene PR apache/lucene#12660 introduced concurrent HNSW merge and PR apache/lucene#13124 made ConcurrentMergeScheduler to provide an intra merge executor. But to enable concurrent intra merge for HNSW graphs we still need to provide numMergeWorker to the codec definition. This PR does the following: - provides numMergeWorker to the HNSW vectors codecs definions, where numMergerWorkes set as a node's property of index.merge.scheduler.max_thread_count - enables concurrent merge only for force merge operations.
mayya-sharipova
force-pushed
the
hnsw-concurrent-merge
branch
from
0456125 to
1ac0e0d
Compare
|
@mayya-sharipova one of the failures is due to your change: One of the test failures is worrisome and indicates a leak @original-brownbear : |
benwtrent
reviewed
May 8, 2024
Comment on lines
+82
to
+92
| // TODO: this is temporarily, remove this override and enable multithreaded merges for all kind of merges | ||
| @Override | ||
| public Executor getIntraMergeExecutor(MergePolicy.OneMerge merge) { | ||
| // Enable multithreaded merges only for force merge operations | ||
| if (merge.getStoreMergeInfo().mergeMaxNumSegments != -1) { | ||
| return super.getIntraMergeExecutor(merge); | ||
| } else { | ||
| return null; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I do not think this should return null. It should instead return a same thread executor.
benwtrent
reviewed
May 8, 2024
| @@ -838,7 +841,7 @@ private abstract static class IndexOptions implements ToXContent { | |||
| this.type = type; | |||
| } | |||
|
|
|||
| abstract KnnVectorsFormat getVectorsFormat(); | |||
| abstract KnnVectorsFormat getVectorsFormat(int mergeThreadCount); | |||
There was a problem hiding this comment.
Something about this really bugs me. The merge thread count is a dynamically updatable value. But doing this, is it really dynamic for the workers in the dense vector field mapper?
javanna
added
Team:Search Relevance
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
ChrisHegarty
force-pushed
the
lucene_snapshot
branch
from
80233a9 to
b527f59
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
apache/lucene#12660 introduced concurrent HNSW merge,
and apache/lucene#13124 made ConcurrentMergeScheduler
to provide an intra merge executor.
But to enable concurrent intra merge for HNSW graphs we still need to provide numMergeWorker to the codec definition.
This PR does the following: