[Searchable Snapshots] Remove virtual file and fix duplicate clone #6345
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
A "virtual file" in this context is a small file whose entire contents is stored in the snapshot metadata as opposed to in a discrete file in the remote repository. OnDemandVirtualFileSnapshotIndexInput was a complicated wrapper that ultimately returned a ByteArrayIndexInput wrapping the file contents pulled from the metadata data. This change simplifies things a lot and just creates the ByteArrayIndexInput directly. The other change (which led to removal of the virtual file) is to remove a duplicate clone() call of the index input. The file cache design calls for keeping the "origin" IndexInput instance in the cache and always [returning clones][1]. OnDemandBlockIndexInput was incorrectly duplicating this clone operation. [1]: https://github.com/opensearch-project/OpenSearch/blob/0ca51a774211184835c4825dfeff38b23198352e/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java#L105 Signed-off-by: Andrew Ross <andrross@amazon.com>
andrross
requested review from
reta,
anasalkouz,
Bukhtawar,
CEHENKLE,
dblock,
gbbafna,
setiah,
kartg,
kotwanikunal,
mch2,
nknize,
owaiskazi19,
adnapibar,
Rishikesh1159,
ryanbogan,
saratvemulapalli,
shwetathareja,
dreamer-89,
tlfeng,
VachaShah and
xuezhou25
as code owners
kotwanikunal
approved these changes
Feb 16, 2023
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #6345 +/- ##
============================================
- Coverage 70.73% 70.71% -0.02%
+ Complexity 59024 58995 -29
============================================
Files 4800 4799 -1
Lines 282453 282421 -32
Branches 40718 40717 -1
============================================
- Hits 199799 199727 -72
- Misses 66259 66316 +57
+ Partials 16395 16378 -17
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
WhiteSource check is failing for existing issues. No dependencies changed in this commit, so I'm going to merge. |
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Feb 16, 2023
A "virtual file" in this context is a small file whose entire contents is stored in the snapshot metadata as opposed to in a discrete file in the remote repository. OnDemandVirtualFileSnapshotIndexInput was a complicated wrapper that ultimately returned a ByteArrayIndexInput wrapping the file contents pulled from the metadata data. This change simplifies things a lot and just creates the ByteArrayIndexInput directly. The other change (which led to removal of the virtual file) is to remove a duplicate clone() call of the index input. The file cache design calls for keeping the "origin" IndexInput instance in the cache and always [returning clones][1]. OnDemandBlockIndexInput was incorrectly duplicating this clone operation. [1]: https://github.com/opensearch-project/OpenSearch/blob/0ca51a774211184835c4825dfeff38b23198352e/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java#L105 Signed-off-by: Andrew Ross <andrross@amazon.com> (cherry picked from commit 1cdff3b) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
andrross
pushed a commit
that referenced
this pull request
Feb 16, 2023
A "virtual file" in this context is a small file whose entire contents is stored in the snapshot metadata as opposed to in a discrete file in the remote repository. OnDemandVirtualFileSnapshotIndexInput was a complicated wrapper that ultimately returned a ByteArrayIndexInput wrapping the file contents pulled from the metadata data. This change simplifies things a lot and just creates the ByteArrayIndexInput directly. The other change (which led to removal of the virtual file) is to remove a duplicate clone() call of the index input. The file cache design calls for keeping the "origin" IndexInput instance in the cache and always [returning clones][1]. OnDemandBlockIndexInput was incorrectly duplicating this clone operation. [1]: https://github.com/opensearch-project/OpenSearch/blob/0ca51a774211184835c4825dfeff38b23198352e/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java#L105 (cherry picked from commit 1cdff3b) Signed-off-by: Andrew Ross <andrross@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
andrross
added a commit
to andrross/OpenSearch
that referenced
this pull request
Feb 18, 2023
In PR opensearch-project#6345 I did remove a duplicate clone, however this resulted in cloning the IndexInput in the wrong place. When requesting a file that needs to be downloaded, we have a mechanism to ensure that concurrent calls do not end up duplicating the download, which results in multiple threads being given the same instance. The clone must happen _after_ this point to ensure that each thread gets its own clone. Signed-off-by: Andrew Ross <andrross@amazon.com>
andrross
added a commit
to andrross/OpenSearch
that referenced
this pull request
Feb 18, 2023
In PR opensearch-project#6345 I did remove a duplicate clone, however this resulted in cloning the IndexInput in the wrong place. When requesting a file that needs to be downloaded, we have a mechanism to ensure that concurrent calls do not end up duplicating the download, which results in multiple threads being given the same instance. The clone must happen _after_ this point to ensure that each thread gets its own clone. Signed-off-by: Andrew Ross <andrross@amazon.com>
andrross
added a commit
to andrross/OpenSearch
that referenced
this pull request
Feb 18, 2023
In PR opensearch-project#6345 I did remove a duplicate clone, however this resulted in cloning the IndexInput in the wrong place. When requesting a file that needs to be downloaded, we have a mechanism to ensure that concurrent calls do not end up duplicating the download, which results in multiple threads being given the same instance. The clone must happen _after_ this point to ensure that each thread gets its own clone. Signed-off-by: Andrew Ross <andrross@amazon.com>
andrross
added a commit
to andrross/OpenSearch
that referenced
this pull request
Feb 18, 2023
In PR opensearch-project#6345 I did remove a duplicate clone, however this resulted in cloning the IndexInput in the wrong place. When requesting a file that needs to be downloaded, we have a mechanism to ensure that concurrent calls do not end up duplicating the download, which results in multiple threads being given the same instance. The clone must happen _after_ this point to ensure that each thread gets its own clone. Signed-off-by: Andrew Ross <andrross@amazon.com>
andrross
added a commit
to andrross/OpenSearch
that referenced
this pull request
Feb 18, 2023
In PR opensearch-project#6345 I did remove a duplicate clone, however this resulted in cloning the IndexInput in the wrong place. When requesting a file that needs to be downloaded, we have a mechanism to ensure that concurrent calls do not end up duplicating the download, which results in multiple threads being given the same instance. The clone must happen _after_ this point to ensure that each thread gets its own clone. Signed-off-by: Andrew Ross <andrross@amazon.com>
andrross
added a commit
to andrross/OpenSearch
that referenced
this pull request
Feb 18, 2023
In PR opensearch-project#6345 I did remove a duplicate clone, however this resulted in cloning the IndexInput in the wrong place. When requesting a file that needs to be downloaded, we have a mechanism to ensure that concurrent calls do not end up duplicating the download, which results in multiple threads being given the same instance. The clone must happen _after_ this point to ensure that each thread gets its own clone. Signed-off-by: Andrew Ross <andrross@amazon.com>
andrross
added a commit
to andrross/OpenSearch
that referenced
this pull request
Feb 18, 2023
In PR opensearch-project#6345 I did remove a duplicate clone, however this resulted in cloning the IndexInput in the wrong place. When requesting a file that needs to be downloaded, we have a mechanism to ensure that concurrent calls do not end up duplicating the download, which results in multiple threads being given the same instance. The clone must happen _after_ this point to ensure that each thread gets its own clone. Signed-off-by: Andrew Ross <andrross@amazon.com>
6 tasks
andrross
added a commit
to andrross/OpenSearch
that referenced
this pull request
Feb 20, 2023
In PR opensearch-project#6345 I did remove a duplicate clone, however this resulted in cloning the IndexInput in the wrong place. When requesting a file that needs to be downloaded, we have a mechanism to ensure that concurrent calls do not end up duplicating the download, which results in multiple threads being given the same instance. The clone must happen _after_ this point to ensure that each thread gets its own clone. Signed-off-by: Andrew Ross <andrross@amazon.com>
andrross
added a commit
to andrross/OpenSearch
that referenced
this pull request
Feb 20, 2023
In PR opensearch-project#6345 I did remove a duplicate clone, however this resulted in cloning the IndexInput in the wrong place. When requesting a file that needs to be downloaded, we have a mechanism to ensure that concurrent calls do not end up duplicating the download, which results in multiple threads being given the same instance. The clone must happen _after_ this point to ensure that each thread gets its own clone. Signed-off-by: Andrew Ross <andrross@amazon.com>
andrross
added a commit
to andrross/OpenSearch
that referenced
this pull request
Feb 20, 2023
In PR opensearch-project#6345 I did remove a duplicate clone, however this resulted in cloning the IndexInput in the wrong place. When requesting a file that needs to be downloaded, we have a mechanism to ensure that concurrent calls do not end up duplicating the download, which results in multiple threads being given the same instance. The clone must happen _after_ this point to ensure that each thread gets its own clone. Signed-off-by: Andrew Ross <andrross@amazon.com>
andrross
added a commit
that referenced
this pull request
Feb 20, 2023
In PR #6345 I did remove a duplicate clone, however this resulted in cloning the IndexInput in the wrong place. When requesting a file that needs to be downloaded, we have a mechanism to ensure that concurrent calls do not end up duplicating the download, which results in multiple threads being given the same instance. The clone must happen _after_ this point to ensure that each thread gets its own clone. Signed-off-by: Andrew Ross <andrross@amazon.com>
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Feb 20, 2023
In PR #6345 I did remove a duplicate clone, however this resulted in cloning the IndexInput in the wrong place. When requesting a file that needs to be downloaded, we have a mechanism to ensure that concurrent calls do not end up duplicating the download, which results in multiple threads being given the same instance. The clone must happen _after_ this point to ensure that each thread gets its own clone. Signed-off-by: Andrew Ross <andrross@amazon.com> (cherry picked from commit 5e5c83b) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
andrross
pushed a commit
that referenced
this pull request
Feb 20, 2023
In PR #6345 I did remove a duplicate clone, however this resulted in cloning the IndexInput in the wrong place. When requesting a file that needs to be downloaded, we have a mechanism to ensure that concurrent calls do not end up duplicating the download, which results in multiple threads being given the same instance. The clone must happen _after_ this point to ensure that each thread gets its own clone. (cherry picked from commit 5e5c83b) Signed-off-by: Andrew Ross <andrross@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.
A "virtual file" in this context is a small file whose entire contents is stored in the snapshot metadata as opposed to in a discrete file in the remote repository. OnDemandVirtualFileSnapshotIndexInput was a complicated wrapper that ultimately returned a ByteArrayIndexInput wrapping the file contents pulled from the metadata data. This change simplifies things a lot and just creates the ByteArrayIndexInput directly.
The other change (which led to removal of the virtual file) is to remove a duplicate clone() call of the index input. The file cache design calls for keeping the "origin" IndexInput instance in the cache and always returning clones. OnDemandBlockIndexInput was incorrectly duplicating this clone operation.
Check List
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.