Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Backport 2.x] [Writable Warm] Composite Directory implementation and integrating it with FileCache #14489

Merged
merged 2 commits into from
Jun 24, 2024

Conversation

rayshrey
Copy link
Contributor


Description

Raising a manual back-port request for #12782

Related Issues

Resolves #12781

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

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.

… with FileCache (opensearch-project#12782)

* Composite Directory POC

Signed-off-by: Shreyansh Ray <[email protected]>

* Refactor TransferManager interface to RemoteStoreFileTrackerAdapter

Signed-off-by: Shreyansh Ray <[email protected]>

* Implement block level fetch for Composite Directory

Signed-off-by: Shreyansh Ray <[email protected]>

* Removed CACHE state from FileTracker

Signed-off-by: Shreyansh Ray <[email protected]>

* Fixes after latest pull

Signed-off-by: Shreyansh Ray <[email protected]>

* Add new setting for warm, remove store type setting, FileTracker and RemoteStoreFileTrackerAdapter, CompositeDirectoryFactory and update Composite Directory implementation

Signed-off-by: Shreyansh Ray <[email protected]>

* Modify TransferManager - replace BlobContainer with Functional Interface to fetch an InputStream instead

Signed-off-by: Shreyansh Ray <[email protected]>

* Reuse OnDemandBlockSnapshotIndexInput instead of OnDemandBlockCompositeIndexInput

Signed-off-by: Shreyansh Ray <[email protected]>

* Modify constructors to avoid breaking public api contract and code review fixes

Signed-off-by: Shreyansh Ray <[email protected]>

* Add experimental annotations for newly created classes and review comment fixes

Signed-off-by: Shreyansh Ray <[email protected]>

* Use ref count as a temporary measure to prevent file from eviction until uploaded to Remote

Signed-off-by: Shreyansh Ray <[email protected]>

* Remove method level locks

Signed-off-by: Shreyansh Ray <[email protected]>

* Handle tmp file deletion

Signed-off-by: Shreyansh Ray <[email protected]>

* Nit fixes

Signed-off-by: Shreyansh Ray <[email protected]>

* Handle delete and close in Composite Directory, log current state of FileCache and correct it's clear method and modify unit and integration tests as per review comments

Signed-off-by: Shreyansh Ray <[email protected]>

* Refactor usages of WRITEABLE_REMOTE_INDEX_SETTING to TIERED_REMOTE_INDEX_SETTING

Signed-off-by: Shreyansh Ray <[email protected]>

* Add tests for FileCachedIndexInput and review comment fixes

Signed-off-by: Shreyansh Ray <[email protected]>

* Add additional IT for feature flag disabled

Signed-off-by: Shreyansh Ray <[email protected]>

* Move setting for Partial Locality type behind Feature Flag, fix bug for ref count via cloneMap in FullFileCachedIndexInput and other review fixes

Signed-off-by: Shreyansh Ray <[email protected]>

* Minor test and nit fixes

Signed-off-by: Shreyansh Ray <[email protected]>

* Add javadocs for FullFileCachedIndexInput

Signed-off-by: Shreyansh Ray <[email protected]>

* Minor precommit fixes

Signed-off-by: Shreyansh Ray <[email protected]>

---------

Signed-off-by: Shreyansh Ray <[email protected]>
Copy link
Contributor

❌ Gradle check result for b42a50e: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@sohami
Copy link
Collaborator

sohami commented Jun 21, 2024

❌ Gradle check result for b42a50e: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@rayshrey There are failed test which is being added by this PR. Can you please check on that. We will need to fix the test first before merging this PR.

org.opensearch.remotestore.WritableWarmIT.testWritableWarmFeatureFlagDisabled

Copy link
Contributor

❌ Gradle check result for 9e678a3: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Copy link
Contributor

✅ Gradle check result for e6c3b57: SUCCESS

Copy link

codecov bot commented Jun 24, 2024

Codecov Report

Attention: Patch coverage is 75.17241% with 72 lines in your changes missing coverage. Please review.

Project coverage is 71.25%. Comparing base (0dd892c) to head (e6c3b57).
Report is 412 commits behind head on 2.x.

Files Patch % Lines
...org/opensearch/index/store/CompositeDirectory.java 81.48% 13 Missing and 7 partials ⚠️
...earch/index/store/remote/utils/cache/LRUCache.java 33.33% 10 Missing ⚠️
...c/main/java/org/opensearch/index/IndexService.java 36.36% 6 Missing and 1 partial ⚠️
...index/store/remote/utils/cache/SegmentedCache.java 14.28% 6 Missing ⚠️
...search/index/store/remote/filecache/FileCache.java 16.66% 5 Missing ⚠️
...ore/remote/filecache/FullFileCachedIndexInput.java 85.71% 4 Missing and 1 partial ⚠️
...search/index/store/remote/utils/FileTypeUtils.java 33.33% 4 Missing ⚠️
server/src/main/java/org/opensearch/node/Node.java 50.00% 0 Missing and 3 partials ⚠️
...rc/main/java/org/opensearch/index/IndexModule.java 89.47% 1 Missing and 1 partial ⚠️
...search/index/shard/RemoteStoreRefreshListener.java 33.33% 1 Missing and 1 partial ⚠️
... and 6 more
Additional details and impacted files
@@             Coverage Diff              @@
##                2.x   #14489      +/-   ##
============================================
- Coverage     71.28%   71.25%   -0.04%     
- Complexity    60145    62141    +1996     
============================================
  Files          4957     5101     +144     
  Lines        282799   293006   +10207     
  Branches      41409    42700    +1291     
============================================
+ Hits         201591   208772    +7181     
- Misses        64189    66654    +2465     
- Partials      17019    17580     +561     

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

@sohami sohami merged commit d56d8c8 into opensearch-project:2.x Jun 24, 2024
32 checks passed
kkewwei pushed a commit to kkewwei/OpenSearch that referenced this pull request Jul 24, 2024
… integrating it with FileCache (opensearch-project#14489)

* [Writable Warm] Composite Directory implementation and integrating it with FileCache (opensearch-project#12782)

* Composite Directory POC

Signed-off-by: Shreyansh Ray <[email protected]>

* Refactor TransferManager interface to RemoteStoreFileTrackerAdapter

Signed-off-by: Shreyansh Ray <[email protected]>

* Implement block level fetch for Composite Directory

Signed-off-by: Shreyansh Ray <[email protected]>

* Removed CACHE state from FileTracker

Signed-off-by: Shreyansh Ray <[email protected]>

* Fixes after latest pull

Signed-off-by: Shreyansh Ray <[email protected]>

* Add new setting for warm, remove store type setting, FileTracker and RemoteStoreFileTrackerAdapter, CompositeDirectoryFactory and update Composite Directory implementation

Signed-off-by: Shreyansh Ray <[email protected]>

* Modify TransferManager - replace BlobContainer with Functional Interface to fetch an InputStream instead

Signed-off-by: Shreyansh Ray <[email protected]>

* Reuse OnDemandBlockSnapshotIndexInput instead of OnDemandBlockCompositeIndexInput

Signed-off-by: Shreyansh Ray <[email protected]>

* Modify constructors to avoid breaking public api contract and code review fixes

Signed-off-by: Shreyansh Ray <[email protected]>

* Add experimental annotations for newly created classes and review comment fixes

Signed-off-by: Shreyansh Ray <[email protected]>

* Use ref count as a temporary measure to prevent file from eviction until uploaded to Remote

Signed-off-by: Shreyansh Ray <[email protected]>

* Remove method level locks

Signed-off-by: Shreyansh Ray <[email protected]>

* Handle tmp file deletion

Signed-off-by: Shreyansh Ray <[email protected]>

* Nit fixes

Signed-off-by: Shreyansh Ray <[email protected]>

* Handle delete and close in Composite Directory, log current state of FileCache and correct it's clear method and modify unit and integration tests as per review comments

Signed-off-by: Shreyansh Ray <[email protected]>

* Refactor usages of WRITEABLE_REMOTE_INDEX_SETTING to TIERED_REMOTE_INDEX_SETTING

Signed-off-by: Shreyansh Ray <[email protected]>

* Add tests for FileCachedIndexInput and review comment fixes

Signed-off-by: Shreyansh Ray <[email protected]>

* Add additional IT for feature flag disabled

Signed-off-by: Shreyansh Ray <[email protected]>

* Move setting for Partial Locality type behind Feature Flag, fix bug for ref count via cloneMap in FullFileCachedIndexInput and other review fixes

Signed-off-by: Shreyansh Ray <[email protected]>

* Minor test and nit fixes

Signed-off-by: Shreyansh Ray <[email protected]>

* Add javadocs for FullFileCachedIndexInput

Signed-off-by: Shreyansh Ray <[email protected]>

* Minor precommit fixes

Signed-off-by: Shreyansh Ray <[email protected]>

---------

Signed-off-by: Shreyansh Ray <[email protected]>

* Fix Writable Warm test for feature flag disabled condition by changing exception type caught

Signed-off-by: Shreyansh Ray <[email protected]>

---------

Signed-off-by: Shreyansh Ray <[email protected]>
Signed-off-by: kkewwei <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement or improvement to existing feature or request Roadmap:Cost/Performance/Scale Project-wide roadmap label Storage:Remote
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants