[Draft] Refactor remote store within existing index shard store

[kotwanikunal]

Description

• Testing changes for [Searchable Remote Index] Create a prototype for CompositeStore combining remote and local store in tandem with the HybridDirectory #7763

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff
• Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE ❕
• TEST FAILURES:

      1 org.opensearch.snapshots.DedicatedClusterSnapshotRestoreIT.testIndexDeletionDuringSnapshotCreationInQueue
      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchTaskCancellationWithHighCpu
      1 org.opensearch.remotestore.RemoteStoreRefreshListenerIT.testRemoteRefreshRetryOnFailure

• URL: https://build.ci.opensearch.org/job/gradle-check/16785/
• CommitID: 45d94d6
  Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov]

Codecov Report

Merging #7904 (5eae0a7) into main (aa21585) will increase coverage by 0.03%.
The diff coverage is 82.35%.

@@             Coverage Diff              @@
##               main    #7904      +/-   ##
============================================
+ Coverage     70.78%   70.82%   +0.03%     
- Complexity    56251    56264      +13     
============================================
  Files          4689     4689              
  Lines        266304   266305       +1     
  Branches      39086    39086              
============================================
+ Hits         188505   188610     +105     
+ Misses        61847    61765      -82     
+ Partials      15952    15930      -22     

Impacted Files	Coverage Δ
...java/org/opensearch/index/shard/StoreRecovery.java	66.25% <0.00%> (-0.94%)	⬇️
...c/main/java/org/opensearch/index/IndexService.java	74.66% <66.66%> (+0.84%)	⬆️
...in/java/org/opensearch/index/shard/IndexShard.java	70.15% <66.66%> (+0.73%)	⬆️
...search/index/shard/RemoteStoreRefreshListener.java	84.95% <100.00%> (ø)
...rc/main/java/org/opensearch/index/store/Store.java	79.21% <100.00%> (-0.73%)	⬇️

... and 434 files with indirect coverage changes

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: UNSTABLE ❕
• TEST FAILURES:

      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchShardTaskCancellationWithHighCpu

• URL: https://build.ci.opensearch.org/job/gradle-check/16859/
• CommitID: 5eae0a7
  Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[kotwanikunal]

@Bukhtawar / @sachinpkale / @andrross
I was working through the current remote store structure and I realized we only utilize directory object from the store to perform remote store operations.
In this prototype, I instead refactored the current store structure to have an additional remoteDirectory which can work in tandem with the localDirectory.

The relation would be as follows -

• An index shard will have a store. (1:1)
• A store can encapsulate the local and remote directory or any new directory implementations (1:M)

We can further enhance the store operations to perform operations between directories, for example, in case of a writeable index perform local + remote writes within a single operation.

Thoughts?

[andrross]

Is there any way to avoid doing all these casts? We're plumbing in the explicit concept of a remote directory into the store, so can it store and expose that directory as an explicit remote directory type to avoid the need for the consumers to unwrap and cast?

This really isn't following the principles of object oriented programming because even though the type is exposed as the a generic Directory, the consumers require this to be a very specific type (a FilterDirectory wrapping a FilterDirectory wrapping a RemoteSegmentStoreDirectory) or else it will fail at runtime. If the consumers can't be made agnostic to the specific type, then it would probably be better to be explicit about the type and let the compiler enforce it.

[sachinpkale]

Agree, I think this is just a workaround as RemoteDirectory does not support all the methods in the directory. We need to get away from this and should be used as a drop-in replacement for any directory.

[Bukhtawar]

@Bukhtawar / @sachinpkale / @andrross I was working through the current remote store structure and I realized we only utilize directory object from the store to perform remote store operations. In this prototype, I instead refactored the current store structure to have an additional remoteDirectory which can work in tandem with the localDirectory.

The relation would be as follows -

• An index shard will have a store. (1:1)
• A store can encapsulate the local and remote directory or any new directory implementations (1:M)

We can further enhance the store operations to perform operations between directories, for example, in case of a writeable index perform local + remote writes within a single operation.

Thoughts?

I think there was a work item pending from the Remote Store initial implementation days #3719. The reason behind a composite store was to make the intent of local and remote store working in tandem more explicit and not optional(if remote store is enabled)

Why Store and not Directory?
Store encapsulates data and its corresponding metadata and I think we need this encapsulation for local store and remote store going forward

[kotwanikunal]

@Bukhtawar / @sachinpkale / @andrross I was working through the current remote store structure and I realized we only utilize directory object from the store to perform remote store operations. In this prototype, I instead refactored the current store structure to have an additional remoteDirectory which can work in tandem with the localDirectory.
The relation would be as follows -

• An index shard will have a store. (1:1)
• A store can encapsulate the local and remote directory or any new directory implementations (1:M)

We can further enhance the store operations to perform operations between directories, for example, in case of a writeable index perform local + remote writes within a single operation.
Thoughts?

I think there was a work item pending from the Remote Store initial implementation days #3719. The reason behind a composite store was to make the intent of local and remote store working in tandem more explicit and not optional(if remote store is enabled)

Why Store and not Directory? Store encapsulates data and its corresponding metadata and I think we need this encapsulation for local store and remote store going forward

Thanks for the response @Bukhtawar. The metadata seems to be fetched at run time from the directory and I had the proposal to add fetchRemoteXXX variants (sample: fetchRemoteMetadata()) for needed functions which can also make diffXXX() (sample: diffRemoteMetadata()) possible directly within the said single store implementation.

class CompositeStore extends Store
{ 
  private Store localStore;
  private Store remoteStore, 
}

The above pattern adds in a lot of delegation to the local store for most of these functions which seems to be not so ideal IMO.

[Bukhtawar]

I would like us to think more from a cloud native architecture where metadata is free to be stored elsewhere and doesn't have to reside/consume the same Directory interface. The delegation can be handled by making Store abstract and keeping common business logic there. That way remote store has a clear way to extend the same

[sachinpkale]

Why Store and not Directory?
Store encapsulates data and its corresponding metadata and I think we need this encapsulation for local store and remote store going forward

When we started the implementation of remote store for the first time, one approach tried was to include remote directory in the store which is the same approach that is implemented in this PR (I used to call it as primary and secondary directory).

There are two problems with this approach:

• Javadoc of Store reads: A Store provides plain access to files written by an opensearch index shard. This makes Store a kind of wrapper over a directory. If we provide two directories to Store, this assumption breaks. For example: which directory will Store.readLastCommittedSegmentsInfo use?
• The second problem is explicit assumption between Store and 2 directories (local and remote). We still don't answer the question: what if we have a 3rd directory.

IMO, Store should not use more than one directory. Having a HybridDirectory that is passed to the store should help here.

[Bukhtawar]

HybridDirectory is already too overloaded for instance we use it today to choose between NIOFSDirectory and MMapDirectory. So Directory is still a lower level concept and overloading it for the purpose of Local/Remote store interactions might not be the right abstraction we are looking.
At the minimal based on current interactions this is how I feel we should refactor. I have concerns on how the store is being concurrently accessed and how there are methods outside store that decide how the store should recover by directly accessing the underlying Directory.

Any implementation requiring a single Directory like local, read-only searchable snapshots could use SimpleStore while Remote store, writeable/readable stores can use CompositeStore where writer would use a FSDirectory while readers can use any specialisation of Directory

[kotwanikunal]

HybridDirectory is already too overloaded for instance we use it today to choose between NIOFSDirectory and MMapDirectory. So Directory is still a lower level concept and overloading it for the purpose of Local/Remote store interactions might not be the right abstraction we are looking. At the minimal based on current interactions this is how I feel we should refactor. I have concerns on how the store is being concurrently accessed and how there are methods outside store that decide how the store should recover by directly accessing the underlying Directory.

Any implementation requiring a single Directory like local, read-only searchable snapshots could use SimpleStore while Remote store, writeable/readable stores can use CompositeStore where writer would use a FSDirectory while readers can use any specialisation of Directory

Thanks for the detailed info @Bukhtawar!
I have an incremental take on #7960 which is a composite store architecture and I can modify + build towards the above.
I have abstracted out any functions from the Store which use Directory. I have still kept references to the Directory within the Store for an easy plug-in and usage in current code.
But with the initial idea you suggested, we can use a non directory based implementation within the RemoteStore functions now.
The simplification of Store as you have laid out currently will need some additional refactoring.

I do not plan to merge #7960 in since it requires better documentation/tests/productionization and is only incremental in its current form.
But any additional feedback is appreciated.

@sachinpkale / @Bukhtawar / @andrross