[Remote Vector Index Build] Add download + indexOuput#write implementation to RemoteIndexBuildStrategy #2554
Conversation
This file contains 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
jed326
requested review from
heemin32,
navneet1v,
VijayanB,
vamshin,
jmazanec15,
naveentatikonda,
junqiu-lei,
martin-gaievski,
ryanbogan,
luyuncheng,
shatejas,
0ctopus13prime and
Vikasht34
as code owners
jed326
force-pushed
the
remote-vector-dev-2
branch
from
1a11805 to
3674aa2
Compare
18 tasks
navneet1v
reviewed
Feb 24, 2025
src/main/java/org/opensearch/knn/index/codec/nativeindex/remote/RemoteIndexBuildStrategy.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/codec/nativeindex/remote/RemoteIndexBuildStrategy.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/codec/nativeindex/remote/RemoteIndexBuildStrategy.java
Outdated
Show resolved
Hide resolved
jed326
force-pushed
the
remote-vector-dev-2
branch
from
3674aa2 to
6d950eb
Compare
jed326
commented
Feb 24, 2025
src/main/java/org/opensearch/knn/index/codec/nativeindex/remote/RemoteIndexBuildStrategy.java
Outdated
Show resolved
Hide resolved
jed326
force-pushed
the
remote-vector-dev-2
branch
2 times, most recently
from
bc0f66a to
c945c88
Compare
navneet1v
reviewed
Feb 25, 2025
src/main/java/org/opensearch/knn/index/codec/nativeindex/remote/RemoteIndexBuildStrategy.java
Outdated
Show resolved
Hide resolved
navneet1v
reviewed
Feb 25, 2025
src/main/java/org/opensearch/knn/index/store/IndexOutputWithBuffer.java
Outdated
Show resolved
Hide resolved
jed326
force-pushed
the
remote-vector-dev-2
branch
from
c945c88 to
6b6c3b1
Compare
rchitale7
reviewed
Feb 25, 2025
src/main/java/org/opensearch/knn/index/codec/nativeindex/remote/RemoteIndexBuildStrategy.java
Outdated
Show resolved
Hide resolved
navneet1v
previously approved these changes
Feb 26, 2025
jed326
force-pushed
the
remote-vector-dev-2
branch
from
6b6c3b1 to
e7afbc4
Compare
navneet1v
reviewed
Feb 26, 2025
| @VisibleForTesting | ||
| public void readFromRepository(String path, IndexOutputWithBuffer indexOutputWithBuffer) throws IOException { |
There was a problem hiding this comment.
should this be public if you are adding @VisibleForTesting
There was a problem hiding this comment.
Ah that's an artifact from rebasing. Since we're implementing an interface now, it has to be public. I will make the VectorRepositoryAccessor package private though.
…ategy Signed-off-by: Jay Deng <[email protected]>
jed326
force-pushed
the
remote-vector-dev-2
branch
from
e7afbc4 to
6187dfb
Compare
navneet1v
approved these changes
Feb 26, 2025
shatejas
approved these changes
Feb 27, 2025
| * @see IndexOutputWithBuffer#writeFromStreamWithBuffer(InputStream, byte[]) | ||
| */ | ||
| public void writeFromStreamWithBuffer(InputStream inputStream) throws IOException { | ||
| writeFromStreamWithBuffer(inputStream, this.buffer); |
There was a problem hiding this comment.
nit: Not sure why we need to wrap the method again
There was a problem hiding this comment.
I can remove this if you think it would be better, but the idea is that the buffer we use for downloading graph files for remote index builds is not necessarily the same buffer as the one we use to buffer bytes out of native memory.
As a part of #2464 we will tune this buffer size (as well as the other ones used in the upload path), but for now we just use the same buffer as a starting point.
There was a problem hiding this comment.
See previous discussion here: #2554 (comment)
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.
Description
As a follow-up to #2550, this PR includes the implementation + testing for downloading a graph file from the remote repository and then writing to an
IndexOutput.The key change here is that in order to manage memory consumption, we use a buffer of size 64kb, the same as used in the
IndexOutputWithBufferclass, to buffer the graph file from the remote repository into theIndexOutput, else we will go OOM on the download of larger graph files.For now, we are only using sequential download for the graph file. Parallel download of the graph file has several difficulties associated with it, both on the download side as well as on the writing to
IndexOutputside. For more details on these, see: #2464. We take on parallel blob download as a future improvement.Related Issues
Relates #2465
Relates #2392
Relates #2391
Check List
- [ ] New functionality has been documented.- [ ] API changes companion pull request created.--signoff.- [ ] Public documentation issue/PR created.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.