Add RemoteDirectory instance to Store as a secondary directory by sachinpkale · Pull Request #3285 · opensearch-project/OpenSearch

sachinpkale · 2022-05-11T11:44:40Z

Signed-off-by: Sachin Kale kalsac@amazon.com

Description

This change adds another Directory instance to Store. With this change, Store will have two directories: primaryDirectory and secondaryDirectory. The idea is to encapsulate RemoteDirectory in Store instead of creating a separate instance of Store, remoteStore.
Discussing both the approaches here: [Remote Segment Store] Add another instance of Store to InternalEngine that uses RemoteDirectory #2994 (comment)

Issues Resolved

[Remote Segment Store] Add another instance of Store to InternalEngine that uses RemoteDirectory #2994

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

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.

sachinpkale · 2022-05-11T11:49:37Z

server/src/main/java/org/opensearch/index/store/Store.java

2 extra constructor methods due to secondaryDirectory. Should we use a builder instead?

Should we use a builder instead?

Yes, and the builder should validate the arguments and throw a RuntimeException if the REMOTE_STORE feature flag is not set.

In the latest commit, I have removed extra parameter to Store. So, this is not required anymore.

sachinpkale · 2022-05-11T11:54:03Z

Fixing spotless checks.

opensearch-ci-bot · 2022-05-11T12:09:33Z

❌ Gradle Check failure 399f1dff56b54dc3010b29e46b523e29c478ed9d
Log 5230

Reports 5230

sachinpkale · 2022-05-11T12:22:51Z

server/src/main/java/org/opensearch/index/IndexService.java

+            Directory remoteDirectory = null;
+            if (this.indexSettings.isRemoteStoreEnabled()) {
+                try {
+                    Repository repository = repositoriesService.repository("dragon-stone");


This repository name is hardcoded for now. It would be made configurable as a part of #3286

opensearch-ci-bot · 2022-05-11T12:35:32Z

✅ Gradle Check success ce5f6cd
Log 5231

Reports 5231

sachinpkale · 2022-05-11T12:40:59Z

Please refer this comment to understand why we went with having secondaryDirectory in the same Store instead of having a separate Store instance.

Bukhtawar · 2022-05-11T12:50:48Z

server/src/main/java/org/opensearch/index/store/Store.java

+        if (secondaryDirectory != null) {
+            ByteSizeCachingDirectory sizeCachingSecondaryDir = new ByteSizeCachingDirectory(secondaryDirectory, refreshInterval);
+            this.secondaryDirectory = new StoreDirectory(sizeCachingSecondaryDir, Loggers.getLogger("index.store.deletes", shardId));
+        } else {
+            this.secondaryDirectory = null;
+        }


I think we should we have a Directory factory instead instead of always having two directories always. Primary and secondary directory sounds confusing as well

What is your view on having a separate store instance with RemoteDirectory?

The presence of branching logic with primary / secondary is confusing, please consider subclassing the Store (fe CachedStore) to have a clean separation between regular Store and Store backed by a cache / secondary.

As I think more on the comments from @Bukhtawar and @eirsep, we may need to have a CompositeDirectory which will have 2 directories (we can create compositions for multiple directories if required). CompositeDirectory will be abstract and its implementations can define how to handle operations on the encapsulated directories. Implementation can be parallel or sequential.

Going forward I do feel a need to have an abstraction at a Store level, espl with #2900 where we might extend search functionality directly on a remote store. Then we have to perform StoreRecovery operations when the shard data is lost, integrity, checksums which is what the store abstraction provides. How about a CompositeStore with each Store based out of their corresponding Directory implementation and IndexShard interfacing with this CompositeStore

As I think more, going either with CompositeStore or CompositeDirectory would be too early. I was able to get a quick implementation out with CompositeDirectory but it would require most of the RemoteDirectory methods to be implemented before we can actually start using it. Using these abstractions will not avoid having a reconciliation mechanism between local and remote directory. I made some changes in the latest commit and Store is not changed in the latest change. Please take a look.

eirsep · 2022-05-11T12:16:45Z

server/src/main/java/org/opensearch/index/IndexService.java

+                    remoteDirectory = remoteDirectoryFactory.newDirectory(this.indexSettings, path, repository);
+                } catch (RepositoryMissingException e) {
+                    throw new IllegalArgumentException(
+                        "Repository should be created before creating index with remote_store enabled setting",


Is repository created by user or by system?

As of now, it is created by user. Once we make the name configurable as a part of #3286, we can check if the repository can be created during bootstrap.

So, you're saying as of now it will have to be created by user but he can only name it as "dragon-stone"? That doesn't seem correct right?

It is hard-coded in this commit. As part of Remote Store V1, we are tracking meta issue: #2992. One of the tasks in the meta takes care of making repository name configurable. #3286

eirsep · 2022-05-11T12:58:36Z

server/src/main/java/org/opensearch/index/store/Store.java


    private final AtomicBoolean isClosed = new AtomicBoolean(false);
    private final StoreDirectory directory;
+    private final StoreDirectory secondaryDirectory;


can store not have to worry about how many directories are there and just have one StoreDirectory which is extended by a Remote+Local store Directory wrapper?

#3285 (comment)

Is secondaryDirectory just a cache? It looks like it, so why not to call it as such?

Please check the latest changes. I have removed reference to secondaryDirectory from the Store.

nknize

This needs to incorporate the REMOTE_STORE feature flag and throw a RuntimeException if the system is trying to bootstrap the remote directories with the feature flag disabled. Bypassing the feature flag here is trappy.

nknize · 2022-05-11T17:16:59Z

server/src/main/java/org/opensearch/index/store/Store.java

Should we use a builder instead?

Yes, and the builder should validate the arguments and throw a RuntimeException if the REMOTE_STORE feature flag is not set.

nknize · 2022-05-12T12:52:51Z

server/src/test/java/org/opensearch/index/store/RemoteDirectoryFactoryTests.java

+import java.util.Collections;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.*;


No wildcard imports

Replaced with individual imports. I will check if we can add this check to spotless

sachinpkale · 2022-05-13T19:58:48Z

This needs to incorporate the REMOTE_STORE feature flag and throw a RuntimeException if the system is trying to bootstrap the remote directories with the feature flag disabled. Bypassing the feature flag here is trappy.

Yes, while creating instance of RemoteDirectory, I am checking if remote_store setting is enabled or not. As the setting itself is not active if feature flag is not set, we don't need explicit feature flag check, right?

…hListener Signed-off-by: Sachin Kale <kalsac@amazon.com>

sachinpkale · 2022-05-13T20:12:59Z

server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java

+
+    @Override
+    public void afterRefresh(boolean didRefresh) throws IOException {
+        // ToDo Add implementation


Actual implementation will be added in the next commit

opensearch-ci-bot · 2022-05-13T20:24:28Z

✅ Gradle Check success d13a660cf4f666d36f222c3b54f2b2f00f5b953b
Log 5313

Reports 5313

opensearch-ci-bot · 2022-05-13T20:53:23Z

❌ Gradle Check failure 207941d
Log 5317

Reports 5317

sachinpkale · 2022-05-13T20:56:20Z

yamlRestTest failed which is a flaky test.

Bukhtawar · 2022-05-16T04:59:29Z

start gradle check

opensearch-ci-bot · 2022-05-16T05:28:54Z

✅ Gradle Check success 207941d
Log 5353

Reports 5353

Bukhtawar · 2022-05-16T13:27:24Z

server/src/main/java/org/opensearch/index/store/RemoteDirectoryFactory.java

+public class RemoteDirectoryFactory implements IndexStorePlugin.RemoteDirectoryFactory {
+
+    @Override
+    public Directory newDirectory(IndexSettings indexSettings, ShardPath path, Repository repository) throws IOException {
+        assert repository instanceof BlobStoreRepository : "repository should be instance of BlobStoreRepository";
+        BlobPath blobPath = new BlobPath();
+        blobPath = blobPath.add(indexSettings.getIndex().getName()).add(String.valueOf(path.getShardId().getId()));
+        BlobContainer blobContainer = ((BlobStoreRepository) repository).blobStore().blobContainer(blobPath);
+        return new RemoteDirectory(blobContainer);
+    }
+}


Is the factory supposed to vend out another variation based on the inputs? If not do we need a factory?

Yes, there will be a directory instance per shard as path will be different per directory.

Wondering if that qualifies to be a factory f.e look at FsDirectoryFactory which creates different a HybridDirectory or NIOFSDirectory based on certain attributes

Got it. You mean, if it is only one variant each time, we do not need a factory, right?
I don't have very strong inclination towards having a factory. It is mostly to keep things similar to what we have with FS based factories. It hides the complexity of understanding all the metadata and creating BlobContainer. Without factory, the code would be in the constructor of RemoteDirectory.

Can we call it a Builder instead?

Bukhtawar · 2022-05-16T13:37:27Z

server/src/main/java/org/opensearch/index/shard/IndexShard.java

+        final List<ReferenceManager.RefreshListener> internalRefreshListener;
+        if (remoteStoreRefreshListener != null && shardRouting.primary()) {
+            internalRefreshListener = Arrays.asList(new RefreshMetricUpdater(refreshMetric), remoteStoreRefreshListener);
+        } else {
+            internalRefreshListener = Collections.singletonList(new RefreshMetricUpdater(refreshMetric));
+        }


Lets push this down to IndexShard and pass a NoopRemoteStoreRefreshListener as needed?

I did not get this. You mean push it down to EngineConfig?

Sorry I meant IndexServices that create IndexShard

is this to avoid null check? As RemoteStoreRefreshListener is injected into IndexShard, tests can have the NoopRemoteStoreRefreshListener if does not want to actually upload the data.

I am fine with what we have now, but if the checks grow at multiple place we might want to give this a revisit

This is identical to the listener for segrep - here. What about building a list of additional listeners in IndexService and pass directly to IndexShard?

@Bukhtawar Agree, adding a task for the same.
@mch2 Yes, once we merge with SegRep code, we can abstract it out.

…hListener (opensearch-project#3285) Co-authored-by: Sachin Kale <kalsac@amazon.com> Signed-off-by: Sachin Kale <kalsac@amazon.com>

sachinpkale requested review from a team and reta as code owners May 11, 2022 11:44

sachinpkale mentioned this pull request May 11, 2022

[Remote Segment Store] Add another instance of Store to InternalEngine that uses RemoteDirectory #2994

Closed

sachinpkale commented May 11, 2022

View reviewed changes

sachinpkale closed this May 11, 2022

sachinpkale force-pushed the feature/durability-enhancements branch from 399f1df to bcfd328 Compare May 11, 2022 11:58

sachinpkale reopened this May 11, 2022

sachinpkale mentioned this pull request May 11, 2022

[Remote Store] Make repository name configurable #3286

Closed

sachinpkale commented May 11, 2022

View reviewed changes

Bukhtawar reviewed May 11, 2022

View reviewed changes

eirsep reviewed May 11, 2022

View reviewed changes

nknize suggested changes May 11, 2022

View reviewed changes

nknize suggested changes May 12, 2022

View reviewed changes

sachinpkale force-pushed the feature/durability-enhancements branch from ce5f6cd to d13a660 Compare May 13, 2022 19:55

Add RemoteDirectoryFactory and use RemoteDirectory instance in Refres…

207941d

…hListener Signed-off-by: Sachin Kale <kalsac@amazon.com>

sachinpkale force-pushed the feature/durability-enhancements branch from d13a660 to 207941d Compare May 13, 2022 20:11

sachinpkale commented May 13, 2022

View reviewed changes

sachinpkale requested review from Bukhtawar and nknize May 13, 2022 20:28

Bukhtawar reviewed May 16, 2022

View reviewed changes

Bukhtawar approved these changes May 16, 2022

View reviewed changes

Bukhtawar merged commit 4408e6b into opensearch-project:feature/durability-enhancements May 16, 2022

mch2 mentioned this pull request May 16, 2022

Adding CheckpointRefreshListener to trigger when Segment replication is turned on and Primary shard refreshes #3108

Merged

5 tasks

sachinpkale mentioned this pull request May 27, 2022

[Remote Store] Upload segments to remote store post refresh #3460

Merged

5 tasks

Conversation

sachinpkale commented May 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachinpkale commented May 11, 2022

Uh oh!

opensearch-ci-bot commented May 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opensearch-ci-bot commented May 11, 2022

Uh oh!

sachinpkale commented May 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reta May 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eirsep May 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nknize left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachinpkale commented May 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

opensearch-ci-bot commented May 13, 2022

Uh oh!

opensearch-ci-bot commented May 13, 2022

Uh oh!

sachinpkale commented May 13, 2022

Uh oh!

Bukhtawar commented May 16, 2022

Uh oh!

opensearch-ci-bot commented May 16, 2022

Uh oh!

Choose a reason for hiding this comment

sachinpkale commented May 11, 2022 •

edited

Loading

reta May 11, 2022 •

edited

Loading

eirsep May 11, 2022 •

edited

Loading

nknize left a comment •

edited

Loading