From c5b021ba0e3659ca3e8a31254e41268a8e3a850c Mon Sep 17 00:00:00 2001 From: Shreyansh Ray Date: Wed, 13 Mar 2024 16:26:02 +0530 Subject: [PATCH 01/16] Composite Directory POC Signed-off-by: Shreyansh Ray --- .../remotestore/CompositeDirectoryIT.java | 66 ++++++++ .../metadata/MetadataCreateIndexService.java | 12 ++ .../org/opensearch/index/IndexModule.java | 7 +- .../shard/RemoteStoreRefreshListener.java | 5 + .../index/store/CompositeDirectory.java | 152 ++++++++++++++++++ .../store/CompositeDirectoryFactory.java | 52 ++++++ .../CompositeDirectoryTransferManager.java | 81 ++++++++++ .../index/store/TransferManager.java | 32 ++++ .../remote/utils/filetracker/FileState.java | 15 ++ .../utils/filetracker/FileTrackingInfo.java | 27 ++++ .../remote/utils/filetracker/FileType.java | 18 +++ .../main/java/org/opensearch/node/Node.java | 8 +- 12 files changed, 472 insertions(+), 3 deletions(-) create mode 100644 server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java create mode 100644 server/src/main/java/org/opensearch/index/store/CompositeDirectory.java create mode 100644 server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java create mode 100644 server/src/main/java/org/opensearch/index/store/CompositeDirectoryTransferManager.java create mode 100644 server/src/main/java/org/opensearch/index/store/TransferManager.java create mode 100644 server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileState.java create mode 100644 server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileTrackingInfo.java create mode 100644 server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileType.java diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java new file mode 100644 index 0000000000000..abb291644e479 --- /dev/null +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java @@ -0,0 +1,66 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.remotestore; + +import org.apache.lucene.store.Directory; +import org.apache.lucene.store.FilterDirectory; +import org.opensearch.action.admin.indices.get.GetIndexRequest; +import org.opensearch.action.admin.indices.get.GetIndexResponse; +import org.opensearch.cluster.metadata.IndexMetadata; +import org.opensearch.common.settings.Settings; +import org.opensearch.index.IndexModule; +import org.opensearch.index.IndexService; +import org.opensearch.index.shard.IndexShard; +import org.opensearch.index.store.CompositeDirectory; +import org.opensearch.indices.IndicesService; +import org.opensearch.indices.replication.common.ReplicationType; + +import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY; +import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED; +import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY; +import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REPLICATION_TYPE; +import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; + +public class CompositeDirectoryIT extends RemoteStoreBaseIntegTestCase { + public void testCompositeDirectory() throws Exception { + Settings settings = Settings.builder() + .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) + .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), "compositefs") + .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) + .build(); + assertAcked(client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()); + GetIndexResponse getIndexResponse = client().admin() + .indices() + .getIndex(new GetIndexRequest().indices("test-idx-1").includeDefaults(true)) + .get(); + boolean indexServiceFound = false; + String[] nodes = internalCluster().getNodeNames(); + for (String node : nodes) { + IndexService indexService = internalCluster().getInstance(IndicesService.class, node).indexService(resolveIndex("test-idx-1")); + if (indexService == null) { + continue; + } + IndexShard shard = indexService.getShardOrNull(0); + Directory directory = (((FilterDirectory) (((FilterDirectory) (shard.store().directory())).getDelegate())).getDelegate()); + assertTrue(directory instanceof CompositeDirectory); + indexServiceFound = true; + } + assertTrue(indexServiceFound); + Settings indexSettings = getIndexResponse.settings().get("test-idx-1"); + assertEquals(ReplicationType.SEGMENT.toString(), indexSettings.get(SETTING_REPLICATION_TYPE)); + assertEquals("true", indexSettings.get(SETTING_REMOTE_STORE_ENABLED)); + assertEquals(REPOSITORY_NAME, indexSettings.get(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY)); + assertEquals(REPOSITORY_2_NAME, indexSettings.get(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY)); + assertEquals("compositefs", indexSettings.get("index.store.type")); + + ensureGreen("test-idx-1"); + indexData(10, false, "test-idx-1"); + ensureGreen("test-idx-1"); + } +} diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java index 121f8d935cf48..a2a79ae7f379c 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java @@ -75,6 +75,7 @@ import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.xcontent.XContentHelper; import org.opensearch.core.action.ActionListener; import org.opensearch.core.common.Strings; @@ -986,6 +987,7 @@ static Settings aggregateIndexSettings( validateStoreTypeSettings(indexSettings); validateRefreshIntervalSettings(request.settings(), clusterSettings); validateTranslogDurabilitySettings(request.settings(), clusterSettings, settings); + validateCompositeFS(request.settings()); return indexSettings; } @@ -1679,4 +1681,14 @@ static void validateTranslogDurabilitySettings(Settings requestSettings, Cluster } } + + public static void validateCompositeFS(Settings indexSettings) { + if (indexSettings.get(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), "") + .equalsIgnoreCase(IndexModule.Type.COMPOSITEFS.getSettingsKey()) + && !FeatureFlags.isEnabled(FeatureFlags.WRITEABLE_REMOTE_INDEX_SETTING)) { + throw new IllegalArgumentException( + "ERROR - Composite FS store type can be enabled only if Feature Flag for Writable Remote Index is true" + ); + } + } } diff --git a/server/src/main/java/org/opensearch/index/IndexModule.java b/server/src/main/java/org/opensearch/index/IndexModule.java index 3c4cb4fd596c1..16dce2645b2d3 100644 --- a/server/src/main/java/org/opensearch/index/IndexModule.java +++ b/server/src/main/java/org/opensearch/index/IndexModule.java @@ -74,6 +74,7 @@ import org.opensearch.index.shard.IndexingOperationListener; import org.opensearch.index.shard.SearchOperationListener; import org.opensearch.index.similarity.SimilarityService; +import org.opensearch.index.store.CompositeDirectoryFactory; import org.opensearch.index.store.FsDirectoryFactory; import org.opensearch.index.store.remote.directory.RemoteSnapshotDirectoryFactory; import org.opensearch.index.store.remote.filecache.FileCache; @@ -506,7 +507,8 @@ public enum Type { MMAPFS("mmapfs"), SIMPLEFS("simplefs"), FS("fs"), - REMOTE_SNAPSHOT("remote_snapshot"); + REMOTE_SNAPSHOT("remote_snapshot"), + COMPOSITEFS("compositefs"); private final String settingsKey; private final boolean deprecated; @@ -788,6 +790,9 @@ public static Map createBuiltInDirect new RemoteSnapshotDirectoryFactory(repositoriesService, threadPool, remoteStoreFileCache) ); break; + case COMPOSITEFS: + factories.put(type.getSettingsKey(), new CompositeDirectoryFactory(repositoriesService, remoteStoreFileCache)); + break; default: throw new IllegalStateException("No directory factory mapping for built-in type " + type); } diff --git a/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java b/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java index 351aec6e3af6c..5379d21119ad9 100644 --- a/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java +++ b/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java @@ -30,6 +30,7 @@ import org.opensearch.index.engine.InternalEngine; import org.opensearch.index.remote.RemoteSegmentTransferTracker; import org.opensearch.index.seqno.SequenceNumbers; +import org.opensearch.index.store.CompositeDirectory; import org.opensearch.index.store.RemoteSegmentStoreDirectory; import org.opensearch.index.store.remote.metadata.RemoteSegmentMetadata; import org.opensearch.index.translog.Translog; @@ -286,6 +287,10 @@ public void onFailure(Exception e) { // Start the segments files upload uploadNewSegments(localSegmentsPostRefresh, localSegmentsSizeMap, segmentUploadsCompletedListener); + Directory directory = ((FilterDirectory) (((FilterDirectory) storeDirectory).getDelegate())).getDelegate(); + if (directory instanceof CompositeDirectory) { + ((CompositeDirectory) directory).afterSyncToRemote(localSegmentsPostRefresh); + } latch.await(); } catch (EngineException e) { logger.warn("Exception while reading SegmentInfosSnapshot", e); diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java new file mode 100644 index 0000000000000..080b42a2b65a2 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java @@ -0,0 +1,152 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.store; + +import org.apache.lucene.store.FSDirectory; +import org.apache.lucene.store.FilterDirectory; +import org.apache.lucene.store.IOContext; +import org.apache.lucene.store.IndexInput; +import org.apache.lucene.store.IndexOutput; +import org.apache.lucene.store.Lock; +import org.opensearch.common.blobstore.BlobContainer; +import org.opensearch.index.store.remote.filecache.CachedIndexInput; +import org.opensearch.index.store.remote.filecache.FileCache; +import org.opensearch.index.store.remote.utils.filetracker.FileState; +import org.opensearch.index.store.remote.utils.filetracker.FileType; + +import java.io.IOException; +import java.util.Collection; +import java.util.Set; + +public class CompositeDirectory extends FilterDirectory { + + private final FSDirectory localDirectory; + private final TransferManager transferManager; + private final FileCache fileCache; + + public CompositeDirectory(FSDirectory localDirectory, BlobContainer blobContainer, FileCache fileCache) { + super(localDirectory); + this.localDirectory = localDirectory; + this.fileCache = fileCache; + this.transferManager = new CompositeDirectoryTransferManager(fileCache, blobContainer); + } + + @Override + public String[] listAll() throws IOException { + return localDirectory.listAll(); + } + + @Override + public void deleteFile(String name) throws IOException { + super.deleteFile(name); + transferManager.removeFileFromTracker(name); + fileCache.remove(localDirectory.getDirectory().resolve(name)); + } + + @Override + public long fileLength(String name) throws IOException { + return localDirectory.fileLength(name); + } + + @Override + public IndexOutput createOutput(String name, IOContext context) throws IOException { + transferManager.trackFile(name, FileState.DISK, FileType.NON_BLOCK); + return localDirectory.createOutput(name, context); + } + + @Override + public IndexOutput createTempOutput(String prefix, String suffix, IOContext context) throws IOException { + return localDirectory.createTempOutput(prefix, suffix, context); + } + + @Override + public void sync(Collection names) throws IOException { + localDirectory.sync(names); + } + + @Override + public void syncMetaData() throws IOException { + localDirectory.syncMetaData(); + } + + @Override + public void rename(String source, String dest) throws IOException { + localDirectory.rename(source, dest); + transferManager.trackFile(dest, transferManager.getFileState(source), transferManager.getFileType(source)); + transferManager.removeFileFromTracker(source); + } + + @Override + public IndexInput openInput(String name, IOContext context) throws IOException { + if (!transferManager.isFilePresent(name)) { + return localDirectory.openInput(name, context); + } + IndexInput indexInput = null; + switch (transferManager.getFileState(name)) { + case DISK: + indexInput = localDirectory.openInput(name, context); + break; + + case CACHE: + indexInput = fileCache.get(localDirectory.getDirectory().resolve(name)).getIndexInput(); + break; + + case REMOTE_ONLY: + // TODO - return an implementation of OnDemandBlockIndexInput where the fetchBlock method is implemented + break; + } + return indexInput; + } + + @Override + public Lock obtainLock(String name) throws IOException { + return localDirectory.obtainLock(name); + } + + @Override + public void close() throws IOException { + localDirectory.close(); + } + + @Override + public Set getPendingDeletions() throws IOException { + return localDirectory.getPendingDeletions(); + } + + public void afterSyncToRemote(Collection files) throws IOException { + for (String fileName : files) { + if (transferManager.isFilePresent(fileName) && !transferManager.getFileState(fileName).equals(FileState.CACHE)) { + transferManager.updateFileState(fileName, FileState.CACHE); + } + fileCache.put(localDirectory.getDirectory().resolve(fileName), new CachedIndexInput() { + @Override + public IndexInput getIndexInput() throws IOException { + return localDirectory.openInput(fileName, IOContext.READ); + } + + @Override + public long length() { + try { + return localDirectory.fileLength(fileName); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + @Override + public boolean isClosed() { + return false; + } + + @Override + public void close() {} + }); + } + } +} diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java new file mode 100644 index 0000000000000..52095614f56a9 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java @@ -0,0 +1,52 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.store; + +import org.apache.lucene.store.Directory; +import org.apache.lucene.store.FSDirectory; +import org.opensearch.common.blobstore.BlobContainer; +import org.opensearch.common.blobstore.BlobPath; +import org.opensearch.index.IndexSettings; +import org.opensearch.index.shard.ShardPath; +import org.opensearch.index.store.remote.filecache.FileCache; +import org.opensearch.plugins.IndexStorePlugin; +import org.opensearch.repositories.RepositoriesService; +import org.opensearch.repositories.Repository; +import org.opensearch.repositories.blobstore.BlobStoreRepository; + +import java.io.IOException; +import java.nio.file.Path; +import java.util.function.Supplier; + +public class CompositeDirectoryFactory implements IndexStorePlugin.DirectoryFactory { + + private final Supplier repositoriesService; + private final FileCache remoteStoreFileCache; + + public CompositeDirectoryFactory(Supplier repositoriesService, FileCache remoteStoreFileCache) { + this.repositoriesService = repositoriesService; + this.remoteStoreFileCache = remoteStoreFileCache; + } + + @Override + public Directory newDirectory(IndexSettings indexSettings, ShardPath shardPath) throws IOException { + String repositoryName = indexSettings.getRemoteStoreRepository(); + Repository repository = repositoriesService.get().repository(repositoryName); + BlobStoreRepository blobStoreRepository = (BlobStoreRepository) repository; + String shardId = String.valueOf(shardPath.getShardId().getId()); + String indexUUID = indexSettings.getIndex().getUUID(); + BlobPath blobPath = blobStoreRepository.basePath().add(indexUUID).add(shardId).add("segments").add("data"); + final BlobContainer blobContainer = blobStoreRepository.blobStore().blobContainer(blobPath); + + final Path location = shardPath.resolveIndex(); + final FSDirectory primaryDirectory = FSDirectory.open(location); + + return new CompositeDirectory(primaryDirectory, blobContainer, remoteStoreFileCache); + } +} diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryTransferManager.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryTransferManager.java new file mode 100644 index 0000000000000..4e10a6db80db3 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryTransferManager.java @@ -0,0 +1,81 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.store; + +import org.apache.lucene.store.IndexInput; +import org.opensearch.common.blobstore.BlobContainer; +import org.opensearch.index.store.remote.filecache.FileCache; +import org.opensearch.index.store.remote.utils.BlobFetchRequest; +import org.opensearch.index.store.remote.utils.filetracker.FileState; +import org.opensearch.index.store.remote.utils.filetracker.FileTrackingInfo; +import org.opensearch.index.store.remote.utils.filetracker.FileType; + +import java.util.HashMap; +import java.util.Map; + +public class CompositeDirectoryTransferManager implements TransferManager { + + private FileCache fileCache; + private Map fileTracker; + private BlobContainer blobContainer; + + public CompositeDirectoryTransferManager(FileCache fileCache, BlobContainer blobContainer) { + this.fileCache = fileCache; + this.blobContainer = blobContainer; + this.fileTracker = new HashMap<>(); + } + + @Override + public IndexInput fetchBlob(BlobFetchRequest blobFetchRequest) { + // TODO - This function will fetch the requested data from blobContainer + return null; + } + + public void trackFile(String name, FileState fileState, FileType fileType) { + if (!fileTracker.containsKey(name)) { + fileTracker.put(name, new FileTrackingInfo(fileState, fileType)); + } + } + + public void updateFileType(String name, FileType fileType) { + FileTrackingInfo fileTrackingInfo = fileTracker.get(name); + if (fileTrackingInfo != null) { + fileTracker.put(name, new FileTrackingInfo(fileTrackingInfo.getFileState(), fileType)); + } + } + + public void updateFileState(String name, FileState fileState) { + FileTrackingInfo fileTrackingInfo = fileTracker.get(name); + if (fileTrackingInfo != null) { + fileTracker.put(name, new FileTrackingInfo(fileState, fileTrackingInfo.getFileType())); + } + } + + public void removeFileFromTracker(String name) { + fileTracker.remove(name); + } + + public FileState getFileState(String name) { + if (!fileTracker.containsKey(name)) { + return null; + } + return fileTracker.get(name).getFileState(); + } + + public FileType getFileType(String name) { + if (!fileTracker.containsKey(name)) { + return null; + } + return fileTracker.get(name).getFileType(); + } + + public boolean isFilePresent(String name) { + return fileTracker.containsKey(name); + } +} diff --git a/server/src/main/java/org/opensearch/index/store/TransferManager.java b/server/src/main/java/org/opensearch/index/store/TransferManager.java new file mode 100644 index 0000000000000..5029c51a34e34 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/store/TransferManager.java @@ -0,0 +1,32 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.store; + +import org.apache.lucene.store.IndexInput; +import org.opensearch.index.store.remote.utils.BlobFetchRequest; +import org.opensearch.index.store.remote.utils.filetracker.FileState; +import org.opensearch.index.store.remote.utils.filetracker.FileType; + +public interface TransferManager { + IndexInput fetchBlob(BlobFetchRequest blobFetchRequest); + + void trackFile(String name, FileState fileState, FileType fileType); + + void updateFileType(String name, FileType fileType); + + void updateFileState(String name, FileState fileState); + + void removeFileFromTracker(String name); + + FileState getFileState(String name); + + FileType getFileType(String name); + + boolean isFilePresent(String name); +} diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileState.java b/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileState.java new file mode 100644 index 0000000000000..564bb87e03857 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileState.java @@ -0,0 +1,15 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.store.remote.utils.filetracker; + +public enum FileState { + DISK, + CACHE, + REMOTE_ONLY; +} diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileTrackingInfo.java b/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileTrackingInfo.java new file mode 100644 index 0000000000000..60310b0fc39a2 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileTrackingInfo.java @@ -0,0 +1,27 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.store.remote.utils.filetracker; + +public class FileTrackingInfo { + private final FileState fileState; + private final FileType fileType; + + public FileTrackingInfo(FileState fileState, FileType fileType) { + this.fileState = fileState; + this.fileType = fileType; + } + + public FileState getFileState() { + return fileState; + } + + public FileType getFileType() { + return fileType; + } +} diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileType.java b/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileType.java new file mode 100644 index 0000000000000..18506f7062b7c --- /dev/null +++ b/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileType.java @@ -0,0 +1,18 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.store.remote.utils.filetracker; + +public enum FileType { + BLOCK, + NON_BLOCK; + + public boolean isBlockFile(FileType fileType) { + return fileType.equals(FileType.BLOCK); + } +} diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 614f39166ea66..85d7d63a96b03 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -1948,7 +1948,8 @@ DiscoveryNode getNode() { * Else it configures the size to 80% of available capacity for a dedicated search node, if not explicitly defined. */ private void initializeFileCache(Settings settings, CircuitBreaker circuitBreaker) throws IOException { - if (DiscoveryNode.isSearchNode(settings)) { + boolean isWritableRemoteIndexEnabled = FeatureFlags.isEnabled(FeatureFlags.WRITEABLE_REMOTE_INDEX_SETTING); + if (DiscoveryNode.isSearchNode(settings) || isWritableRemoteIndexEnabled) { NodeEnvironment.NodePath fileCacheNodePath = nodeEnvironment.fileCacheNodePath(); long capacity = NODE_SEARCH_CACHE_SIZE_SETTING.get(settings).getBytes(); FsInfo.Path info = ExceptionsHelper.catchAsRuntimeException(() -> FsProbe.getFSInfo(fileCacheNodePath)); @@ -1957,7 +1958,10 @@ private void initializeFileCache(Settings settings, CircuitBreaker circuitBreake // Initialize default values for cache if NODE_SEARCH_CACHE_SIZE_SETTING is not set. if (capacity == 0) { // If node is not a dedicated search node without configuration, prevent cache initialization - if (DiscoveryNode.getRolesFromSettings(settings).stream().anyMatch(role -> !DiscoveryNodeRole.SEARCH_ROLE.equals(role))) { + if (!isWritableRemoteIndexEnabled + && DiscoveryNode.getRolesFromSettings(settings) + .stream() + .anyMatch(role -> !DiscoveryNodeRole.SEARCH_ROLE.equals(role))) { throw new SettingsException( "Unable to initialize the " + DiscoveryNodeRole.SEARCH_ROLE.roleName() From 46a63d5f1eeefc48ce3fd30655a806a8ea4e2735 Mon Sep 17 00:00:00 2001 From: Shreyansh Ray Date: Tue, 19 Mar 2024 18:16:13 +0530 Subject: [PATCH 02/16] Refactor TransferManager interface to RemoteStoreFileTrackerAdapter Signed-off-by: Shreyansh Ray --- .../org/opensearch/index/IndexService.java | 12 ++++++++ .../index/store/CompositeDirectory.java | 28 +++++++++++-------- .../store/CompositeDirectoryFactory.java | 11 +------- ...rectoryRemoteStoreFileTrackerAdapter.java} | 13 ++++++--- ...ava => RemoteStoreFileTrackerAdapter.java} | 2 +- 5 files changed, 39 insertions(+), 27 deletions(-) rename server/src/main/java/org/opensearch/index/store/{CompositeDirectoryTransferManager.java => CompositeDirectoryRemoteStoreFileTrackerAdapter.java} (84%) rename server/src/main/java/org/opensearch/index/store/{TransferManager.java => RemoteStoreFileTrackerAdapter.java} (94%) diff --git a/server/src/main/java/org/opensearch/index/IndexService.java b/server/src/main/java/org/opensearch/index/IndexService.java index e501d7eff3f81..3baec81d1e4c5 100644 --- a/server/src/main/java/org/opensearch/index/IndexService.java +++ b/server/src/main/java/org/opensearch/index/IndexService.java @@ -41,6 +41,7 @@ import org.apache.lucene.store.Directory; import org.apache.lucene.util.Accountable; import org.opensearch.client.Client; +import org.opensearch.cluster.metadata.ComposableIndexTemplate; import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; import org.opensearch.cluster.node.DiscoveryNode; @@ -92,6 +93,7 @@ import org.opensearch.index.shard.ShardPath; import org.opensearch.index.similarity.SimilarityService; import org.opensearch.index.store.RemoteSegmentStoreDirectoryFactory; +import org.opensearch.index.store.CompositeDirectory; import org.opensearch.index.store.Store; import org.opensearch.index.translog.Translog; import org.opensearch.index.translog.TranslogFactory; @@ -109,6 +111,7 @@ import org.opensearch.search.aggregations.support.ValuesSourceRegistry; import org.opensearch.threadpool.ThreadPool; +import java.awt.*; import java.io.Closeable; import java.io.IOException; import java.nio.file.Path; @@ -495,6 +498,7 @@ public synchronized IndexShard createShard( } }; Store remoteStore = null; +<<<<<<< HEAD boolean seedRemote = false; if (targetNode.isRemoteStoreNode()) { final Directory remoteDirectory; @@ -516,6 +520,11 @@ public synchronized IndexShard createShard( this.indexSettings.getRemoteStorePathStrategy() ); } +======= + Directory remoteDirectory = null; + if (this.indexSettings.isRemoteStoreEnabled()) { + remoteDirectory = remoteDirectoryFactory.newDirectory(this.indexSettings, path); +>>>>>>> f1cd4e4895d (Refactor TransferManager interface to RemoteStoreFileTrackerAdapter) remoteStore = new Store(shardId, this.indexSettings, remoteDirectory, lock, Store.OnClose.EMPTY, path); } else { // Disallow shards with remote store based settings to be created on non-remote store enabled nodes @@ -531,6 +540,9 @@ public synchronized IndexShard createShard( } Directory directory = directoryFactory.newDirectory(this.indexSettings, path); + if (directory instanceof CompositeDirectory) { + ((CompositeDirectory) directory).setRemoteDirectory(remoteDirectory); + } store = new Store( shardId, this.indexSettings, diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java index 080b42a2b65a2..ba134ad9344f3 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java @@ -8,13 +8,13 @@ package org.opensearch.index.store; +import org.apache.lucene.store.Directory; import org.apache.lucene.store.FSDirectory; import org.apache.lucene.store.FilterDirectory; import org.apache.lucene.store.IOContext; import org.apache.lucene.store.IndexInput; import org.apache.lucene.store.IndexOutput; import org.apache.lucene.store.Lock; -import org.opensearch.common.blobstore.BlobContainer; import org.opensearch.index.store.remote.filecache.CachedIndexInput; import org.opensearch.index.store.remote.filecache.FileCache; import org.opensearch.index.store.remote.utils.filetracker.FileState; @@ -27,14 +27,18 @@ public class CompositeDirectory extends FilterDirectory { private final FSDirectory localDirectory; - private final TransferManager transferManager; + private final RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter; private final FileCache fileCache; - public CompositeDirectory(FSDirectory localDirectory, BlobContainer blobContainer, FileCache fileCache) { + public CompositeDirectory(FSDirectory localDirectory, FileCache fileCache) { super(localDirectory); this.localDirectory = localDirectory; this.fileCache = fileCache; - this.transferManager = new CompositeDirectoryTransferManager(fileCache, blobContainer); + this.remoteStoreFileTrackerAdapter = new CompositeDirectoryRemoteStoreFileTrackerAdapter(fileCache); + } + + public void setRemoteDirectory(Directory remoteDirectory) { + ((CompositeDirectoryRemoteStoreFileTrackerAdapter)remoteStoreFileTrackerAdapter).setRemoteDirectory(remoteDirectory); } @Override @@ -45,7 +49,7 @@ public String[] listAll() throws IOException { @Override public void deleteFile(String name) throws IOException { super.deleteFile(name); - transferManager.removeFileFromTracker(name); + remoteStoreFileTrackerAdapter.removeFileFromTracker(name); fileCache.remove(localDirectory.getDirectory().resolve(name)); } @@ -56,7 +60,7 @@ public long fileLength(String name) throws IOException { @Override public IndexOutput createOutput(String name, IOContext context) throws IOException { - transferManager.trackFile(name, FileState.DISK, FileType.NON_BLOCK); + remoteStoreFileTrackerAdapter.trackFile(name, FileState.DISK, FileType.NON_BLOCK); return localDirectory.createOutput(name, context); } @@ -78,17 +82,17 @@ public void syncMetaData() throws IOException { @Override public void rename(String source, String dest) throws IOException { localDirectory.rename(source, dest); - transferManager.trackFile(dest, transferManager.getFileState(source), transferManager.getFileType(source)); - transferManager.removeFileFromTracker(source); + remoteStoreFileTrackerAdapter.trackFile(dest, remoteStoreFileTrackerAdapter.getFileState(source), remoteStoreFileTrackerAdapter.getFileType(source)); + remoteStoreFileTrackerAdapter.removeFileFromTracker(source); } @Override public IndexInput openInput(String name, IOContext context) throws IOException { - if (!transferManager.isFilePresent(name)) { + if (!remoteStoreFileTrackerAdapter.isFilePresent(name)) { return localDirectory.openInput(name, context); } IndexInput indexInput = null; - switch (transferManager.getFileState(name)) { + switch (remoteStoreFileTrackerAdapter.getFileState(name)) { case DISK: indexInput = localDirectory.openInput(name, context); break; @@ -121,8 +125,8 @@ public Set getPendingDeletions() throws IOException { public void afterSyncToRemote(Collection files) throws IOException { for (String fileName : files) { - if (transferManager.isFilePresent(fileName) && !transferManager.getFileState(fileName).equals(FileState.CACHE)) { - transferManager.updateFileState(fileName, FileState.CACHE); + if (remoteStoreFileTrackerAdapter.isFilePresent(fileName) && !remoteStoreFileTrackerAdapter.getFileState(fileName).equals(FileState.CACHE)) { + remoteStoreFileTrackerAdapter.updateFileState(fileName, FileState.CACHE); } fileCache.put(localDirectory.getDirectory().resolve(fileName), new CachedIndexInput() { @Override diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java index 52095614f56a9..bed6e2454574a 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java @@ -36,17 +36,8 @@ public CompositeDirectoryFactory(Supplier repositoriesServi @Override public Directory newDirectory(IndexSettings indexSettings, ShardPath shardPath) throws IOException { - String repositoryName = indexSettings.getRemoteStoreRepository(); - Repository repository = repositoriesService.get().repository(repositoryName); - BlobStoreRepository blobStoreRepository = (BlobStoreRepository) repository; - String shardId = String.valueOf(shardPath.getShardId().getId()); - String indexUUID = indexSettings.getIndex().getUUID(); - BlobPath blobPath = blobStoreRepository.basePath().add(indexUUID).add(shardId).add("segments").add("data"); - final BlobContainer blobContainer = blobStoreRepository.blobStore().blobContainer(blobPath); - final Path location = shardPath.resolveIndex(); final FSDirectory primaryDirectory = FSDirectory.open(location); - - return new CompositeDirectory(primaryDirectory, blobContainer, remoteStoreFileCache); + return new CompositeDirectory(primaryDirectory, remoteStoreFileCache); } } diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryTransferManager.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java similarity index 84% rename from server/src/main/java/org/opensearch/index/store/CompositeDirectoryTransferManager.java rename to server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java index 4e10a6db80db3..b6ae712579375 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryTransferManager.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java @@ -8,6 +8,7 @@ package org.opensearch.index.store; +import org.apache.lucene.store.Directory; import org.apache.lucene.store.IndexInput; import org.opensearch.common.blobstore.BlobContainer; import org.opensearch.index.store.remote.filecache.FileCache; @@ -19,18 +20,22 @@ import java.util.HashMap; import java.util.Map; -public class CompositeDirectoryTransferManager implements TransferManager { +public class CompositeDirectoryRemoteStoreFileTrackerAdapter implements RemoteStoreFileTrackerAdapter { private FileCache fileCache; private Map fileTracker; - private BlobContainer blobContainer; + private RemoteSegmentStoreDirectory remoteDirectory; - public CompositeDirectoryTransferManager(FileCache fileCache, BlobContainer blobContainer) { + public CompositeDirectoryRemoteStoreFileTrackerAdapter(FileCache fileCache) { this.fileCache = fileCache; - this.blobContainer = blobContainer; + remoteDirectory = null; this.fileTracker = new HashMap<>(); } + public void setRemoteDirectory(Directory remoteDirectory) { + this.remoteDirectory = (RemoteSegmentStoreDirectory) remoteDirectory; + } + @Override public IndexInput fetchBlob(BlobFetchRequest blobFetchRequest) { // TODO - This function will fetch the requested data from blobContainer diff --git a/server/src/main/java/org/opensearch/index/store/TransferManager.java b/server/src/main/java/org/opensearch/index/store/RemoteStoreFileTrackerAdapter.java similarity index 94% rename from server/src/main/java/org/opensearch/index/store/TransferManager.java rename to server/src/main/java/org/opensearch/index/store/RemoteStoreFileTrackerAdapter.java index 5029c51a34e34..9f3ef8a5571d3 100644 --- a/server/src/main/java/org/opensearch/index/store/TransferManager.java +++ b/server/src/main/java/org/opensearch/index/store/RemoteStoreFileTrackerAdapter.java @@ -13,7 +13,7 @@ import org.opensearch.index.store.remote.utils.filetracker.FileState; import org.opensearch.index.store.remote.utils.filetracker.FileType; -public interface TransferManager { +public interface RemoteStoreFileTrackerAdapter { IndexInput fetchBlob(BlobFetchRequest blobFetchRequest); void trackFile(String name, FileState fileState, FileType fileType); From 35f6f1e4fed88a855df1b1bf6f8ed0f4aaf25d82 Mon Sep 17 00:00:00 2001 From: Shreyansh Ray Date: Wed, 20 Mar 2024 12:16:30 +0530 Subject: [PATCH 03/16] Implement block level fetch for Composite Directory Signed-off-by: Shreyansh Ray --- .../index/store/CompositeDirectory.java | 7 +- .../store/CompositeDirectoryFactory.java | 9 +- ...irectoryRemoteStoreFileTrackerAdapter.java | 19 +++- .../store/RemoteSegmentStoreDirectory.java | 7 +- .../store/RemoteStoreFileTrackerAdapter.java | 4 +- .../OnDemandCompositeBlockIndexInput.java | 93 +++++++++++++++++++ 6 files changed, 129 insertions(+), 10 deletions(-) create mode 100644 server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java index ba134ad9344f3..66128299a186b 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java @@ -15,6 +15,7 @@ import org.apache.lucene.store.IndexInput; import org.apache.lucene.store.IndexOutput; import org.apache.lucene.store.Lock; +import org.opensearch.index.store.remote.file.OnDemandCompositeBlockIndexInput; import org.opensearch.index.store.remote.filecache.CachedIndexInput; import org.opensearch.index.store.remote.filecache.FileCache; import org.opensearch.index.store.remote.utils.filetracker.FileState; @@ -29,12 +30,14 @@ public class CompositeDirectory extends FilterDirectory { private final FSDirectory localDirectory; private final RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter; private final FileCache fileCache; + private final FSDirectory localCacheDirectory; - public CompositeDirectory(FSDirectory localDirectory, FileCache fileCache) { + public CompositeDirectory(FSDirectory localDirectory, FSDirectory localCacheDirectory, FileCache fileCache) { super(localDirectory); this.localDirectory = localDirectory; this.fileCache = fileCache; this.remoteStoreFileTrackerAdapter = new CompositeDirectoryRemoteStoreFileTrackerAdapter(fileCache); + this.localCacheDirectory = localCacheDirectory; } public void setRemoteDirectory(Directory remoteDirectory) { @@ -102,7 +105,7 @@ public IndexInput openInput(String name, IOContext context) throws IOException { break; case REMOTE_ONLY: - // TODO - return an implementation of OnDemandBlockIndexInput where the fetchBlock method is implemented + indexInput = new OnDemandCompositeBlockIndexInput(remoteStoreFileTrackerAdapter, name, localCacheDirectory); break; } return indexInput; diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java index bed6e2454574a..9f4fa5b959ed8 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java @@ -21,10 +21,12 @@ import org.opensearch.repositories.blobstore.BlobStoreRepository; import java.io.IOException; +import java.nio.file.Files; import java.nio.file.Path; import java.util.function.Supplier; public class CompositeDirectoryFactory implements IndexStorePlugin.DirectoryFactory { + private static String CACHE_LOCATION = "remote_cache"; private final Supplier repositoriesService; private final FileCache remoteStoreFileCache; @@ -36,8 +38,9 @@ public CompositeDirectoryFactory(Supplier repositoriesServi @Override public Directory newDirectory(IndexSettings indexSettings, ShardPath shardPath) throws IOException { - final Path location = shardPath.resolveIndex(); - final FSDirectory primaryDirectory = FSDirectory.open(location); - return new CompositeDirectory(primaryDirectory, remoteStoreFileCache); + final FSDirectory primaryDirectory = FSDirectory.open(shardPath.resolveIndex()); + final FSDirectory localCacheDirectory = FSDirectory.open(Files.createDirectories(shardPath.getDataPath().resolve(CACHE_LOCATION))); + localCacheDirectory.syncMetaData(); + return new CompositeDirectory(primaryDirectory, localCacheDirectory, remoteStoreFileCache); } } diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java index b6ae712579375..1b205aed88dcb 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java @@ -13,10 +13,12 @@ import org.opensearch.common.blobstore.BlobContainer; import org.opensearch.index.store.remote.filecache.FileCache; import org.opensearch.index.store.remote.utils.BlobFetchRequest; +import org.opensearch.index.store.remote.utils.TransferManager; import org.opensearch.index.store.remote.utils.filetracker.FileState; import org.opensearch.index.store.remote.utils.filetracker.FileTrackingInfo; import org.opensearch.index.store.remote.utils.filetracker.FileType; +import java.io.IOException; import java.util.HashMap; import java.util.Map; @@ -36,10 +38,21 @@ public void setRemoteDirectory(Directory remoteDirectory) { this.remoteDirectory = (RemoteSegmentStoreDirectory) remoteDirectory; } + public String getUploadedFileName(String name) { + return remoteDirectory.getExistingRemoteFilename(name); + } + + public long getFileLength(String name) { + try { + return remoteDirectory.fileLength(name); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + @Override - public IndexInput fetchBlob(BlobFetchRequest blobFetchRequest) { - // TODO - This function will fetch the requested data from blobContainer - return null; + public IndexInput fetchBlob(BlobFetchRequest blobFetchRequest) throws IOException { + return new TransferManager(remoteDirectory.getDataDirectoryBlobContainer(), fileCache).fetchBlob(blobFetchRequest); } public void trackFile(String name, FileState fileState, FileType fileType) { diff --git a/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java b/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java index ec1163fe91b6c..8b65110dd3903 100644 --- a/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java @@ -25,6 +25,7 @@ import org.apache.lucene.util.Version; import org.opensearch.common.UUIDs; import org.opensearch.common.annotation.PublicApi; +import org.opensearch.common.blobstore.BlobContainer; import org.opensearch.common.collect.Tuple; import org.opensearch.common.io.VersionedCodecStreamWrapper; import org.opensearch.common.logging.Loggers; @@ -136,6 +137,10 @@ public RemoteSegmentStoreDirectory( init(); } + public BlobContainer getDataDirectoryBlobContainer() { + return remoteDataDirectory.getBlobContainer(); + } + /** * Initializes the cache which keeps track of all the segment files uploaded to the remote segment store. * As this cache is specific to an instance of RemoteSegmentStoreDirectory, it is possible that cache becomes stale @@ -698,7 +703,7 @@ private String getChecksumOfLocalFile(Directory directory, String file) throws I } } - private String getExistingRemoteFilename(String localFilename) { + public String getExistingRemoteFilename(String localFilename) { if (segmentsUploadedToRemoteStore.containsKey(localFilename)) { return segmentsUploadedToRemoteStore.get(localFilename).uploadedFilename; } else { diff --git a/server/src/main/java/org/opensearch/index/store/RemoteStoreFileTrackerAdapter.java b/server/src/main/java/org/opensearch/index/store/RemoteStoreFileTrackerAdapter.java index 9f3ef8a5571d3..29f05c8dc60e8 100644 --- a/server/src/main/java/org/opensearch/index/store/RemoteStoreFileTrackerAdapter.java +++ b/server/src/main/java/org/opensearch/index/store/RemoteStoreFileTrackerAdapter.java @@ -13,8 +13,10 @@ import org.opensearch.index.store.remote.utils.filetracker.FileState; import org.opensearch.index.store.remote.utils.filetracker.FileType; +import java.io.IOException; + public interface RemoteStoreFileTrackerAdapter { - IndexInput fetchBlob(BlobFetchRequest blobFetchRequest); + IndexInput fetchBlob(BlobFetchRequest blobFetchRequest) throws IOException; void trackFile(String name, FileState fileState, FileType fileType); diff --git a/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java b/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java new file mode 100644 index 0000000000000..8b8975be7a46b --- /dev/null +++ b/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java @@ -0,0 +1,93 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.store.remote.file; + +import org.apache.lucene.store.FSDirectory; +import org.apache.lucene.store.IndexInput; +import org.opensearch.index.store.CompositeDirectoryRemoteStoreFileTrackerAdapter; +import org.opensearch.index.store.RemoteStoreFileTrackerAdapter; +import org.opensearch.index.store.remote.utils.BlobFetchRequest; + +import java.io.IOException; + +public class OnDemandCompositeBlockIndexInput extends OnDemandBlockIndexInput { + + private final RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter; + private final String fileName; + private final Long originalFileSize; + private final FSDirectory directory; + + public OnDemandCompositeBlockIndexInput(RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter, String fileName, FSDirectory directory) { + this( + OnDemandBlockIndexInput.builder(). + resourceDescription("OnDemandCompositeBlockIndexInput"). + isClone(false). + offset(0L). + length(getFileLength(remoteStoreFileTrackerAdapter, fileName)), + remoteStoreFileTrackerAdapter, + fileName, + directory); + } + + public OnDemandCompositeBlockIndexInput(Builder builder, RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter, String fileName, FSDirectory directory) { + super(builder); + this.remoteStoreFileTrackerAdapter = remoteStoreFileTrackerAdapter; + this.directory = null; + this.fileName = fileName; + originalFileSize = getFileLength(remoteStoreFileTrackerAdapter, fileName); + } + + @Override + protected OnDemandCompositeBlockIndexInput buildSlice(String sliceDescription, long offset, long length) { + return new OnDemandCompositeBlockIndexInput( + OnDemandBlockIndexInput.builder(). + blockSizeShift(blockSizeShift). + isClone(true). + offset(this.offset + offset). + length(length). + resourceDescription(sliceDescription), + remoteStoreFileTrackerAdapter, + fileName, + directory + ); + } + + @Override + protected IndexInput fetchBlock(int blockId) throws IOException { + final String uploadedFileName = ((CompositeDirectoryRemoteStoreFileTrackerAdapter)remoteStoreFileTrackerAdapter).getUploadedFileName(fileName); + final String blockFileName = uploadedFileName + "." + blockId; + final long blockStart = getBlockStart(blockId); + final long length = getActualBlockSize(blockId); + + BlobFetchRequest blobFetchRequest = BlobFetchRequest.builder() + .position(blockStart) + .length(length) + .blobName(uploadedFileName) + .directory(directory) + .fileName(blockFileName) + .build(); + return remoteStoreFileTrackerAdapter.fetchBlob(blobFetchRequest); + } + + @Override + public OnDemandBlockIndexInput clone() { + OnDemandCompositeBlockIndexInput clone = buildSlice("clone", 0L, this.length); + // ensures that clones may be positioned at the same point as the blocked file they were cloned from + clone.cloneBlock(this); + return clone; + } + + private long getActualBlockSize(int blockId) { + return (blockId != getBlock(originalFileSize - 1)) ? blockSize : getBlockOffset(originalFileSize - 1) + 1; + } + + private static long getFileLength(RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter, String fileName) { + return ((CompositeDirectoryRemoteStoreFileTrackerAdapter)remoteStoreFileTrackerAdapter).getFileLength(fileName); + } +} From ec27fbe8ad45147654d5ece831005226ac208fe7 Mon Sep 17 00:00:00 2001 From: Shreyansh Ray Date: Mon, 1 Apr 2024 12:45:57 +0530 Subject: [PATCH 04/16] Removed CACHE state from FileTracker Signed-off-by: Shreyansh Ray --- .../shard/RemoteStoreRefreshListener.java | 3 +- .../index/store/CompositeDirectory.java | 238 +++++++++++++----- .../store/CompositeDirectoryFactory.java | 5 +- ...irectoryRemoteStoreFileTrackerAdapter.java | 19 +- .../OnDemandCompositeBlockIndexInput.java | 14 +- .../remote/utils/filetracker/FileState.java | 9 +- 6 files changed, 220 insertions(+), 68 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java b/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java index 5379d21119ad9..193345d391c07 100644 --- a/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java +++ b/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java @@ -285,11 +285,12 @@ public void onFailure(Exception e) { } }, latch); + Collection segmentsToRefresh = localSegmentsPostRefresh.stream().filter(file -> !skipUpload(file)).collect(Collectors.toList()); // Start the segments files upload uploadNewSegments(localSegmentsPostRefresh, localSegmentsSizeMap, segmentUploadsCompletedListener); Directory directory = ((FilterDirectory) (((FilterDirectory) storeDirectory).getDelegate())).getDelegate(); if (directory instanceof CompositeDirectory) { - ((CompositeDirectory) directory).afterSyncToRemote(localSegmentsPostRefresh); + ((CompositeDirectory) directory).afterSyncToRemote(segmentsToRefresh); } latch.await(); } catch (EngineException e) { diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java index 66128299a186b..3df88e1ac2425 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java @@ -8,6 +8,8 @@ package org.opensearch.index.store; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.apache.lucene.store.Directory; import org.apache.lucene.store.FSDirectory; import org.apache.lucene.store.FilterDirectory; @@ -16,103 +18,208 @@ import org.apache.lucene.store.IndexOutput; import org.apache.lucene.store.Lock; import org.opensearch.index.store.remote.file.OnDemandCompositeBlockIndexInput; -import org.opensearch.index.store.remote.filecache.CachedIndexInput; import org.opensearch.index.store.remote.filecache.FileCache; import org.opensearch.index.store.remote.utils.filetracker.FileState; import org.opensearch.index.store.remote.utils.filetracker.FileType; import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; +import java.util.HashSet; +import java.util.List; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.stream.Collectors; public class CompositeDirectory extends FilterDirectory { + private static final Logger logger = LogManager.getLogger(CompositeDirectory.class); private final FSDirectory localDirectory; private final RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter; private final FileCache fileCache; - private final FSDirectory localCacheDirectory; + private final AtomicBoolean isRemoteDirectorySet; + private final ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock(); + private final ReentrantReadWriteLock.WriteLock writeLock = readWriteLock.writeLock(); + private final ReentrantReadWriteLock.ReadLock readLock = readWriteLock.readLock(); - public CompositeDirectory(FSDirectory localDirectory, FSDirectory localCacheDirectory, FileCache fileCache) { + public CompositeDirectory(FSDirectory localDirectory, FileCache fileCache) { super(localDirectory); this.localDirectory = localDirectory; this.fileCache = fileCache; this.remoteStoreFileTrackerAdapter = new CompositeDirectoryRemoteStoreFileTrackerAdapter(fileCache); - this.localCacheDirectory = localCacheDirectory; + isRemoteDirectorySet = new AtomicBoolean(false); } public void setRemoteDirectory(Directory remoteDirectory) { - ((CompositeDirectoryRemoteStoreFileTrackerAdapter)remoteStoreFileTrackerAdapter).setRemoteDirectory(remoteDirectory); + logger.trace("Setting remote Directory ..."); + if (!isRemoteDirectorySet()){ + ((CompositeDirectoryRemoteStoreFileTrackerAdapter)remoteStoreFileTrackerAdapter).setRemoteDirectory(remoteDirectory); + isRemoteDirectorySet.set(true); + } } @Override public String[] listAll() throws IOException { - return localDirectory.listAll(); + logger.trace("listAll() called ..."); + readLock.lock(); + try { + String[] remoteFiles = new String[0]; + String[] localFiles = localDirectory.listAll(); + if (isRemoteDirectorySet()) + remoteFiles = ((CompositeDirectoryRemoteStoreFileTrackerAdapter)remoteStoreFileTrackerAdapter).getRemoteFiles(); + logger.trace("LocalDirectory files : " + Arrays.toString(localFiles)); + logger.trace("Remote Directory files : " + Arrays.toString(remoteFiles)); + Set allFiles = new HashSet<>(Arrays.asList(localFiles)); + allFiles.addAll(Arrays.asList(remoteFiles)); + + Set localLuceneFiles = allFiles.stream() + .filter(file -> !isBlockFile(file)) + .collect(Collectors.toUnmodifiableSet()); + String[] files = new String[localLuceneFiles.size()]; + localLuceneFiles.toArray(files); + Arrays.sort(files); + + logger.trace("listAll() returns : " + Arrays.toString(files)); + + return files; + } finally { + readLock.unlock(); + } } @Override public void deleteFile(String name) throws IOException { - super.deleteFile(name); - remoteStoreFileTrackerAdapter.removeFileFromTracker(name); - fileCache.remove(localDirectory.getDirectory().resolve(name)); + logger.trace("deleteFile() called " + name); + writeLock.lock(); + try { + localDirectory.deleteFile(name); + remoteStoreFileTrackerAdapter.removeFileFromTracker(name); + fileCache.remove(localDirectory.getDirectory().resolve(name)); + logFileTracker(); + } finally { + writeLock.unlock(); + } } @Override public long fileLength(String name) throws IOException { - return localDirectory.fileLength(name); + logger.trace("fileLength() called " + name); + readLock.lock(); + try { + if(remoteStoreFileTrackerAdapter.getFileState(name).equals(FileState.DISK)) + { + logger.trace("fileLength from Local " + localDirectory.fileLength(name)); + return localDirectory.fileLength(name); + } + else + { + logger.trace("fileLength from Remote " + ((CompositeDirectoryRemoteStoreFileTrackerAdapter)remoteStoreFileTrackerAdapter).getFileLength(name)); + return ((CompositeDirectoryRemoteStoreFileTrackerAdapter)remoteStoreFileTrackerAdapter).getFileLength(name); + } + } finally { + readLock.unlock(); + } } @Override public IndexOutput createOutput(String name, IOContext context) throws IOException { - remoteStoreFileTrackerAdapter.trackFile(name, FileState.DISK, FileType.NON_BLOCK); - return localDirectory.createOutput(name, context); + logger.trace("createOutput() called " + name); + writeLock.lock(); + try { + remoteStoreFileTrackerAdapter.trackFile(name, FileState.DISK, FileType.NON_BLOCK); + logFileTracker(); + return localDirectory.createOutput(name, context); + } finally { + writeLock.unlock(); + } } @Override public IndexOutput createTempOutput(String prefix, String suffix, IOContext context) throws IOException { - return localDirectory.createTempOutput(prefix, suffix, context); + logger.trace("createOutput() called " + prefix + "," + suffix); + writeLock.lock(); + try { + return localDirectory.createTempOutput(prefix, suffix, context); + } finally { + writeLock.unlock(); + } } @Override public void sync(Collection names) throws IOException { - localDirectory.sync(names); + logger.trace("sync() called " + names); + writeLock.lock(); + try { + Collection newLocalFiles = new ArrayList<>(); + for (String name : names) { + if(remoteStoreFileTrackerAdapter.getFileState(name).equals(FileState.DISK)) + newLocalFiles.add(name); + } + logger.trace("Synced files : " + newLocalFiles); + localDirectory.sync(newLocalFiles); + } finally { + writeLock.unlock(); + } } @Override public void syncMetaData() throws IOException { - localDirectory.syncMetaData(); + logger.trace("syncMetaData() called "); + writeLock.lock(); + try { + localDirectory.syncMetaData(); + } finally { + writeLock.unlock(); + } } @Override public void rename(String source, String dest) throws IOException { - localDirectory.rename(source, dest); - remoteStoreFileTrackerAdapter.trackFile(dest, remoteStoreFileTrackerAdapter.getFileState(source), remoteStoreFileTrackerAdapter.getFileType(source)); - remoteStoreFileTrackerAdapter.removeFileFromTracker(source); + logger.trace("rename() called " + source + " -> " + dest); + writeLock.lock(); + try { + localDirectory.rename(source, dest); + remoteStoreFileTrackerAdapter.trackFile(dest, remoteStoreFileTrackerAdapter.getFileState(source), remoteStoreFileTrackerAdapter.getFileType(source)); + remoteStoreFileTrackerAdapter.removeFileFromTracker(source); + logFileTracker(); + } finally { + writeLock.unlock(); + } } @Override public IndexInput openInput(String name, IOContext context) throws IOException { - if (!remoteStoreFileTrackerAdapter.isFilePresent(name)) { - return localDirectory.openInput(name, context); - } - IndexInput indexInput = null; - switch (remoteStoreFileTrackerAdapter.getFileState(name)) { - case DISK: - indexInput = localDirectory.openInput(name, context); - break; - - case CACHE: - indexInput = fileCache.get(localDirectory.getDirectory().resolve(name)).getIndexInput(); - break; - - case REMOTE_ONLY: - indexInput = new OnDemandCompositeBlockIndexInput(remoteStoreFileTrackerAdapter, name, localCacheDirectory); - break; + logger.trace("openInput() called " + name); + writeLock.lock(); + try { + if (!remoteStoreFileTrackerAdapter.isFilePresent(name)) { + //Print filename to check which file is not present in tracker + logger.trace("File not found in tracker"); + return localDirectory.openInput(name, context); + } + IndexInput indexInput = null; + switch (remoteStoreFileTrackerAdapter.getFileState(name)) { + case DISK: + logger.trace("File found in disk "); + indexInput = localDirectory.openInput(name, context); + break; + + case REMOTE: + logger.trace("File to be fetched from Remote "); + indexInput = new OnDemandCompositeBlockIndexInput(remoteStoreFileTrackerAdapter, name, localDirectory); + break; + } + return indexInput; + } finally { + writeLock.unlock(); } - return indexInput; } @Override public Lock obtainLock(String name) throws IOException { + logger.trace("obtainLock() called " + name); return localDirectory.obtainLock(name); } @@ -127,33 +234,48 @@ public Set getPendingDeletions() throws IOException { } public void afterSyncToRemote(Collection files) throws IOException { + logger.trace("afterSyncToRemote called for " + files); + if(!isRemoteDirectorySet()) + throw new UnsupportedOperationException("Cannot perform afterSyncToRemote if Remote Directory is not set"); + List delFiles = new ArrayList<>(); for (String fileName : files) { - if (remoteStoreFileTrackerAdapter.isFilePresent(fileName) && !remoteStoreFileTrackerAdapter.getFileState(fileName).equals(FileState.CACHE)) { - remoteStoreFileTrackerAdapter.updateFileState(fileName, FileState.CACHE); - } - fileCache.put(localDirectory.getDirectory().resolve(fileName), new CachedIndexInput() { - @Override - public IndexInput getIndexInput() throws IOException { - return localDirectory.openInput(fileName, IOContext.READ); + if (isSegmentsOrLockFile(fileName)) + continue; + writeLock.lock(); + try { + if (remoteStoreFileTrackerAdapter.isFilePresent(fileName) && remoteStoreFileTrackerAdapter.getFileState(fileName).equals(FileState.DISK)) { + remoteStoreFileTrackerAdapter.updateFileState(fileName, FileState.REMOTE); } + } finally { + writeLock.unlock(); + } + localDirectory.deleteFile(fileName); + delFiles.add(fileName); + } + logger.trace("Files removed form local " + delFiles); + logFileTracker(); + } - @Override - public long length() { - try { - return localDirectory.fileLength(fileName); - } catch (IOException e) { - throw new RuntimeException(e); - } - } + private boolean isSegmentsOrLockFile(String fileName) { + if (fileName.startsWith("segments_") || + fileName.endsWith(".si") || + fileName.endsWith(".lock")) + return true; + return false; + } - @Override - public boolean isClosed() { - return false; - } + private boolean isBlockFile(String fileName) { + if (fileName.contains("_block_")) + return true; + return false; + } - @Override - public void close() {} - }); - } + private boolean isRemoteDirectorySet() { + return isRemoteDirectorySet.get(); + } + + public void logFileTracker() { + String res = ((CompositeDirectoryRemoteStoreFileTrackerAdapter)remoteStoreFileTrackerAdapter).logFileTracker(); + logger.trace(res); } } diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java index 9f4fa5b959ed8..550f89daf34ca 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java @@ -26,7 +26,6 @@ import java.util.function.Supplier; public class CompositeDirectoryFactory implements IndexStorePlugin.DirectoryFactory { - private static String CACHE_LOCATION = "remote_cache"; private final Supplier repositoriesService; private final FileCache remoteStoreFileCache; @@ -39,8 +38,6 @@ public CompositeDirectoryFactory(Supplier repositoriesServi @Override public Directory newDirectory(IndexSettings indexSettings, ShardPath shardPath) throws IOException { final FSDirectory primaryDirectory = FSDirectory.open(shardPath.resolveIndex()); - final FSDirectory localCacheDirectory = FSDirectory.open(Files.createDirectories(shardPath.getDataPath().resolve(CACHE_LOCATION))); - localCacheDirectory.syncMetaData(); - return new CompositeDirectory(primaryDirectory, localCacheDirectory, remoteStoreFileCache); + return new CompositeDirectory(primaryDirectory, remoteStoreFileCache); } } diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java index 1b205aed88dcb..d7e2603607642 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java @@ -10,7 +10,6 @@ import org.apache.lucene.store.Directory; import org.apache.lucene.store.IndexInput; -import org.opensearch.common.blobstore.BlobContainer; import org.opensearch.index.store.remote.filecache.FileCache; import org.opensearch.index.store.remote.utils.BlobFetchRequest; import org.opensearch.index.store.remote.utils.TransferManager; @@ -96,4 +95,22 @@ public FileType getFileType(String name) { public boolean isFilePresent(String name) { return fileTracker.containsKey(name); } + + public String[] getRemoteFiles() throws IOException { + String[] remoteFiles; + try { + remoteFiles = remoteDirectory.listAll(); + } catch (Exception e) { + remoteFiles = new String[0]; + } + return remoteFiles; + } + + public String logFileTracker() { + String result = ""; + for (Map.Entry entry : fileTracker.entrySet()) { + result += entry.getKey() + " : " + entry.getValue().getFileType().name() + " , " + entry.getValue().getFileState().name() +"\n"; + } + return result; + } } diff --git a/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java b/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java index 8b8975be7a46b..a364d745a2075 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java +++ b/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java @@ -8,6 +8,8 @@ package org.opensearch.index.store.remote.file; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.apache.lucene.store.FSDirectory; import org.apache.lucene.store.IndexInput; import org.opensearch.index.store.CompositeDirectoryRemoteStoreFileTrackerAdapter; @@ -18,6 +20,7 @@ public class OnDemandCompositeBlockIndexInput extends OnDemandBlockIndexInput { + private static final Logger logger = LogManager.getLogger(OnDemandCompositeBlockIndexInput.class); private final RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter; private final String fileName; private final Long originalFileSize; @@ -38,7 +41,7 @@ public OnDemandCompositeBlockIndexInput(RemoteStoreFileTrackerAdapter remoteStor public OnDemandCompositeBlockIndexInput(Builder builder, RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter, String fileName, FSDirectory directory) { super(builder); this.remoteStoreFileTrackerAdapter = remoteStoreFileTrackerAdapter; - this.directory = null; + this.directory = directory; this.fileName = fileName; originalFileSize = getFileLength(remoteStoreFileTrackerAdapter, fileName); } @@ -60,10 +63,17 @@ protected OnDemandCompositeBlockIndexInput buildSlice(String sliceDescription, l @Override protected IndexInput fetchBlock(int blockId) throws IOException { + logger.trace("fetchBlock called with blockId -> " + blockId); final String uploadedFileName = ((CompositeDirectoryRemoteStoreFileTrackerAdapter)remoteStoreFileTrackerAdapter).getUploadedFileName(fileName); - final String blockFileName = uploadedFileName + "." + blockId; + final String blockFileName = fileName + "_block_" + blockId; final long blockStart = getBlockStart(blockId); final long length = getActualBlockSize(blockId); + logger.trace("File: " + uploadedFileName + + ", Block File: " + blockFileName + + ", BlockStart: " + blockStart + + ", Length: " + length + + ", BlockSize: " + blockSize + + ", OriginalFileSize: " + originalFileSize); BlobFetchRequest blobFetchRequest = BlobFetchRequest.builder() .position(blockStart) diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileState.java b/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileState.java index 564bb87e03857..73b39bac35f35 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileState.java +++ b/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileState.java @@ -9,7 +9,12 @@ package org.opensearch.index.store.remote.utils.filetracker; public enum FileState { + /** + * DISK State means that currently the file is present only locally and has not yet been uploaded to the Remote Store + */ DISK, - CACHE, - REMOTE_ONLY; + /** + * REMOTE State means that the file has been successfully uploaded to the Remote Store and is safe to be removed locally + */ + REMOTE; } From f93655d368e845aeb83b6d02cf4b262d10b2bfe0 Mon Sep 17 00:00:00 2001 From: Shreyansh Ray Date: Mon, 1 Apr 2024 15:00:27 +0530 Subject: [PATCH 05/16] Fixes after latest pull Signed-off-by: Shreyansh Ray --- .../common/blobstore/BlobContainer.java | 3 + .../common/blobstore/BlobMetadata.java | 3 + .../common/blobstore/DeleteResult.java | 3 + .../org/opensearch/index/IndexService.java | 12 +--- .../shard/RemoteStoreRefreshListener.java | 4 +- .../index/store/CompositeDirectory.java | 58 ++++++++------- .../store/CompositeDirectoryFactory.java | 6 -- ...irectoryRemoteStoreFileTrackerAdapter.java | 7 +- .../OnDemandCompositeBlockIndexInput.java | 71 ++++++++++++------- 9 files changed, 93 insertions(+), 74 deletions(-) diff --git a/server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java b/server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java index 4f5f8d4b1ef5f..1c71a1d431e2b 100644 --- a/server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java +++ b/server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java @@ -34,6 +34,7 @@ import org.opensearch.common.Nullable; import org.opensearch.common.annotation.ExperimentalApi; +import org.opensearch.common.annotation.PublicApi; import org.opensearch.core.action.ActionListener; import java.io.IOException; @@ -50,6 +51,7 @@ * * @opensearch.internal */ +@PublicApi(since = "2.3.0") public interface BlobContainer { /** @@ -277,6 +279,7 @@ default void writeBlobAtomicWithMetadata( /** * The type representing sort order of blob names */ + @PublicApi(since = "2.3.0") enum BlobNameSortOrder { LEXICOGRAPHIC(Comparator.comparing(BlobMetadata::name)); diff --git a/server/src/main/java/org/opensearch/common/blobstore/BlobMetadata.java b/server/src/main/java/org/opensearch/common/blobstore/BlobMetadata.java index 37c70365b6a11..4fdfb413d24d7 100644 --- a/server/src/main/java/org/opensearch/common/blobstore/BlobMetadata.java +++ b/server/src/main/java/org/opensearch/common/blobstore/BlobMetadata.java @@ -32,11 +32,14 @@ package org.opensearch.common.blobstore; +import org.opensearch.common.annotation.PublicApi; + /** * An interface for providing basic metadata about a blob. * * @opensearch.internal */ +@PublicApi(since = "2.3.0") public interface BlobMetadata { /** diff --git a/server/src/main/java/org/opensearch/common/blobstore/DeleteResult.java b/server/src/main/java/org/opensearch/common/blobstore/DeleteResult.java index 3b424c582ebc6..9ee37515cd005 100644 --- a/server/src/main/java/org/opensearch/common/blobstore/DeleteResult.java +++ b/server/src/main/java/org/opensearch/common/blobstore/DeleteResult.java @@ -32,11 +32,14 @@ package org.opensearch.common.blobstore; +import org.opensearch.common.annotation.PublicApi; + /** * The result of deleting multiple blobs from a {@link BlobStore}. * * @opensearch.internal */ +@PublicApi(since = "2.3.0") public final class DeleteResult { public static final DeleteResult ZERO = new DeleteResult(0, 0); diff --git a/server/src/main/java/org/opensearch/index/IndexService.java b/server/src/main/java/org/opensearch/index/IndexService.java index 3baec81d1e4c5..61309b4e29982 100644 --- a/server/src/main/java/org/opensearch/index/IndexService.java +++ b/server/src/main/java/org/opensearch/index/IndexService.java @@ -41,7 +41,6 @@ import org.apache.lucene.store.Directory; import org.apache.lucene.util.Accountable; import org.opensearch.client.Client; -import org.opensearch.cluster.metadata.ComposableIndexTemplate; import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; import org.opensearch.cluster.node.DiscoveryNode; @@ -92,8 +91,8 @@ import org.opensearch.index.shard.ShardNotInPrimaryModeException; import org.opensearch.index.shard.ShardPath; import org.opensearch.index.similarity.SimilarityService; -import org.opensearch.index.store.RemoteSegmentStoreDirectoryFactory; import org.opensearch.index.store.CompositeDirectory; +import org.opensearch.index.store.RemoteSegmentStoreDirectoryFactory; import org.opensearch.index.store.Store; import org.opensearch.index.translog.Translog; import org.opensearch.index.translog.TranslogFactory; @@ -111,7 +110,6 @@ import org.opensearch.search.aggregations.support.ValuesSourceRegistry; import org.opensearch.threadpool.ThreadPool; -import java.awt.*; import java.io.Closeable; import java.io.IOException; import java.nio.file.Path; @@ -498,10 +496,9 @@ public synchronized IndexShard createShard( } }; Store remoteStore = null; -<<<<<<< HEAD + Directory remoteDirectory = null; boolean seedRemote = false; if (targetNode.isRemoteStoreNode()) { - final Directory remoteDirectory; if (this.indexSettings.isRemoteStoreEnabled()) { remoteDirectory = remoteDirectoryFactory.newDirectory(this.indexSettings, path); } else { @@ -520,11 +517,6 @@ public synchronized IndexShard createShard( this.indexSettings.getRemoteStorePathStrategy() ); } -======= - Directory remoteDirectory = null; - if (this.indexSettings.isRemoteStoreEnabled()) { - remoteDirectory = remoteDirectoryFactory.newDirectory(this.indexSettings, path); ->>>>>>> f1cd4e4895d (Refactor TransferManager interface to RemoteStoreFileTrackerAdapter) remoteStore = new Store(shardId, this.indexSettings, remoteDirectory, lock, Store.OnClose.EMPTY, path); } else { // Disallow shards with remote store based settings to be created on non-remote store enabled nodes diff --git a/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java b/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java index 193345d391c07..0cd05721d4ff9 100644 --- a/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java +++ b/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java @@ -285,7 +285,9 @@ public void onFailure(Exception e) { } }, latch); - Collection segmentsToRefresh = localSegmentsPostRefresh.stream().filter(file -> !skipUpload(file)).collect(Collectors.toList()); + Collection segmentsToRefresh = localSegmentsPostRefresh.stream() + .filter(file -> !skipUpload(file)) + .collect(Collectors.toList()); // Start the segments files upload uploadNewSegments(localSegmentsPostRefresh, localSegmentsSizeMap, segmentUploadsCompletedListener); Directory directory = ((FilterDirectory) (((FilterDirectory) storeDirectory).getDelegate())).getDelegate(); diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java index 3df88e1ac2425..291b499eeb1b4 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java @@ -54,8 +54,8 @@ public CompositeDirectory(FSDirectory localDirectory, FileCache fileCache) { public void setRemoteDirectory(Directory remoteDirectory) { logger.trace("Setting remote Directory ..."); - if (!isRemoteDirectorySet()){ - ((CompositeDirectoryRemoteStoreFileTrackerAdapter)remoteStoreFileTrackerAdapter).setRemoteDirectory(remoteDirectory); + if (!isRemoteDirectorySet()) { + ((CompositeDirectoryRemoteStoreFileTrackerAdapter) remoteStoreFileTrackerAdapter).setRemoteDirectory(remoteDirectory); isRemoteDirectorySet.set(true); } } @@ -67,16 +67,14 @@ public String[] listAll() throws IOException { try { String[] remoteFiles = new String[0]; String[] localFiles = localDirectory.listAll(); - if (isRemoteDirectorySet()) - remoteFiles = ((CompositeDirectoryRemoteStoreFileTrackerAdapter)remoteStoreFileTrackerAdapter).getRemoteFiles(); + if (isRemoteDirectorySet()) remoteFiles = ((CompositeDirectoryRemoteStoreFileTrackerAdapter) remoteStoreFileTrackerAdapter) + .getRemoteFiles(); logger.trace("LocalDirectory files : " + Arrays.toString(localFiles)); logger.trace("Remote Directory files : " + Arrays.toString(remoteFiles)); Set allFiles = new HashSet<>(Arrays.asList(localFiles)); allFiles.addAll(Arrays.asList(remoteFiles)); - Set localLuceneFiles = allFiles.stream() - .filter(file -> !isBlockFile(file)) - .collect(Collectors.toUnmodifiableSet()); + Set localLuceneFiles = allFiles.stream().filter(file -> !isBlockFile(file)).collect(Collectors.toUnmodifiableSet()); String[] files = new String[localLuceneFiles.size()]; localLuceneFiles.toArray(files); Arrays.sort(files); @@ -108,15 +106,15 @@ public long fileLength(String name) throws IOException { logger.trace("fileLength() called " + name); readLock.lock(); try { - if(remoteStoreFileTrackerAdapter.getFileState(name).equals(FileState.DISK)) - { + if (remoteStoreFileTrackerAdapter.getFileState(name).equals(FileState.DISK)) { logger.trace("fileLength from Local " + localDirectory.fileLength(name)); return localDirectory.fileLength(name); - } - else - { - logger.trace("fileLength from Remote " + ((CompositeDirectoryRemoteStoreFileTrackerAdapter)remoteStoreFileTrackerAdapter).getFileLength(name)); - return ((CompositeDirectoryRemoteStoreFileTrackerAdapter)remoteStoreFileTrackerAdapter).getFileLength(name); + } else { + logger.trace( + "fileLength from Remote " + + ((CompositeDirectoryRemoteStoreFileTrackerAdapter) remoteStoreFileTrackerAdapter).getFileLength(name) + ); + return ((CompositeDirectoryRemoteStoreFileTrackerAdapter) remoteStoreFileTrackerAdapter).getFileLength(name); } } finally { readLock.unlock(); @@ -154,8 +152,7 @@ public void sync(Collection names) throws IOException { try { Collection newLocalFiles = new ArrayList<>(); for (String name : names) { - if(remoteStoreFileTrackerAdapter.getFileState(name).equals(FileState.DISK)) - newLocalFiles.add(name); + if (remoteStoreFileTrackerAdapter.getFileState(name).equals(FileState.DISK)) newLocalFiles.add(name); } logger.trace("Synced files : " + newLocalFiles); localDirectory.sync(newLocalFiles); @@ -181,7 +178,11 @@ public void rename(String source, String dest) throws IOException { writeLock.lock(); try { localDirectory.rename(source, dest); - remoteStoreFileTrackerAdapter.trackFile(dest, remoteStoreFileTrackerAdapter.getFileState(source), remoteStoreFileTrackerAdapter.getFileType(source)); + remoteStoreFileTrackerAdapter.trackFile( + dest, + remoteStoreFileTrackerAdapter.getFileState(source), + remoteStoreFileTrackerAdapter.getFileType(source) + ); remoteStoreFileTrackerAdapter.removeFileFromTracker(source); logFileTracker(); } finally { @@ -195,7 +196,7 @@ public IndexInput openInput(String name, IOContext context) throws IOException { writeLock.lock(); try { if (!remoteStoreFileTrackerAdapter.isFilePresent(name)) { - //Print filename to check which file is not present in tracker + // Print filename to check which file is not present in tracker logger.trace("File not found in tracker"); return localDirectory.openInput(name, context); } @@ -235,15 +236,16 @@ public Set getPendingDeletions() throws IOException { public void afterSyncToRemote(Collection files) throws IOException { logger.trace("afterSyncToRemote called for " + files); - if(!isRemoteDirectorySet()) - throw new UnsupportedOperationException("Cannot perform afterSyncToRemote if Remote Directory is not set"); + if (!isRemoteDirectorySet()) throw new UnsupportedOperationException( + "Cannot perform afterSyncToRemote if Remote Directory is not set" + ); List delFiles = new ArrayList<>(); for (String fileName : files) { - if (isSegmentsOrLockFile(fileName)) - continue; + if (isSegmentsOrLockFile(fileName)) continue; writeLock.lock(); try { - if (remoteStoreFileTrackerAdapter.isFilePresent(fileName) && remoteStoreFileTrackerAdapter.getFileState(fileName).equals(FileState.DISK)) { + if (remoteStoreFileTrackerAdapter.isFilePresent(fileName) + && remoteStoreFileTrackerAdapter.getFileState(fileName).equals(FileState.DISK)) { remoteStoreFileTrackerAdapter.updateFileState(fileName, FileState.REMOTE); } } finally { @@ -257,16 +259,12 @@ public void afterSyncToRemote(Collection files) throws IOException { } private boolean isSegmentsOrLockFile(String fileName) { - if (fileName.startsWith("segments_") || - fileName.endsWith(".si") || - fileName.endsWith(".lock")) - return true; + if (fileName.startsWith("segments_") || fileName.endsWith(".si") || fileName.endsWith(".lock")) return true; return false; } private boolean isBlockFile(String fileName) { - if (fileName.contains("_block_")) - return true; + if (fileName.contains("_block_")) return true; return false; } @@ -275,7 +273,7 @@ private boolean isRemoteDirectorySet() { } public void logFileTracker() { - String res = ((CompositeDirectoryRemoteStoreFileTrackerAdapter)remoteStoreFileTrackerAdapter).logFileTracker(); + String res = ((CompositeDirectoryRemoteStoreFileTrackerAdapter) remoteStoreFileTrackerAdapter).logFileTracker(); logger.trace(res); } } diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java index 550f89daf34ca..8256e22eadbca 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java @@ -10,19 +10,13 @@ import org.apache.lucene.store.Directory; import org.apache.lucene.store.FSDirectory; -import org.opensearch.common.blobstore.BlobContainer; -import org.opensearch.common.blobstore.BlobPath; import org.opensearch.index.IndexSettings; import org.opensearch.index.shard.ShardPath; import org.opensearch.index.store.remote.filecache.FileCache; import org.opensearch.plugins.IndexStorePlugin; import org.opensearch.repositories.RepositoriesService; -import org.opensearch.repositories.Repository; -import org.opensearch.repositories.blobstore.BlobStoreRepository; import java.io.IOException; -import java.nio.file.Files; -import java.nio.file.Path; import java.util.function.Supplier; public class CompositeDirectoryFactory implements IndexStorePlugin.DirectoryFactory { diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java index d7e2603607642..675c6b8c39871 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java @@ -109,7 +109,12 @@ public String[] getRemoteFiles() throws IOException { public String logFileTracker() { String result = ""; for (Map.Entry entry : fileTracker.entrySet()) { - result += entry.getKey() + " : " + entry.getValue().getFileType().name() + " , " + entry.getValue().getFileState().name() +"\n"; + result += entry.getKey() + + " : " + + entry.getValue().getFileType().name() + + " , " + + entry.getValue().getFileState().name() + + "\n"; } return result; } diff --git a/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java b/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java index a364d745a2075..8792e2294a81b 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java +++ b/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java @@ -17,6 +17,8 @@ import org.opensearch.index.store.remote.utils.BlobFetchRequest; import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; public class OnDemandCompositeBlockIndexInput extends OnDemandBlockIndexInput { @@ -26,19 +28,29 @@ public class OnDemandCompositeBlockIndexInput extends OnDemandBlockIndexInput { private final Long originalFileSize; private final FSDirectory directory; - public OnDemandCompositeBlockIndexInput(RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter, String fileName, FSDirectory directory) { + public OnDemandCompositeBlockIndexInput( + RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter, + String fileName, + FSDirectory directory + ) { this( - OnDemandBlockIndexInput.builder(). - resourceDescription("OnDemandCompositeBlockIndexInput"). - isClone(false). - offset(0L). - length(getFileLength(remoteStoreFileTrackerAdapter, fileName)), + OnDemandBlockIndexInput.builder() + .resourceDescription("OnDemandCompositeBlockIndexInput") + .isClone(false) + .offset(0L) + .length(getFileLength(remoteStoreFileTrackerAdapter, fileName)), remoteStoreFileTrackerAdapter, fileName, - directory); + directory + ); } - public OnDemandCompositeBlockIndexInput(Builder builder, RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter, String fileName, FSDirectory directory) { + public OnDemandCompositeBlockIndexInput( + Builder builder, + RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter, + String fileName, + FSDirectory directory + ) { super(builder); this.remoteStoreFileTrackerAdapter = remoteStoreFileTrackerAdapter; this.directory = directory; @@ -49,12 +61,12 @@ public OnDemandCompositeBlockIndexInput(Builder builder, RemoteStoreFileTrackerA @Override protected OnDemandCompositeBlockIndexInput buildSlice(String sliceDescription, long offset, long length) { return new OnDemandCompositeBlockIndexInput( - OnDemandBlockIndexInput.builder(). - blockSizeShift(blockSizeShift). - isClone(true). - offset(this.offset + offset). - length(length). - resourceDescription(sliceDescription), + OnDemandBlockIndexInput.builder() + .blockSizeShift(blockSizeShift) + .isClone(true) + .offset(this.offset + offset) + .length(length) + .resourceDescription(sliceDescription), remoteStoreFileTrackerAdapter, fileName, directory @@ -64,21 +76,28 @@ protected OnDemandCompositeBlockIndexInput buildSlice(String sliceDescription, l @Override protected IndexInput fetchBlock(int blockId) throws IOException { logger.trace("fetchBlock called with blockId -> " + blockId); - final String uploadedFileName = ((CompositeDirectoryRemoteStoreFileTrackerAdapter)remoteStoreFileTrackerAdapter).getUploadedFileName(fileName); + final String uploadedFileName = ((CompositeDirectoryRemoteStoreFileTrackerAdapter) remoteStoreFileTrackerAdapter) + .getUploadedFileName(fileName); final String blockFileName = fileName + "_block_" + blockId; final long blockStart = getBlockStart(blockId); final long length = getActualBlockSize(blockId); - logger.trace("File: " + uploadedFileName + - ", Block File: " + blockFileName + - ", BlockStart: " + blockStart + - ", Length: " + length + - ", BlockSize: " + blockSize + - ", OriginalFileSize: " + originalFileSize); - + logger.trace( + "File: " + + uploadedFileName + + ", Block File: " + + blockFileName + + ", BlockStart: " + + blockStart + + ", Length: " + + length + + ", BlockSize: " + + blockSize + + ", OriginalFileSize: " + + originalFileSize + ); + BlobFetchRequest.BlobPart blobPart = new BlobFetchRequest.BlobPart(uploadedFileName, blockStart, length); BlobFetchRequest blobFetchRequest = BlobFetchRequest.builder() - .position(blockStart) - .length(length) - .blobName(uploadedFileName) + .blobParts(new ArrayList<>(Arrays.asList(blobPart))) .directory(directory) .fileName(blockFileName) .build(); @@ -98,6 +117,6 @@ private long getActualBlockSize(int blockId) { } private static long getFileLength(RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter, String fileName) { - return ((CompositeDirectoryRemoteStoreFileTrackerAdapter)remoteStoreFileTrackerAdapter).getFileLength(fileName); + return ((CompositeDirectoryRemoteStoreFileTrackerAdapter) remoteStoreFileTrackerAdapter).getFileLength(fileName); } } From 04bad8a32e818dd928911149dce0c4fa3922a025 Mon Sep 17 00:00:00 2001 From: Shreyansh Ray Date: Fri, 5 Apr 2024 01:56:41 +0530 Subject: [PATCH 06/16] Add new setting for warm, remove store type setting, FileTracker and RemoteStoreFileTrackerAdapter, CompositeDirectoryFactory and update Composite Directory implementation Signed-off-by: Shreyansh Ray --- .../remotestore/CompositeDirectoryIT.java | 7 +- .../metadata/MetadataCreateIndexService.java | 10 +- .../common/blobstore/BlobContainer.java | 3 - .../common/blobstore/BlobMetadata.java | 3 - .../common/blobstore/DeleteResult.java | 3 - .../common/settings/IndexScopedSettings.java | 1 + .../org/opensearch/index/IndexModule.java | 62 +++- .../org/opensearch/index/IndexService.java | 28 +- .../org/opensearch/index/IndexSettings.java | 12 + .../index/store/CompositeDirectory.java | 284 +++++++++++------- .../store/CompositeDirectoryFactory.java | 37 --- ...irectoryRemoteStoreFileTrackerAdapter.java | 121 -------- .../index/store/RemoteDirectory.java | 16 +- .../store/RemoteSegmentStoreDirectory.java | 7 +- .../store/RemoteStoreFileTrackerAdapter.java | 34 --- .../OnDemandCompositeBlockIndexInput.java | 215 +++++++++---- .../store/remote/utils/BlockIOContext.java | 62 ++++ .../index/store/remote/utils/FileType.java | 38 +++ .../remote/utils/filetracker/FileState.java | 20 -- .../utils/filetracker/FileTrackingInfo.java | 27 -- .../remote/utils/filetracker/FileType.java | 18 -- .../opensearch/indices/IndicesService.java | 12 +- .../main/java/org/opensearch/node/Node.java | 3 +- .../opensearch/index/IndexModuleTests.java | 12 +- .../snapshots/SnapshotResiliencyTests.java | 3 +- 25 files changed, 572 insertions(+), 466 deletions(-) delete mode 100644 server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java delete mode 100644 server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java delete mode 100644 server/src/main/java/org/opensearch/index/store/RemoteStoreFileTrackerAdapter.java create mode 100644 server/src/main/java/org/opensearch/index/store/remote/utils/BlockIOContext.java create mode 100644 server/src/main/java/org/opensearch/index/store/remote/utils/FileType.java delete mode 100644 server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileState.java delete mode 100644 server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileTrackingInfo.java delete mode 100644 server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileType.java diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java index abb291644e479..35499c2f8cc5e 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java @@ -14,6 +14,7 @@ import org.opensearch.action.admin.indices.get.GetIndexResponse; import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.common.settings.Settings; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.index.IndexModule; import org.opensearch.index.IndexService; import org.opensearch.index.shard.IndexShard; @@ -30,9 +31,11 @@ public class CompositeDirectoryIT extends RemoteStoreBaseIntegTestCase { public void testCompositeDirectory() throws Exception { Settings settings = Settings.builder() + .put(super.featureFlagSettings()) + .put(FeatureFlags.WRITEABLE_REMOTE_INDEX, "true") .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) - .put(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), "compositefs") .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) + .put(IndexModule.INDEX_STORE_LOCALITY_SETTING.getKey(), "partial") .build(); assertAcked(client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()); GetIndexResponse getIndexResponse = client().admin() @@ -57,7 +60,7 @@ public void testCompositeDirectory() throws Exception { assertEquals("true", indexSettings.get(SETTING_REMOTE_STORE_ENABLED)); assertEquals(REPOSITORY_NAME, indexSettings.get(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY)); assertEquals(REPOSITORY_2_NAME, indexSettings.get(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY)); - assertEquals("compositefs", indexSettings.get("index.store.type")); + assertEquals("partial", indexSettings.get("index.store.locality")); ensureGreen("test-idx-1"); indexData(10, false, "test-idx-1"); diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java index a2a79ae7f379c..db6af7fffaeac 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java @@ -987,7 +987,7 @@ static Settings aggregateIndexSettings( validateStoreTypeSettings(indexSettings); validateRefreshIntervalSettings(request.settings(), clusterSettings); validateTranslogDurabilitySettings(request.settings(), clusterSettings, settings); - validateCompositeFS(request.settings()); + validateIndexStoreLocality(request.settings()); return indexSettings; } @@ -1682,12 +1682,12 @@ static void validateTranslogDurabilitySettings(Settings requestSettings, Cluster } - public static void validateCompositeFS(Settings indexSettings) { - if (indexSettings.get(IndexModule.INDEX_STORE_TYPE_SETTING.getKey(), "") - .equalsIgnoreCase(IndexModule.Type.COMPOSITEFS.getSettingsKey()) + public static void validateIndexStoreLocality(Settings indexSettings) { + if (indexSettings.get(IndexModule.INDEX_STORE_LOCALITY_SETTING.getKey(), IndexModule.DataLocalityType.FULL.toString()) + .equalsIgnoreCase(IndexModule.DataLocalityType.PARTIAL.toString()) && !FeatureFlags.isEnabled(FeatureFlags.WRITEABLE_REMOTE_INDEX_SETTING)) { throw new IllegalArgumentException( - "ERROR - Composite FS store type can be enabled only if Feature Flag for Writable Remote Index is true" + "index.store.locality can be set to PARTIAL only if Feature Flag for Writable Remote Index is true" ); } } diff --git a/server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java b/server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java index 1c71a1d431e2b..4f5f8d4b1ef5f 100644 --- a/server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java +++ b/server/src/main/java/org/opensearch/common/blobstore/BlobContainer.java @@ -34,7 +34,6 @@ import org.opensearch.common.Nullable; import org.opensearch.common.annotation.ExperimentalApi; -import org.opensearch.common.annotation.PublicApi; import org.opensearch.core.action.ActionListener; import java.io.IOException; @@ -51,7 +50,6 @@ * * @opensearch.internal */ -@PublicApi(since = "2.3.0") public interface BlobContainer { /** @@ -279,7 +277,6 @@ default void writeBlobAtomicWithMetadata( /** * The type representing sort order of blob names */ - @PublicApi(since = "2.3.0") enum BlobNameSortOrder { LEXICOGRAPHIC(Comparator.comparing(BlobMetadata::name)); diff --git a/server/src/main/java/org/opensearch/common/blobstore/BlobMetadata.java b/server/src/main/java/org/opensearch/common/blobstore/BlobMetadata.java index 4fdfb413d24d7..37c70365b6a11 100644 --- a/server/src/main/java/org/opensearch/common/blobstore/BlobMetadata.java +++ b/server/src/main/java/org/opensearch/common/blobstore/BlobMetadata.java @@ -32,14 +32,11 @@ package org.opensearch.common.blobstore; -import org.opensearch.common.annotation.PublicApi; - /** * An interface for providing basic metadata about a blob. * * @opensearch.internal */ -@PublicApi(since = "2.3.0") public interface BlobMetadata { /** diff --git a/server/src/main/java/org/opensearch/common/blobstore/DeleteResult.java b/server/src/main/java/org/opensearch/common/blobstore/DeleteResult.java index 9ee37515cd005..3b424c582ebc6 100644 --- a/server/src/main/java/org/opensearch/common/blobstore/DeleteResult.java +++ b/server/src/main/java/org/opensearch/common/blobstore/DeleteResult.java @@ -32,14 +32,11 @@ package org.opensearch.common.blobstore; -import org.opensearch.common.annotation.PublicApi; - /** * The result of deleting multiple blobs from a {@link BlobStore}. * * @opensearch.internal */ -@PublicApi(since = "2.3.0") public final class DeleteResult { public static final DeleteResult ZERO = new DeleteResult(0, 0); diff --git a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java index 980c432774f6e..f70b558273242 100644 --- a/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/IndexScopedSettings.java @@ -188,6 +188,7 @@ public final class IndexScopedSettings extends AbstractScopedSettings { MapperService.INDEX_MAPPING_FIELD_NAME_LENGTH_LIMIT_SETTING, BitsetFilterCache.INDEX_LOAD_RANDOM_ACCESS_FILTERS_EAGERLY_SETTING, IndexModule.INDEX_STORE_TYPE_SETTING, + IndexModule.INDEX_STORE_LOCALITY_SETTING, IndexModule.INDEX_STORE_PRE_LOAD_SETTING, IndexModule.INDEX_STORE_HYBRID_MMAP_EXTENSIONS, IndexModule.INDEX_STORE_HYBRID_NIO_EXTENSIONS, diff --git a/server/src/main/java/org/opensearch/index/IndexModule.java b/server/src/main/java/org/opensearch/index/IndexModule.java index 16dce2645b2d3..b4fd7666ba45a 100644 --- a/server/src/main/java/org/opensearch/index/IndexModule.java +++ b/server/src/main/java/org/opensearch/index/IndexModule.java @@ -74,7 +74,6 @@ import org.opensearch.index.shard.IndexingOperationListener; import org.opensearch.index.shard.SearchOperationListener; import org.opensearch.index.similarity.SimilarityService; -import org.opensearch.index.store.CompositeDirectoryFactory; import org.opensearch.index.store.FsDirectoryFactory; import org.opensearch.index.store.remote.directory.RemoteSnapshotDirectoryFactory; import org.opensearch.index.store.remote.filecache.FileCache; @@ -108,6 +107,8 @@ import java.util.function.Function; import java.util.function.Supplier; +import static org.apache.logging.log4j.util.Strings.toRootUpperCase; + /** * IndexModule represents the central extension point for index level custom implementations like: *
    @@ -142,6 +143,17 @@ public final class IndexModule { Property.NodeScope ); + /** + * Index setting which used to determine how the data is cached locally fully or partially + */ + public static final Setting INDEX_STORE_LOCALITY_SETTING = new Setting<>( + "index.store.data_locality", + DataLocalityType.FULL.name(), + DataLocalityType::getValueOf, + Property.IndexScope, + Property.NodeScope + ); + public static final Setting INDEX_RECOVERY_TYPE_SETTING = new Setting<>( "index.recovery.type", "", @@ -298,6 +310,7 @@ public Iterator> settings() { private final AtomicBoolean frozen = new AtomicBoolean(false); private final BooleanSupplier allowExpensiveQueries; private final Map recoveryStateFactories; + private final FileCache fileCache; /** * Construct the index module for the index with the specified index settings. The index module contains extension points for plugins @@ -316,7 +329,8 @@ public IndexModule( final Map directoryFactories, final BooleanSupplier allowExpensiveQueries, final IndexNameExpressionResolver expressionResolver, - final Map recoveryStateFactories + final Map recoveryStateFactories, + final FileCache fileCache ) { this.indexSettings = indexSettings; this.analysisRegistry = analysisRegistry; @@ -328,6 +342,7 @@ public IndexModule( this.allowExpensiveQueries = allowExpensiveQueries; this.expressionResolver = expressionResolver; this.recoveryStateFactories = recoveryStateFactories; + this.fileCache = fileCache; } /** @@ -507,8 +522,7 @@ public enum Type { MMAPFS("mmapfs"), SIMPLEFS("simplefs"), FS("fs"), - REMOTE_SNAPSHOT("remote_snapshot"), - COMPOSITEFS("compositefs"); + REMOTE_SNAPSHOT("remote_snapshot"); private final String settingsKey; private final boolean deprecated; @@ -579,6 +593,40 @@ public boolean match(Settings settings) { } } + /** + * Indicates the locality of the data - whether it will be cached fully or partially + */ + public enum DataLocalityType { + /** + * Indicates that all the data will be cached locally + */ + FULL, + /** + * Indicates that only a subset of the data will be cached locally + */ + PARTIAL; + + private static final Map LOCALITY_TYPES; + + static { + final Map localityTypes = new HashMap<>(values().length); + for (final DataLocalityType dataLocalityType : values()) { + localityTypes.put(dataLocalityType.name(), dataLocalityType); + } + LOCALITY_TYPES = Collections.unmodifiableMap(localityTypes); + } + + public static DataLocalityType getValueOf(final String localityType) { + Objects.requireNonNull(localityType, "No locality type given."); + final String localityTypeName = toRootUpperCase(localityType.trim()); + final DataLocalityType type = LOCALITY_TYPES.get(localityTypeName); + if (type != null) { + return type; + } + throw new IllegalArgumentException("Unknown Locality Type constant [" + localityType + "]."); + } + } + public static Type defaultStoreType(final boolean allowMmap) { if (allowMmap && Constants.JRE_IS_64BIT && MMapDirectory.UNMAP_SUPPORTED) { return Type.HYBRIDFS; @@ -667,7 +715,8 @@ public IndexService newIndexService( translogFactorySupplier, clusterDefaultRefreshIntervalSupplier, recoverySettings, - remoteStoreSettings + remoteStoreSettings, + fileCache ); success = true; return indexService; @@ -790,9 +839,6 @@ public static Map createBuiltInDirect new RemoteSnapshotDirectoryFactory(repositoriesService, threadPool, remoteStoreFileCache) ); break; - case COMPOSITEFS: - factories.put(type.getSettingsKey(), new CompositeDirectoryFactory(repositoriesService, remoteStoreFileCache)); - break; default: throw new IllegalStateException("No directory factory mapping for built-in type " + type); } diff --git a/server/src/main/java/org/opensearch/index/IndexService.java b/server/src/main/java/org/opensearch/index/IndexService.java index 61309b4e29982..08a9779a23c02 100644 --- a/server/src/main/java/org/opensearch/index/IndexService.java +++ b/server/src/main/java/org/opensearch/index/IndexService.java @@ -39,6 +39,7 @@ import org.apache.lucene.search.Sort; import org.apache.lucene.store.AlreadyClosedException; import org.apache.lucene.store.Directory; +import org.apache.lucene.store.FSDirectory; import org.apache.lucene.util.Accountable; import org.opensearch.client.Client; import org.opensearch.cluster.metadata.IndexMetadata; @@ -55,6 +56,7 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.BigArrays; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.concurrent.AbstractAsyncTask; import org.opensearch.common.util.concurrent.AbstractRunnable; import org.opensearch.common.util.io.IOUtils; @@ -92,8 +94,10 @@ import org.opensearch.index.shard.ShardPath; import org.opensearch.index.similarity.SimilarityService; import org.opensearch.index.store.CompositeDirectory; +import org.opensearch.index.store.RemoteSegmentStoreDirectory; import org.opensearch.index.store.RemoteSegmentStoreDirectoryFactory; import org.opensearch.index.store.Store; +import org.opensearch.index.store.remote.filecache.FileCache; import org.opensearch.index.translog.Translog; import org.opensearch.index.translog.TranslogFactory; import org.opensearch.indices.RemoteStoreSettings; @@ -189,6 +193,7 @@ public class IndexService extends AbstractIndexComponent implements IndicesClust private final Supplier clusterDefaultRefreshIntervalSupplier; private final RecoverySettings recoverySettings; private final RemoteStoreSettings remoteStoreSettings; + private final FileCache fileCache; public IndexService( IndexSettings indexSettings, @@ -224,7 +229,8 @@ public IndexService( BiFunction translogFactorySupplier, Supplier clusterDefaultRefreshIntervalSupplier, RecoverySettings recoverySettings, - RemoteStoreSettings remoteStoreSettings + RemoteStoreSettings remoteStoreSettings, + FileCache fileCache ) { super(indexSettings); this.allowExpensiveQueries = allowExpensiveQueries; @@ -302,6 +308,7 @@ public IndexService( this.translogFactorySupplier = translogFactorySupplier; this.recoverySettings = recoverySettings; this.remoteStoreSettings = remoteStoreSettings; + this.fileCache = fileCache; updateFsyncTaskIfNecessary(); } @@ -531,10 +538,23 @@ public synchronized IndexShard createShard( } } - Directory directory = directoryFactory.newDirectory(this.indexSettings, path); - if (directory instanceof CompositeDirectory) { - ((CompositeDirectory) directory).setRemoteDirectory(remoteDirectory); + Directory directory = null; + if (FeatureFlags.isEnabled(FeatureFlags.WRITEABLE_REMOTE_INDEX_SETTING) && + // TODO : Need to remove this check after support for hot indices is added in Composite Directory + this.indexSettings.isStoreLocalityPartial()) { + /** + * Currently Composite Directory only supports local directory to be of type FSDirectory + * The reason is that FileCache currently has it key type as Path + * Composite Directory currently uses FSDirectory's getDirectory() method to fetch and use the Path for operating on FileCache + * TODO : Refactor FileCache to have key in form of String instead of Path. Once that is done we can remove this assertion + */ + Directory localDirectory = directoryFactory.newDirectory(this.indexSettings, path); + assert localDirectory instanceof FSDirectory : "For Composite Directory, local directory must be of type FSDirectory"; + directory = new CompositeDirectory((FSDirectory) localDirectory, (RemoteSegmentStoreDirectory) remoteDirectory, fileCache); + } else { + directory = directoryFactory.newDirectory(this.indexSettings, path); } + store = new Store( shardId, this.indexSettings, diff --git a/server/src/main/java/org/opensearch/index/IndexSettings.java b/server/src/main/java/org/opensearch/index/IndexSettings.java index 9d8ab6815eecc..27d194b59bc82 100644 --- a/server/src/main/java/org/opensearch/index/IndexSettings.java +++ b/server/src/main/java/org/opensearch/index/IndexSettings.java @@ -725,6 +725,7 @@ public static IndexMergePolicy fromString(String text) { private final int numberOfShards; private final ReplicationType replicationType; private final boolean isRemoteStoreEnabled; + private final boolean isStoreLocalityPartial; private volatile TimeValue remoteTranslogUploadBufferInterval; private final String remoteStoreTranslogRepository; private final String remoteStoreRepository; @@ -914,6 +915,10 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti numberOfShards = settings.getAsInt(IndexMetadata.SETTING_NUMBER_OF_SHARDS, null); replicationType = IndexMetadata.INDEX_REPLICATION_TYPE_SETTING.get(settings); isRemoteStoreEnabled = settings.getAsBoolean(IndexMetadata.SETTING_REMOTE_STORE_ENABLED, false); + isStoreLocalityPartial = settings.get( + IndexModule.INDEX_STORE_LOCALITY_SETTING.getKey(), + IndexModule.DataLocalityType.FULL.toString() + ).equalsIgnoreCase(IndexModule.DataLocalityType.PARTIAL.toString()); remoteStoreTranslogRepository = settings.get(IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY); remoteTranslogUploadBufferInterval = INDEX_REMOTE_TRANSLOG_BUFFER_INTERVAL_SETTING.get(settings); remoteStoreRepository = settings.get(IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY); @@ -1271,6 +1276,13 @@ public String getRemoteStoreTranslogRepository() { return remoteStoreTranslogRepository; } + /** + * Returns true if the store locality is partial + */ + public boolean isStoreLocalityPartial() { + return isStoreLocalityPartial; + } + /** * Returns true if this is remote/searchable snapshot */ diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java index 291b499eeb1b4..e668ac31e57ee 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java @@ -10,133 +10,165 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.apache.lucene.store.Directory; import org.apache.lucene.store.FSDirectory; import org.apache.lucene.store.FilterDirectory; import org.apache.lucene.store.IOContext; import org.apache.lucene.store.IndexInput; import org.apache.lucene.store.IndexOutput; import org.apache.lucene.store.Lock; +import org.apache.lucene.store.LockObtainFailedException; import org.opensearch.index.store.remote.file.OnDemandCompositeBlockIndexInput; import org.opensearch.index.store.remote.filecache.FileCache; -import org.opensearch.index.store.remote.utils.filetracker.FileState; -import org.opensearch.index.store.remote.utils.filetracker.FileType; +import org.opensearch.index.store.remote.utils.FileType; +import java.io.FileNotFoundException; import java.io.IOException; +import java.nio.file.NoSuchFileException; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.HashSet; -import java.util.List; import java.util.Set; -import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.stream.Collectors; +/** + * Composite Directory will contain both local and remote directory + * Consumers of Composite directory need not worry whether file is in local or remote + * All such abstractions will be handled by the Composite directory itself + * Implements all required methods by Directory abstraction + */ public class CompositeDirectory extends FilterDirectory { private static final Logger logger = LogManager.getLogger(CompositeDirectory.class); - private final FSDirectory localDirectory; - private final RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter; + private final RemoteSegmentStoreDirectory remoteDirectory; private final FileCache fileCache; - private final AtomicBoolean isRemoteDirectorySet; private final ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock(); private final ReentrantReadWriteLock.WriteLock writeLock = readWriteLock.writeLock(); private final ReentrantReadWriteLock.ReadLock readLock = readWriteLock.readLock(); - public CompositeDirectory(FSDirectory localDirectory, FileCache fileCache) { + /** + * Constructor to initialise the composite directory + * @param localDirectory corresponding to the local FSDirectory + * @param remoteDirectory corresponding to the remote directory + * @param fileCache used to cache the remote files locally + */ + public CompositeDirectory(FSDirectory localDirectory, RemoteSegmentStoreDirectory remoteDirectory, FileCache fileCache) { super(localDirectory); this.localDirectory = localDirectory; + this.remoteDirectory = remoteDirectory; this.fileCache = fileCache; - this.remoteStoreFileTrackerAdapter = new CompositeDirectoryRemoteStoreFileTrackerAdapter(fileCache); - isRemoteDirectorySet = new AtomicBoolean(false); - } - - public void setRemoteDirectory(Directory remoteDirectory) { - logger.trace("Setting remote Directory ..."); - if (!isRemoteDirectorySet()) { - ((CompositeDirectoryRemoteStoreFileTrackerAdapter) remoteStoreFileTrackerAdapter).setRemoteDirectory(remoteDirectory); - isRemoteDirectorySet.set(true); - } } + /** + * Returns names of all files stored in this directory in sorted order + * Does not include locally stored block files (having _block_ in their names) + * + * @throws IOException in case of I/O error + */ @Override public String[] listAll() throws IOException { logger.trace("listAll() called ..."); readLock.lock(); try { - String[] remoteFiles = new String[0]; String[] localFiles = localDirectory.listAll(); - if (isRemoteDirectorySet()) remoteFiles = ((CompositeDirectoryRemoteStoreFileTrackerAdapter) remoteStoreFileTrackerAdapter) - .getRemoteFiles(); - logger.trace("LocalDirectory files : " + Arrays.toString(localFiles)); - logger.trace("Remote Directory files : " + Arrays.toString(remoteFiles)); + logger.trace("LocalDirectory files : {}", () -> Arrays.toString(localFiles)); Set allFiles = new HashSet<>(Arrays.asList(localFiles)); - allFiles.addAll(Arrays.asList(remoteFiles)); - - Set localLuceneFiles = allFiles.stream().filter(file -> !isBlockFile(file)).collect(Collectors.toUnmodifiableSet()); + if (remoteDirectory != null) { + String[] remoteFiles = getRemoteFiles(); + allFiles.addAll(Arrays.asList(remoteFiles)); + logger.trace("Remote Directory files : {}", () -> Arrays.toString(remoteFiles)); + } + Set localLuceneFiles = allFiles.stream() + .filter(file -> !FileType.isBlockFile(file)) + .collect(Collectors.toUnmodifiableSet()); String[] files = new String[localLuceneFiles.size()]; localLuceneFiles.toArray(files); Arrays.sort(files); - - logger.trace("listAll() returns : " + Arrays.toString(files)); - + logger.trace("listAll() returns : {}", () -> Arrays.toString(files)); return files; } finally { readLock.unlock(); } } + /** + * Removes an existing file in the directory. + * Throws {@link NoSuchFileException} or {@link FileNotFoundException} in case file is not present locally and in remote as well + * @param name the name of an existing file. + * @throws IOException in case of I/O error + */ @Override public void deleteFile(String name) throws IOException { - logger.trace("deleteFile() called " + name); + logger.trace("deleteFile() called {}", name); writeLock.lock(); try { localDirectory.deleteFile(name); - remoteStoreFileTrackerAdapter.removeFileFromTracker(name); fileCache.remove(localDirectory.getDirectory().resolve(name)); - logFileTracker(); + } catch (NoSuchFileException | FileNotFoundException e) { + logger.trace("File {} not found in local, trying to delete from Remote", name); + try { + remoteDirectory.deleteFile(name); + } finally { + writeLock.unlock(); + } } finally { writeLock.unlock(); } } + /** + * Returns the byte length of a file in the directory. + * Throws {@link NoSuchFileException} or {@link FileNotFoundException} in case file is not present locally and in remote as well + * @param name the name of an existing file. + * @throws IOException in case of I/O error + */ @Override public long fileLength(String name) throws IOException { - logger.trace("fileLength() called " + name); + logger.trace("fileLength() called {}", name); readLock.lock(); try { - if (remoteStoreFileTrackerAdapter.getFileState(name).equals(FileState.DISK)) { - logger.trace("fileLength from Local " + localDirectory.fileLength(name)); - return localDirectory.fileLength(name); + long fileLength; + if (Arrays.asList(getRemoteFiles()).contains(name) == false) { + fileLength = localDirectory.fileLength(name); + logger.trace("fileLength from Local {}", fileLength); } else { - logger.trace( - "fileLength from Remote " - + ((CompositeDirectoryRemoteStoreFileTrackerAdapter) remoteStoreFileTrackerAdapter).getFileLength(name) - ); - return ((CompositeDirectoryRemoteStoreFileTrackerAdapter) remoteStoreFileTrackerAdapter).getFileLength(name); + fileLength = remoteDirectory.fileLength(name); + logger.trace("fileLength from Remote {}", fileLength); } + return fileLength; } finally { readLock.unlock(); } } + /** + * Creates a new, empty file in the directory and returns an {@link IndexOutput} instance for + * appending data to this file. + * @param name the name of the file to create. + * @throws IOException in case of I/O error + */ @Override public IndexOutput createOutput(String name, IOContext context) throws IOException { - logger.trace("createOutput() called " + name); + logger.trace("createOutput() called {}", name); writeLock.lock(); try { - remoteStoreFileTrackerAdapter.trackFile(name, FileState.DISK, FileType.NON_BLOCK); - logFileTracker(); return localDirectory.createOutput(name, context); } finally { writeLock.unlock(); } } + /** + * Creates a new, empty, temporary file in the directory and returns an {@link IndexOutput} + * instance for appending data to this file. + * + *

    The temporary file name (accessible via {@link IndexOutput#getName()}) will start with + * {@code prefix}, end with {@code suffix} and have a reserved file extension {@code .tmp}. + */ @Override public IndexOutput createTempOutput(String prefix, String suffix, IOContext context) throws IOException { - logger.trace("createOutput() called " + prefix + "," + suffix); + logger.trace("createTempOutput() called {} , {}", prefix, suffix); writeLock.lock(); try { return localDirectory.createTempOutput(prefix, suffix, context); @@ -145,22 +177,31 @@ public IndexOutput createTempOutput(String prefix, String suffix, IOContext cont } } + /** + * Ensures that any writes to these files are moved to stable storage (made durable). + * @throws IOException in case of I/O error + */ @Override public void sync(Collection names) throws IOException { - logger.trace("sync() called " + names); + logger.trace("sync() called {}", names); writeLock.lock(); try { Collection newLocalFiles = new ArrayList<>(); + Collection remoteFiles = Arrays.asList(getRemoteFiles()); for (String name : names) { - if (remoteStoreFileTrackerAdapter.getFileState(name).equals(FileState.DISK)) newLocalFiles.add(name); + if (remoteFiles.contains(name) == false) newLocalFiles.add(name); } - logger.trace("Synced files : " + newLocalFiles); + logger.trace("Synced files : {}", newLocalFiles); localDirectory.sync(newLocalFiles); } finally { writeLock.unlock(); } } + /** + * Ensures that directory metadata, such as recent file renames, are moved to stable storage. + * @throws IOException in case of I/O error + */ @Override public void syncMetaData() throws IOException { logger.trace("syncMetaData() called "); @@ -172,108 +213,133 @@ public void syncMetaData() throws IOException { } } + /** + * Renames {@code source} file to {@code dest} file where {@code dest} must not already exist in + * the directory. + * @throws IOException in case of I/O error + */ @Override public void rename(String source, String dest) throws IOException { - logger.trace("rename() called " + source + " -> " + dest); + logger.trace("rename() called {}, {}", source, dest); writeLock.lock(); try { localDirectory.rename(source, dest); - remoteStoreFileTrackerAdapter.trackFile( - dest, - remoteStoreFileTrackerAdapter.getFileState(source), - remoteStoreFileTrackerAdapter.getFileType(source) - ); - remoteStoreFileTrackerAdapter.removeFileFromTracker(source); - logFileTracker(); } finally { writeLock.unlock(); } } + /** + * Opens a stream for reading an existing file. + * Check whether the file is present locally or in remote and return the IndexInput accordingly + * @param name the name of an existing file. + * @throws IOException in case of I/O error + */ @Override public IndexInput openInput(String name, IOContext context) throws IOException { - logger.trace("openInput() called " + name); + logger.trace("openInput() called {}", name); writeLock.lock(); try { - if (!remoteStoreFileTrackerAdapter.isFilePresent(name)) { - // Print filename to check which file is not present in tracker - logger.trace("File not found in tracker"); + if (Arrays.asList(getRemoteFiles()).contains(name) == false) { + // If file has not yet been uploaded to Remote Store, fetch it from the local directory + logger.trace("File found in disk"); return localDirectory.openInput(name, context); + } else { + // If file has been uploaded to the Remote Store, fetch it from the Remote Store in blocks via + // OnDemandCompositeBlockIndexInput + logger.trace("File to be fetched from Remote"); + return new OnDemandCompositeBlockIndexInput(remoteDirectory, name, localDirectory, fileCache, context); } - IndexInput indexInput = null; - switch (remoteStoreFileTrackerAdapter.getFileState(name)) { - case DISK: - logger.trace("File found in disk "); - indexInput = localDirectory.openInput(name, context); - break; - - case REMOTE: - logger.trace("File to be fetched from Remote "); - indexInput = new OnDemandCompositeBlockIndexInput(remoteStoreFileTrackerAdapter, name, localDirectory); - break; - } - return indexInput; } finally { writeLock.unlock(); } } + /** + * Acquires and returns a {@link Lock} for a file with the given name. + * @param name the name of the lock file + * @throws LockObtainFailedException (optional specific exception) if the lock could not be + * obtained because it is currently held elsewhere. + * @throws IOException in case of I/O error + */ @Override public Lock obtainLock(String name) throws IOException { - logger.trace("obtainLock() called " + name); - return localDirectory.obtainLock(name); + logger.trace("obtainLock() called {}", name); + writeLock.lock(); + try { + return localDirectory.obtainLock(name); + } finally { + writeLock.unlock(); + } } + /** + * Closes the directory + * @throws IOException in case of I/O error + */ @Override public void close() throws IOException { - localDirectory.close(); + writeLock.lock(); + try { + localDirectory.close(); + } finally { + writeLock.unlock(); + } } + /** + * Returns a set of files currently pending deletion in this directory. + * @throws IOException in case of I/O error + */ @Override public Set getPendingDeletions() throws IOException { - return localDirectory.getPendingDeletions(); + readLock.lock(); + try { + return localDirectory.getPendingDeletions(); + } finally { + readLock.unlock(); + } } + /** + * Function to perform operations once files have been uploaded to Remote Store + * Currently deleting the local files here, as once uploaded to Remote, local files are safe to delete + * @param files : recent files which have been successfully uploaded to Remote Store + * @throws IOException in case of I/O error + */ public void afterSyncToRemote(Collection files) throws IOException { - logger.trace("afterSyncToRemote called for " + files); - if (!isRemoteDirectorySet()) throw new UnsupportedOperationException( - "Cannot perform afterSyncToRemote if Remote Directory is not set" - ); - List delFiles = new ArrayList<>(); + logger.trace("afterSyncToRemote called for {}", files); + if (remoteDirectory == null) { + logger.trace("afterSyncToRemote called even though remote directory is not set"); + return; + } for (String fileName : files) { - if (isSegmentsOrLockFile(fileName)) continue; writeLock.lock(); try { - if (remoteStoreFileTrackerAdapter.isFilePresent(fileName) - && remoteStoreFileTrackerAdapter.getFileState(fileName).equals(FileState.DISK)) { - remoteStoreFileTrackerAdapter.updateFileState(fileName, FileState.REMOTE); - } + localDirectory.deleteFile(fileName); } finally { writeLock.unlock(); } - localDirectory.deleteFile(fileName); - delFiles.add(fileName); } - logger.trace("Files removed form local " + delFiles); - logFileTracker(); - } - - private boolean isSegmentsOrLockFile(String fileName) { - if (fileName.startsWith("segments_") || fileName.endsWith(".si") || fileName.endsWith(".lock")) return true; - return false; - } - - private boolean isBlockFile(String fileName) { - if (fileName.contains("_block_")) return true; - return false; - } - - private boolean isRemoteDirectorySet() { - return isRemoteDirectorySet.get(); } - public void logFileTracker() { - String res = ((CompositeDirectoryRemoteStoreFileTrackerAdapter) remoteStoreFileTrackerAdapter).logFileTracker(); - logger.trace(res); + /** + * Return the list of files present in Remote + */ + private String[] getRemoteFiles() { + String[] remoteFiles; + try { + remoteFiles = remoteDirectory.listAll(); + } catch (Exception e) { + /** + * There are two scenarios where the listAll() call on remote directory fails: + * - When remote directory is not set + * - When init() of remote directory has not yet been called (which results in NullPointerException while calling listAll() for RemoteSegmentStoreDirectory) + * + * Returning an empty list in these scenarios + */ + remoteFiles = new String[0]; + } + return remoteFiles; } } diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java deleted file mode 100644 index 8256e22eadbca..0000000000000 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryFactory.java +++ /dev/null @@ -1,37 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.index.store; - -import org.apache.lucene.store.Directory; -import org.apache.lucene.store.FSDirectory; -import org.opensearch.index.IndexSettings; -import org.opensearch.index.shard.ShardPath; -import org.opensearch.index.store.remote.filecache.FileCache; -import org.opensearch.plugins.IndexStorePlugin; -import org.opensearch.repositories.RepositoriesService; - -import java.io.IOException; -import java.util.function.Supplier; - -public class CompositeDirectoryFactory implements IndexStorePlugin.DirectoryFactory { - - private final Supplier repositoriesService; - private final FileCache remoteStoreFileCache; - - public CompositeDirectoryFactory(Supplier repositoriesService, FileCache remoteStoreFileCache) { - this.repositoriesService = repositoriesService; - this.remoteStoreFileCache = remoteStoreFileCache; - } - - @Override - public Directory newDirectory(IndexSettings indexSettings, ShardPath shardPath) throws IOException { - final FSDirectory primaryDirectory = FSDirectory.open(shardPath.resolveIndex()); - return new CompositeDirectory(primaryDirectory, remoteStoreFileCache); - } -} diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java deleted file mode 100644 index 675c6b8c39871..0000000000000 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectoryRemoteStoreFileTrackerAdapter.java +++ /dev/null @@ -1,121 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.index.store; - -import org.apache.lucene.store.Directory; -import org.apache.lucene.store.IndexInput; -import org.opensearch.index.store.remote.filecache.FileCache; -import org.opensearch.index.store.remote.utils.BlobFetchRequest; -import org.opensearch.index.store.remote.utils.TransferManager; -import org.opensearch.index.store.remote.utils.filetracker.FileState; -import org.opensearch.index.store.remote.utils.filetracker.FileTrackingInfo; -import org.opensearch.index.store.remote.utils.filetracker.FileType; - -import java.io.IOException; -import java.util.HashMap; -import java.util.Map; - -public class CompositeDirectoryRemoteStoreFileTrackerAdapter implements RemoteStoreFileTrackerAdapter { - - private FileCache fileCache; - private Map fileTracker; - private RemoteSegmentStoreDirectory remoteDirectory; - - public CompositeDirectoryRemoteStoreFileTrackerAdapter(FileCache fileCache) { - this.fileCache = fileCache; - remoteDirectory = null; - this.fileTracker = new HashMap<>(); - } - - public void setRemoteDirectory(Directory remoteDirectory) { - this.remoteDirectory = (RemoteSegmentStoreDirectory) remoteDirectory; - } - - public String getUploadedFileName(String name) { - return remoteDirectory.getExistingRemoteFilename(name); - } - - public long getFileLength(String name) { - try { - return remoteDirectory.fileLength(name); - } catch (IOException e) { - throw new RuntimeException(e); - } - } - - @Override - public IndexInput fetchBlob(BlobFetchRequest blobFetchRequest) throws IOException { - return new TransferManager(remoteDirectory.getDataDirectoryBlobContainer(), fileCache).fetchBlob(blobFetchRequest); - } - - public void trackFile(String name, FileState fileState, FileType fileType) { - if (!fileTracker.containsKey(name)) { - fileTracker.put(name, new FileTrackingInfo(fileState, fileType)); - } - } - - public void updateFileType(String name, FileType fileType) { - FileTrackingInfo fileTrackingInfo = fileTracker.get(name); - if (fileTrackingInfo != null) { - fileTracker.put(name, new FileTrackingInfo(fileTrackingInfo.getFileState(), fileType)); - } - } - - public void updateFileState(String name, FileState fileState) { - FileTrackingInfo fileTrackingInfo = fileTracker.get(name); - if (fileTrackingInfo != null) { - fileTracker.put(name, new FileTrackingInfo(fileState, fileTrackingInfo.getFileType())); - } - } - - public void removeFileFromTracker(String name) { - fileTracker.remove(name); - } - - public FileState getFileState(String name) { - if (!fileTracker.containsKey(name)) { - return null; - } - return fileTracker.get(name).getFileState(); - } - - public FileType getFileType(String name) { - if (!fileTracker.containsKey(name)) { - return null; - } - return fileTracker.get(name).getFileType(); - } - - public boolean isFilePresent(String name) { - return fileTracker.containsKey(name); - } - - public String[] getRemoteFiles() throws IOException { - String[] remoteFiles; - try { - remoteFiles = remoteDirectory.listAll(); - } catch (Exception e) { - remoteFiles = new String[0]; - } - return remoteFiles; - } - - public String logFileTracker() { - String result = ""; - for (Map.Entry entry : fileTracker.entrySet()) { - result += entry.getKey() - + " : " - + entry.getValue().getFileType().name() - + " , " - + entry.getValue().getFileState().name() - + "\n"; - } - return result; - } -} diff --git a/server/src/main/java/org/opensearch/index/store/RemoteDirectory.java b/server/src/main/java/org/opensearch/index/store/RemoteDirectory.java index 345583bbbd1be..e6d5af04e0cb7 100644 --- a/server/src/main/java/org/opensearch/index/store/RemoteDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/RemoteDirectory.java @@ -28,8 +28,10 @@ import org.opensearch.common.blobstore.transfer.RemoteTransferContainer; import org.opensearch.common.blobstore.transfer.stream.OffsetRangeIndexInputStream; import org.opensearch.common.blobstore.transfer.stream.OffsetRangeInputStream; +import org.opensearch.common.lucene.store.ByteArrayIndexInput; import org.opensearch.core.action.ActionListener; import org.opensearch.index.store.exception.ChecksumCombinationException; +import org.opensearch.index.store.remote.utils.BlockIOContext; import java.io.FileNotFoundException; import java.io.IOException; @@ -198,10 +200,18 @@ public IndexInput openInput(String name, IOContext context) throws IOException { public IndexInput openInput(String name, long fileLength, IOContext context) throws IOException { InputStream inputStream = null; try { - inputStream = blobContainer.readBlob(name); - return new RemoteIndexInput(name, downloadRateLimiter.apply(inputStream), fileLength); + if (context instanceof BlockIOContext) { + long position = ((BlockIOContext) context).getBlockStart(); + long length = ((BlockIOContext) context).getBlockSize(); + inputStream = blobContainer.readBlob(name, position, length); + byte[] bytes = downloadRateLimiter.apply(inputStream).readAllBytes(); + return new ByteArrayIndexInput(name, bytes); + } else { + inputStream = blobContainer.readBlob(name); + return new RemoteIndexInput(name, downloadRateLimiter.apply(inputStream), fileLength); + } } catch (Exception e) { - // Incase the RemoteIndexInput creation fails, close the input stream to avoid file handler leak. + // In case the RemoteIndexInput creation fails, close the input stream to avoid file handler leak. if (inputStream != null) { try { inputStream.close(); diff --git a/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java b/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java index 8b65110dd3903..ec1163fe91b6c 100644 --- a/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectory.java @@ -25,7 +25,6 @@ import org.apache.lucene.util.Version; import org.opensearch.common.UUIDs; import org.opensearch.common.annotation.PublicApi; -import org.opensearch.common.blobstore.BlobContainer; import org.opensearch.common.collect.Tuple; import org.opensearch.common.io.VersionedCodecStreamWrapper; import org.opensearch.common.logging.Loggers; @@ -137,10 +136,6 @@ public RemoteSegmentStoreDirectory( init(); } - public BlobContainer getDataDirectoryBlobContainer() { - return remoteDataDirectory.getBlobContainer(); - } - /** * Initializes the cache which keeps track of all the segment files uploaded to the remote segment store. * As this cache is specific to an instance of RemoteSegmentStoreDirectory, it is possible that cache becomes stale @@ -703,7 +698,7 @@ private String getChecksumOfLocalFile(Directory directory, String file) throws I } } - public String getExistingRemoteFilename(String localFilename) { + private String getExistingRemoteFilename(String localFilename) { if (segmentsUploadedToRemoteStore.containsKey(localFilename)) { return segmentsUploadedToRemoteStore.get(localFilename).uploadedFilename; } else { diff --git a/server/src/main/java/org/opensearch/index/store/RemoteStoreFileTrackerAdapter.java b/server/src/main/java/org/opensearch/index/store/RemoteStoreFileTrackerAdapter.java deleted file mode 100644 index 29f05c8dc60e8..0000000000000 --- a/server/src/main/java/org/opensearch/index/store/RemoteStoreFileTrackerAdapter.java +++ /dev/null @@ -1,34 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.index.store; - -import org.apache.lucene.store.IndexInput; -import org.opensearch.index.store.remote.utils.BlobFetchRequest; -import org.opensearch.index.store.remote.utils.filetracker.FileState; -import org.opensearch.index.store.remote.utils.filetracker.FileType; - -import java.io.IOException; - -public interface RemoteStoreFileTrackerAdapter { - IndexInput fetchBlob(BlobFetchRequest blobFetchRequest) throws IOException; - - void trackFile(String name, FileState fileState, FileType fileType); - - void updateFileType(String name, FileType fileType); - - void updateFileState(String name, FileState fileState); - - void removeFileFromTracker(String name); - - FileState getFileState(String name); - - FileType getFileType(String name); - - boolean isFilePresent(String name); -} diff --git a/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java b/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java index 8792e2294a81b..b2af3e7305c5d 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java +++ b/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java @@ -11,97 +11,122 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.lucene.store.FSDirectory; +import org.apache.lucene.store.IOContext; import org.apache.lucene.store.IndexInput; -import org.opensearch.index.store.CompositeDirectoryRemoteStoreFileTrackerAdapter; -import org.opensearch.index.store.RemoteStoreFileTrackerAdapter; -import org.opensearch.index.store.remote.utils.BlobFetchRequest; +import org.opensearch.index.store.RemoteSegmentStoreDirectory; +import org.opensearch.index.store.remote.filecache.CachedIndexInput; +import org.opensearch.index.store.remote.filecache.FileCache; +import org.opensearch.index.store.remote.utils.BlockIOContext; +import java.io.BufferedOutputStream; +import java.io.FileNotFoundException; import java.io.IOException; -import java.util.ArrayList; -import java.util.Arrays; +import java.io.OutputStream; +import java.nio.file.Files; +import java.nio.file.NoSuchFileException; +import java.nio.file.Path; +import java.util.concurrent.atomic.AtomicBoolean; +/** + * OnDemandCompositeBlockIndexInput is used by the Composite Directory to read data in blocks from Remote and cache those blocks in FileCache + */ public class OnDemandCompositeBlockIndexInput extends OnDemandBlockIndexInput { private static final Logger logger = LogManager.getLogger(OnDemandCompositeBlockIndexInput.class); - private final RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter; + private final RemoteSegmentStoreDirectory remoteDirectory; private final String fileName; private final Long originalFileSize; - private final FSDirectory directory; + private final FSDirectory localDirectory; + private final IOContext context; + private final FileCache fileCache; public OnDemandCompositeBlockIndexInput( - RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter, + RemoteSegmentStoreDirectory remoteDirectory, String fileName, - FSDirectory directory - ) { + FSDirectory localDirectory, + FileCache fileCache, + IOContext context + ) throws IOException { this( OnDemandBlockIndexInput.builder() .resourceDescription("OnDemandCompositeBlockIndexInput") .isClone(false) .offset(0L) - .length(getFileLength(remoteStoreFileTrackerAdapter, fileName)), - remoteStoreFileTrackerAdapter, + .length(remoteDirectory.fileLength(fileName)), + remoteDirectory, fileName, - directory + localDirectory, + fileCache, + context ); } public OnDemandCompositeBlockIndexInput( Builder builder, - RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter, + RemoteSegmentStoreDirectory remoteDirectory, String fileName, - FSDirectory directory - ) { + FSDirectory localDirectory, + FileCache fileCache, + IOContext context + ) throws IOException { super(builder); - this.remoteStoreFileTrackerAdapter = remoteStoreFileTrackerAdapter; - this.directory = directory; + this.remoteDirectory = remoteDirectory; + this.localDirectory = localDirectory; this.fileName = fileName; - originalFileSize = getFileLength(remoteStoreFileTrackerAdapter, fileName); + this.fileCache = fileCache; + this.context = context; + originalFileSize = remoteDirectory.fileLength(fileName); } @Override protected OnDemandCompositeBlockIndexInput buildSlice(String sliceDescription, long offset, long length) { - return new OnDemandCompositeBlockIndexInput( - OnDemandBlockIndexInput.builder() - .blockSizeShift(blockSizeShift) - .isClone(true) - .offset(this.offset + offset) - .length(length) - .resourceDescription(sliceDescription), - remoteStoreFileTrackerAdapter, - fileName, - directory - ); + try { + return new OnDemandCompositeBlockIndexInput( + OnDemandBlockIndexInput.builder() + .blockSizeShift(blockSizeShift) + .isClone(true) + .offset(this.offset + offset) + .length(length) + .resourceDescription(sliceDescription), + remoteDirectory, + fileName, + localDirectory, + fileCache, + context + ); + } catch (IOException e) { + throw new RuntimeException(e); + } } @Override protected IndexInput fetchBlock(int blockId) throws IOException { - logger.trace("fetchBlock called with blockId -> " + blockId); - final String uploadedFileName = ((CompositeDirectoryRemoteStoreFileTrackerAdapter) remoteStoreFileTrackerAdapter) - .getUploadedFileName(fileName); + logger.trace("fetchBlock called with blockId -> {}", blockId); final String blockFileName = fileName + "_block_" + blockId; final long blockStart = getBlockStart(blockId); final long length = getActualBlockSize(blockId); logger.trace( - "File: " - + uploadedFileName - + ", Block File: " - + blockFileName - + ", BlockStart: " - + blockStart - + ", Length: " - + length - + ", BlockSize: " - + blockSize - + ", OriginalFileSize: " - + originalFileSize + "File: {} , Block File: {} , Length: {} , BlockSize: {} , OriginalFileSize: {}", + fileName, + blockFileName, + blockStart, + length, + originalFileSize ); - BlobFetchRequest.BlobPart blobPart = new BlobFetchRequest.BlobPart(uploadedFileName, blockStart, length); - BlobFetchRequest blobFetchRequest = BlobFetchRequest.builder() - .blobParts(new ArrayList<>(Arrays.asList(blobPart))) - .directory(directory) - .fileName(blockFileName) - .build(); - return remoteStoreFileTrackerAdapter.fetchBlob(blobFetchRequest); + Path blockFilePath = getLocalFilePath(blockFileName); + final CachedIndexInput cacheEntry = fileCache.compute(blockFilePath, (path, cachedIndexInput) -> { + if (cachedIndexInput == null || cachedIndexInput.isClosed()) { + // Doesn't exist or is closed, either way create a new one + IndexInput indexInput = fetchIndexInput(blockFileName, blockStart, length); + return new CachedIndexInputImpl(indexInput); + } else { + logger.trace("Block already present in cache"); + // already in the cache and ready to be used (open) + return cachedIndexInput; + } + }); + + return cacheEntry.getIndexInput(); } @Override @@ -116,7 +141,89 @@ private long getActualBlockSize(int blockId) { return (blockId != getBlock(originalFileSize - 1)) ? blockSize : getBlockOffset(originalFileSize - 1) + 1; } - private static long getFileLength(RemoteStoreFileTrackerAdapter remoteStoreFileTrackerAdapter, String fileName) { - return ((CompositeDirectoryRemoteStoreFileTrackerAdapter) remoteStoreFileTrackerAdapter).getFileLength(fileName); + private Path getLocalFilePath(String file) { + return localDirectory.getDirectory().resolve(file); + } + + private IndexInput fetchIndexInput(String blockFileName, long start, long length) { + IndexInput indexInput; + Path filePath = getLocalFilePath(blockFileName); + try { + // Fetch from local if block file is present locally in disk + indexInput = localDirectory.openInput(blockFileName, IOContext.READ); + logger.trace("Block file present locally, just putting it in cache"); + } catch (FileNotFoundException | NoSuchFileException e) { + logger.trace("Block file not present locally, fetching from Remote"); + // If block file is not present locally in disk, fetch from remote and persist the block file in disk + try ( + OutputStream fileOutputStream = Files.newOutputStream(filePath); + OutputStream localFileOutputStream = new BufferedOutputStream(fileOutputStream) + ) { + logger.trace("Fetching block file from Remote"); + indexInput = remoteDirectory.openInput(fileName, new BlockIOContext(IOContext.READ, start, length)); + logger.trace("Persisting the fetched blocked file from Remote"); + int indexInputLength = (int) indexInput.length(); + byte[] bytes = new byte[indexInputLength]; + indexInput.readBytes(bytes, 0, indexInputLength); + localFileOutputStream.write(bytes); + } catch (Exception err) { + logger.trace("Exception while fetching block from remote and persisting it on disk"); + throw new RuntimeException(err); + } + } catch (Exception e) { + logger.trace("Exception while fetching block file locally"); + throw new RuntimeException(e); + } + return indexInput; + } + + /** + * Implementation of the CachedIndexInput interface + */ + private class CachedIndexInputImpl implements CachedIndexInput { + + IndexInput indexInput; + AtomicBoolean isClosed; + + /** + * Constructor - takes IndexInput as parameter + */ + CachedIndexInputImpl(IndexInput indexInput) { + this.indexInput = indexInput; + isClosed = new AtomicBoolean(false); + } + + /** + * Returns the wrapped indexInput + */ + @Override + public IndexInput getIndexInput() throws IOException { + return indexInput; + } + + /** + * Returns the length of the wrapped indexInput + */ + @Override + public long length() { + return indexInput.length(); + } + + /** + * Checks if the wrapped indexInput is closed + */ + @Override + public boolean isClosed() { + return isClosed.get(); + } + + /** + * Closes the wrapped indexInput + */ + @Override + public void close() throws Exception { + indexInput.close(); + isClosed.set(true); + } } } diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/BlockIOContext.java b/server/src/main/java/org/opensearch/index/store/remote/utils/BlockIOContext.java new file mode 100644 index 0000000000000..da94e4a46d307 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/store/remote/utils/BlockIOContext.java @@ -0,0 +1,62 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.store.remote.utils; + +import org.apache.lucene.store.IOContext; + +/** + * BlockIOContext is an extension of IOContext which can be used to pass block related information to the openInput() method of any directory + */ +public class BlockIOContext extends IOContext { + + private final boolean isBlockRequest; + private long blockStart; + private long blockSize; + + /** + * Default constructor + */ + BlockIOContext(IOContext ctx) { + super(ctx.context); + this.isBlockRequest = false; + this.blockStart = -1; + this.blockSize = -1; + } + + /** + * Constructor to initialise BlockIOContext with block related information + */ + public BlockIOContext(IOContext ctx, long blockStart, long blockSize) { + super(ctx.context); + this.isBlockRequest = true; + this.blockStart = blockStart; + this.blockSize = blockSize; + } + + /** + * Function to check if the Context contains a block request or not + */ + public boolean isBlockRequest() { + return isBlockRequest; + } + + /** + * Getter for blockStart + */ + public long getBlockStart() { + return blockStart; + } + + /** + * Getter for blockSize + */ + public long getBlockSize() { + return blockSize; + } +} diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/FileType.java b/server/src/main/java/org/opensearch/index/store/remote/utils/FileType.java new file mode 100644 index 0000000000000..418f8a24a5f24 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/store/remote/utils/FileType.java @@ -0,0 +1,38 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.store.remote.utils; + +/** + * Enum to represent whether a file is block or not + */ +public enum FileType { + /** + * Block file + */ + BLOCK, + /** + * Non block file + */ + NON_BLOCK; + + /** + * Returns if the fileType is a block file or not + */ + public static boolean isBlockFile(FileType fileType) { + return fileType.equals(FileType.BLOCK); + } + + /** + * Returns if the fileName is block file or not + */ + public static boolean isBlockFile(String fileName) { + if (fileName.contains("_block_")) return true; + return false; + } +} diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileState.java b/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileState.java deleted file mode 100644 index 73b39bac35f35..0000000000000 --- a/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileState.java +++ /dev/null @@ -1,20 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.index.store.remote.utils.filetracker; - -public enum FileState { - /** - * DISK State means that currently the file is present only locally and has not yet been uploaded to the Remote Store - */ - DISK, - /** - * REMOTE State means that the file has been successfully uploaded to the Remote Store and is safe to be removed locally - */ - REMOTE; -} diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileTrackingInfo.java b/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileTrackingInfo.java deleted file mode 100644 index 60310b0fc39a2..0000000000000 --- a/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileTrackingInfo.java +++ /dev/null @@ -1,27 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.index.store.remote.utils.filetracker; - -public class FileTrackingInfo { - private final FileState fileState; - private final FileType fileType; - - public FileTrackingInfo(FileState fileState, FileType fileType) { - this.fileState = fileState; - this.fileType = fileType; - } - - public FileState getFileState() { - return fileState; - } - - public FileType getFileType() { - return fileType; - } -} diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileType.java b/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileType.java deleted file mode 100644 index 18506f7062b7c..0000000000000 --- a/server/src/main/java/org/opensearch/index/store/remote/utils/filetracker/FileType.java +++ /dev/null @@ -1,18 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.index.store.remote.utils.filetracker; - -public enum FileType { - BLOCK, - NON_BLOCK; - - public boolean isBlockFile(FileType fileType) { - return fileType.equals(FileType.BLOCK); - } -} diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index 251be8a990055..7fc758f4bfcd6 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/server/src/main/java/org/opensearch/indices/IndicesService.java @@ -136,6 +136,7 @@ import org.opensearch.index.shard.IndexingOperationListener; import org.opensearch.index.shard.IndexingStats; import org.opensearch.index.shard.IndexingStats.Stats.DocStatusStats; +import org.opensearch.index.store.remote.filecache.FileCache; import org.opensearch.index.translog.InternalTranslogFactory; import org.opensearch.index.translog.RemoteBlobStoreInternalTranslogFactory; import org.opensearch.index.translog.TranslogFactory; @@ -354,6 +355,7 @@ public class IndicesService extends AbstractLifecycleComponent private final BiFunction translogFactorySupplier; private volatile TimeValue clusterDefaultRefreshInterval; private final SearchRequestStats searchRequestStats; + private final FileCache fileCache; @Override protected void doStart() { @@ -388,7 +390,8 @@ public IndicesService( @Nullable RemoteStoreStatsTrackerFactory remoteStoreStatsTrackerFactory, RecoverySettings recoverySettings, CacheService cacheService, - RemoteStoreSettings remoteStoreSettings + RemoteStoreSettings remoteStoreSettings, + FileCache fileCache ) { this.settings = settings; this.threadPool = threadPool; @@ -440,6 +443,7 @@ public void onRemoval(ShardId shardId, String fieldName, boolean wasEvicted, lon this.directoryFactories = directoryFactories; this.recoveryStateFactories = recoveryStateFactories; + this.fileCache = fileCache; // doClose() is called when shutting down a node, yet there might still be ongoing requests // that we need to wait for before closing some resources such as the caches. In order to // avoid closing these resources while ongoing requests are still being processed, we use a @@ -873,7 +877,8 @@ private synchronized IndexService createIndexService( directoryFactories, () -> allowExpensiveQueries, indexNameExpressionResolver, - recoveryStateFactories + recoveryStateFactories, + fileCache ); for (IndexingOperationListener operationListener : indexingOperationListeners) { indexModule.addIndexOperationListener(operationListener); @@ -963,7 +968,8 @@ public synchronized MapperService createIndexMapperService(IndexMetadata indexMe directoryFactories, () -> allowExpensiveQueries, indexNameExpressionResolver, - recoveryStateFactories + recoveryStateFactories, + fileCache ); pluginsService.onIndexModule(indexModule); return indexModule.newIndexMapperService(xContentRegistry, mapperRegistry, scriptService); diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 85d7d63a96b03..11ba0ff34d1a8 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -840,7 +840,8 @@ protected Node( remoteStoreStatsTrackerFactory, recoverySettings, cacheService, - remoteStoreSettings + remoteStoreSettings, + fileCache ); final IngestService ingestService = new IngestService( diff --git a/server/src/test/java/org/opensearch/index/IndexModuleTests.java b/server/src/test/java/org/opensearch/index/IndexModuleTests.java index 4ce4936c047d9..6eddd8c4a9b4a 100644 --- a/server/src/test/java/org/opensearch/index/IndexModuleTests.java +++ b/server/src/test/java/org/opensearch/index/IndexModuleTests.java @@ -278,7 +278,8 @@ public void testWrapperIsBound() throws IOException { Collections.emptyMap(), () -> true, new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)), - Collections.emptyMap() + Collections.emptyMap(), + null ); module.setReaderWrapper(s -> new Wrapper()); @@ -304,7 +305,8 @@ public void testRegisterIndexStore() throws IOException { indexStoreFactories, () -> true, new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)), - Collections.emptyMap() + Collections.emptyMap(), + null ); final IndexService indexService = newIndexService(module); @@ -632,7 +634,8 @@ public void testRegisterCustomRecoveryStateFactory() throws IOException { Collections.emptyMap(), () -> true, new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)), - recoveryStateFactories + recoveryStateFactories, + null ); final IndexService indexService = newIndexService(module); @@ -664,7 +667,8 @@ private static IndexModule createIndexModule(IndexSettings indexSettings, Analys Collections.emptyMap(), () -> true, new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)), - Collections.emptyMap() + Collections.emptyMap(), + null ); } diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java index 95a343f3b4025..8bfd88d0a8e56 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java @@ -2079,7 +2079,8 @@ public void onFailure(final Exception e) { new RemoteStoreStatsTrackerFactory(clusterService, settings), DefaultRecoverySettings.INSTANCE, new CacheModule(new ArrayList<>(), settings).getCacheService(), - DefaultRemoteStoreSettings.INSTANCE + DefaultRemoteStoreSettings.INSTANCE, + null ); final RecoverySettings recoverySettings = new RecoverySettings(settings, clusterSettings); snapshotShardsService = new SnapshotShardsService( From d09b75a4d471452ef299be61e28fdc427410332c Mon Sep 17 00:00:00 2001 From: Shreyansh Ray Date: Tue, 30 Apr 2024 15:26:05 +0530 Subject: [PATCH 07/16] Modify TransferManager - replace BlobContainer with Functional Interface to fetch an InputStream instead Signed-off-by: Shreyansh Ray --- .../remotestore/CompositeDirectoryIT.java | 21 ++- .../index/store/CompositeDirectory.java | 103 +++++++++++---- .../index/store/RemoteDirectory.java | 2 + .../RemoteSnapshotDirectoryFactory.java | 2 +- .../OnDemandCompositeBlockIndexInput.java | 122 +++--------------- .../filecache/WrappedCachedIndexInput.java | 66 ++++++++++ .../store/remote/utils/TransferManager.java | 26 ++-- .../remote/utils/TransferManagerTests.java | 2 +- 8 files changed, 200 insertions(+), 144 deletions(-) create mode 100644 server/src/main/java/org/opensearch/index/store/remote/filecache/WrappedCachedIndexInput.java diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java index 35499c2f8cc5e..71255236a3809 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java @@ -8,6 +8,8 @@ package org.opensearch.remotestore; +import com.carrotsearch.randomizedtesting.annotations.ThreadLeakFilters; + import org.apache.lucene.store.Directory; import org.apache.lucene.store.FilterDirectory; import org.opensearch.action.admin.indices.get.GetIndexRequest; @@ -19,8 +21,11 @@ import org.opensearch.index.IndexService; import org.opensearch.index.shard.IndexShard; import org.opensearch.index.store.CompositeDirectory; +import org.opensearch.index.store.remote.file.CleanerDaemonThreadLeakFilter; import org.opensearch.indices.IndicesService; import org.opensearch.indices.replication.common.ReplicationType; +import org.opensearch.test.OpenSearchIntegTestCase; +import org.opensearch.test.junit.annotations.TestLogging; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED; @@ -28,11 +33,21 @@ import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REPLICATION_TYPE; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; +@ThreadLeakFilters(filters = CleanerDaemonThreadLeakFilter.class) +@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST) +//@TestLogging(reason = "Getting trace logs from composite directory package", value = "org.opensearch.index.store:TRACE") public class CompositeDirectoryIT extends RemoteStoreBaseIntegTestCase { + + @Override + protected Settings featureFlagSettings() { + Settings.Builder featureSettings = Settings.builder(); + featureSettings.put(FeatureFlags.WRITEABLE_REMOTE_INDEX, true); + + return featureSettings.build(); + } + public void testCompositeDirectory() throws Exception { Settings settings = Settings.builder() - .put(super.featureFlagSettings()) - .put(FeatureFlags.WRITEABLE_REMOTE_INDEX, "true") .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1) .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0) .put(IndexModule.INDEX_STORE_LOCALITY_SETTING.getKey(), "partial") @@ -60,7 +75,7 @@ public void testCompositeDirectory() throws Exception { assertEquals("true", indexSettings.get(SETTING_REMOTE_STORE_ENABLED)); assertEquals(REPOSITORY_NAME, indexSettings.get(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY)); assertEquals(REPOSITORY_2_NAME, indexSettings.get(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY)); - assertEquals("partial", indexSettings.get("index.store.locality")); + assertEquals("partial", indexSettings.get("index.store.data_locality")); ensureGreen("test-idx-1"); indexData(10, false, "test-idx-1"); diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java index e668ac31e57ee..26faf9d2039c0 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java @@ -17,13 +17,20 @@ import org.apache.lucene.store.IndexOutput; import org.apache.lucene.store.Lock; import org.apache.lucene.store.LockObtainFailedException; +import org.opensearch.common.lucene.store.FilterIndexOutput; +import org.opensearch.common.lucene.store.InputStreamIndexInput; import org.opensearch.index.store.remote.file.OnDemandCompositeBlockIndexInput; import org.opensearch.index.store.remote.filecache.FileCache; +import org.opensearch.index.store.remote.filecache.WrappedCachedIndexInput; +import org.opensearch.index.store.remote.utils.BlobFetchRequest; +import org.opensearch.index.store.remote.utils.BlockIOContext; import org.opensearch.index.store.remote.utils.FileType; +import org.opensearch.index.store.remote.utils.TransferManager; import java.io.FileNotFoundException; import java.io.IOException; import java.nio.file.NoSuchFileException; +import java.nio.file.Path; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -43,6 +50,7 @@ public class CompositeDirectory extends FilterDirectory { private final FSDirectory localDirectory; private final RemoteSegmentStoreDirectory remoteDirectory; private final FileCache fileCache; + private final TransferManager transferManager; private final ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock(); private final ReentrantReadWriteLock.WriteLock writeLock = readWriteLock.writeLock(); private final ReentrantReadWriteLock.ReadLock readLock = readWriteLock.readLock(); @@ -58,6 +66,9 @@ public CompositeDirectory(FSDirectory localDirectory, RemoteSegmentStoreDirector this.localDirectory = localDirectory; this.remoteDirectory = remoteDirectory; this.fileCache = fileCache; + transferManager = new TransferManager( + (name, position, length) -> new InputStreamIndexInput(remoteDirectory.openInput(name, new BlockIOContext(IOContext.DEFAULT, position, length)), length), + fileCache); } /** @@ -107,11 +118,7 @@ public void deleteFile(String name) throws IOException { fileCache.remove(localDirectory.getDirectory().resolve(name)); } catch (NoSuchFileException | FileNotFoundException e) { logger.trace("File {} not found in local, trying to delete from Remote", name); - try { - remoteDirectory.deleteFile(name); - } finally { - writeLock.unlock(); - } + remoteDirectory.deleteFile(name); } finally { writeLock.unlock(); } @@ -129,7 +136,7 @@ public long fileLength(String name) throws IOException { readLock.lock(); try { long fileLength; - if (Arrays.asList(getRemoteFiles()).contains(name) == false) { + if (isTempFile(name) || fileCache.get(localDirectory.getDirectory().resolve(name)) != null) { fileLength = localDirectory.fileLength(name); logger.trace("fileLength from Local {}", fileLength); } else { @@ -153,7 +160,10 @@ public IndexOutput createOutput(String name, IOContext context) throws IOExcepti logger.trace("createOutput() called {}", name); writeLock.lock(); try { - return localDirectory.createOutput(name, context); + /** + * The CacheableIndexOutput will ensure that the file is added to FileCache once write is completed on this file + */ + return new CacheableIndexOutput(localDirectory.createOutput(name, context), name); } finally { writeLock.unlock(); } @@ -224,6 +234,8 @@ public void rename(String source, String dest) throws IOException { writeLock.lock(); try { localDirectory.rename(source, dest); + fileCache.remove(localDirectory.getDirectory().resolve(source)); + cacheFile(dest); } finally { writeLock.unlock(); } @@ -240,14 +252,31 @@ public IndexInput openInput(String name, IOContext context) throws IOException { logger.trace("openInput() called {}", name); writeLock.lock(); try { - if (Arrays.asList(getRemoteFiles()).contains(name) == false) { - // If file has not yet been uploaded to Remote Store, fetch it from the local directory - logger.trace("File found in disk"); + /** + * We aren't tracking temporary files (created via createTempOutput) currently in FileCache as these are created and then deleted within a very short span of time + * We will be reading them directory from the local directory + */ + if (isTempFile(name)) { return localDirectory.openInput(name, context); - } else { - // If file has been uploaded to the Remote Store, fetch it from the Remote Store in blocks via - // OnDemandCompositeBlockIndexInput - logger.trace("File to be fetched from Remote"); + } + /** + * Return directly from the FileCache (via TransferManager) if complete file is present + */ + else if (fileCache.get(localDirectory.getDirectory().resolve(name)) != null) { + logger.trace("Complete file found in FileCache"); + BlobFetchRequest blobFetchRequest = BlobFetchRequest.builder() + .directory(localDirectory) + .fileName(name) + // position and length are not required here since this is a complete file, just adding dummy values for validation + .blobParts(new ArrayList<>(Arrays.asList(new BlobFetchRequest.BlobPart(name, 0, 1)))) + .build(); + return transferManager.fetchBlob(blobFetchRequest); + } + /** + * If file has been uploaded to the Remote Store, fetch it from the Remote Store in blocks via OnDemandCompositeBlockIndexInput + */ + else { + logger.trace("Complete file not in FileCache, to be fetched in Blocks from Remote"); return new OnDemandCompositeBlockIndexInput(remoteDirectory, name, localDirectory, fileCache, context); } } finally { @@ -293,17 +322,17 @@ public void close() throws IOException { */ @Override public Set getPendingDeletions() throws IOException { - readLock.lock(); + writeLock.lock(); try { return localDirectory.getPendingDeletions(); } finally { - readLock.unlock(); + writeLock.unlock(); } } /** * Function to perform operations once files have been uploaded to Remote Store - * Currently deleting the local files here, as once uploaded to Remote, local files are safe to delete + * Currently deleting the local files here, as once uploaded to Remote, local files become eligible for eviction from FileCache * @param files : recent files which have been successfully uploaded to Remote Store * @throws IOException in case of I/O error */ @@ -316,25 +345,34 @@ public void afterSyncToRemote(Collection files) throws IOException { for (String fileName : files) { writeLock.lock(); try { - localDirectory.deleteFile(fileName); + /** + * TODO - Unpin the files here from FileCache so that they become eligible for eviction, once pinning/unpinning support is added in FileCache + * Uncomment the below commented line(to remove the file from cache once uploaded) to test block based functionality + */ + logger.trace("File {} uploaded to Remote Store and now can be eligible for eviction in FileCache", fileName); + // fileCache.remove(localDirectory.getDirectory().resolve(fileName)); } finally { writeLock.unlock(); } } } + private boolean isTempFile(String name) { + return name.endsWith(".tmp"); + } + /** * Return the list of files present in Remote */ - private String[] getRemoteFiles() { + private String[] getRemoteFiles() throws IOException { String[] remoteFiles; try { remoteFiles = remoteDirectory.listAll(); - } catch (Exception e) { + } catch (NullPointerException e) { /** - * There are two scenarios where the listAll() call on remote directory fails: + * There are two scenarios where the listAll() call on remote directory returns NullPointerException: * - When remote directory is not set - * - When init() of remote directory has not yet been called (which results in NullPointerException while calling listAll() for RemoteSegmentStoreDirectory) + * - When init() of remote directory has not yet been called * * Returning an empty list in these scenarios */ @@ -342,4 +380,25 @@ private String[] getRemoteFiles() { } return remoteFiles; } + + private void cacheFile(String name) throws IOException { + Path filePath = localDirectory.getDirectory().resolve(name); + fileCache.put(filePath, new WrappedCachedIndexInput(localDirectory.openInput(name, IOContext.READ))); + } + + private class CacheableIndexOutput extends FilterIndexOutput { + + String fileName; + + public CacheableIndexOutput(IndexOutput out, String fileName) { + super("CacheableIndexOutput for file : " + fileName, out); + this.fileName = fileName; + } + + @Override + public void close() throws IOException { + super.close(); + cacheFile(fileName); + } + } } diff --git a/server/src/main/java/org/opensearch/index/store/RemoteDirectory.java b/server/src/main/java/org/opensearch/index/store/RemoteDirectory.java index e6d5af04e0cb7..9ed64501600c1 100644 --- a/server/src/main/java/org/opensearch/index/store/RemoteDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/RemoteDirectory.java @@ -204,7 +204,9 @@ public IndexInput openInput(String name, long fileLength, IOContext context) thr long position = ((BlockIOContext) context).getBlockStart(); long length = ((BlockIOContext) context).getBlockSize(); inputStream = blobContainer.readBlob(name, position, length); + // TODO - Explore how we can buffer small chunks of data instead of having the whole 8MB block in memory byte[] bytes = downloadRateLimiter.apply(inputStream).readAllBytes(); + inputStream.close(); return new ByteArrayIndexInput(name, bytes); } else { inputStream = blobContainer.readBlob(name); diff --git a/server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java b/server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java index 7cfa738e75e52..177f0526e7571 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java +++ b/server/src/main/java/org/opensearch/index/store/remote/directory/RemoteSnapshotDirectoryFactory.java @@ -94,7 +94,7 @@ private Future createRemoteSnapshotDirectoryFromSnapsho assert indexShardSnapshot instanceof BlobStoreIndexShardSnapshot : "indexShardSnapshot should be an instance of BlobStoreIndexShardSnapshot"; final BlobStoreIndexShardSnapshot snapshot = (BlobStoreIndexShardSnapshot) indexShardSnapshot; - TransferManager transferManager = new TransferManager(blobContainer, remoteStoreFileCache); + TransferManager transferManager = new TransferManager(blobContainer::readBlob, remoteStoreFileCache); return new RemoteSnapshotDirectory(snapshot, localStoreDir, transferManager); }); } diff --git a/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java b/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java index b2af3e7305c5d..f024592330512 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java +++ b/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java @@ -13,19 +13,16 @@ import org.apache.lucene.store.FSDirectory; import org.apache.lucene.store.IOContext; import org.apache.lucene.store.IndexInput; +import org.opensearch.common.lucene.store.InputStreamIndexInput; import org.opensearch.index.store.RemoteSegmentStoreDirectory; -import org.opensearch.index.store.remote.filecache.CachedIndexInput; import org.opensearch.index.store.remote.filecache.FileCache; +import org.opensearch.index.store.remote.utils.BlobFetchRequest; import org.opensearch.index.store.remote.utils.BlockIOContext; +import org.opensearch.index.store.remote.utils.TransferManager; -import java.io.BufferedOutputStream; -import java.io.FileNotFoundException; import java.io.IOException; -import java.io.OutputStream; -import java.nio.file.Files; -import java.nio.file.NoSuchFileException; -import java.nio.file.Path; -import java.util.concurrent.atomic.AtomicBoolean; +import java.util.ArrayList; +import java.util.List; /** * OnDemandCompositeBlockIndexInput is used by the Composite Directory to read data in blocks from Remote and cache those blocks in FileCache @@ -39,6 +36,7 @@ public class OnDemandCompositeBlockIndexInput extends OnDemandBlockIndexInput { private final FSDirectory localDirectory; private final IOContext context; private final FileCache fileCache; + private final TransferManager transferManager; public OnDemandCompositeBlockIndexInput( RemoteSegmentStoreDirectory remoteDirectory, @@ -75,6 +73,9 @@ public OnDemandCompositeBlockIndexInput( this.fileName = fileName; this.fileCache = fileCache; this.context = context; + this.transferManager = new TransferManager( + (name, position, length) -> new InputStreamIndexInput(remoteDirectory.openInput(name, new BlockIOContext(context, position, length)), length), + fileCache); originalFileSize = remoteDirectory.fileLength(fileName); } @@ -113,20 +114,12 @@ protected IndexInput fetchBlock(int blockId) throws IOException { length, originalFileSize ); - Path blockFilePath = getLocalFilePath(blockFileName); - final CachedIndexInput cacheEntry = fileCache.compute(blockFilePath, (path, cachedIndexInput) -> { - if (cachedIndexInput == null || cachedIndexInput.isClosed()) { - // Doesn't exist or is closed, either way create a new one - IndexInput indexInput = fetchIndexInput(blockFileName, blockStart, length); - return new CachedIndexInputImpl(indexInput); - } else { - logger.trace("Block already present in cache"); - // already in the cache and ready to be used (open) - return cachedIndexInput; - } - }); - - return cacheEntry.getIndexInput(); + BlobFetchRequest blobFetchRequest = BlobFetchRequest.builder() + .directory(localDirectory) + .fileName(blockFileName) + .blobParts(new ArrayList<>(List.of(new BlobFetchRequest.BlobPart(fileName, blockStart, length)))) + .build(); + return transferManager.fetchBlob(blobFetchRequest); } @Override @@ -141,89 +134,4 @@ private long getActualBlockSize(int blockId) { return (blockId != getBlock(originalFileSize - 1)) ? blockSize : getBlockOffset(originalFileSize - 1) + 1; } - private Path getLocalFilePath(String file) { - return localDirectory.getDirectory().resolve(file); - } - - private IndexInput fetchIndexInput(String blockFileName, long start, long length) { - IndexInput indexInput; - Path filePath = getLocalFilePath(blockFileName); - try { - // Fetch from local if block file is present locally in disk - indexInput = localDirectory.openInput(blockFileName, IOContext.READ); - logger.trace("Block file present locally, just putting it in cache"); - } catch (FileNotFoundException | NoSuchFileException e) { - logger.trace("Block file not present locally, fetching from Remote"); - // If block file is not present locally in disk, fetch from remote and persist the block file in disk - try ( - OutputStream fileOutputStream = Files.newOutputStream(filePath); - OutputStream localFileOutputStream = new BufferedOutputStream(fileOutputStream) - ) { - logger.trace("Fetching block file from Remote"); - indexInput = remoteDirectory.openInput(fileName, new BlockIOContext(IOContext.READ, start, length)); - logger.trace("Persisting the fetched blocked file from Remote"); - int indexInputLength = (int) indexInput.length(); - byte[] bytes = new byte[indexInputLength]; - indexInput.readBytes(bytes, 0, indexInputLength); - localFileOutputStream.write(bytes); - } catch (Exception err) { - logger.trace("Exception while fetching block from remote and persisting it on disk"); - throw new RuntimeException(err); - } - } catch (Exception e) { - logger.trace("Exception while fetching block file locally"); - throw new RuntimeException(e); - } - return indexInput; - } - - /** - * Implementation of the CachedIndexInput interface - */ - private class CachedIndexInputImpl implements CachedIndexInput { - - IndexInput indexInput; - AtomicBoolean isClosed; - - /** - * Constructor - takes IndexInput as parameter - */ - CachedIndexInputImpl(IndexInput indexInput) { - this.indexInput = indexInput; - isClosed = new AtomicBoolean(false); - } - - /** - * Returns the wrapped indexInput - */ - @Override - public IndexInput getIndexInput() throws IOException { - return indexInput; - } - - /** - * Returns the length of the wrapped indexInput - */ - @Override - public long length() { - return indexInput.length(); - } - - /** - * Checks if the wrapped indexInput is closed - */ - @Override - public boolean isClosed() { - return isClosed.get(); - } - - /** - * Closes the wrapped indexInput - */ - @Override - public void close() throws Exception { - indexInput.close(); - isClosed.set(true); - } - } } diff --git a/server/src/main/java/org/opensearch/index/store/remote/filecache/WrappedCachedIndexInput.java b/server/src/main/java/org/opensearch/index/store/remote/filecache/WrappedCachedIndexInput.java new file mode 100644 index 0000000000000..67b12a10810ed --- /dev/null +++ b/server/src/main/java/org/opensearch/index/store/remote/filecache/WrappedCachedIndexInput.java @@ -0,0 +1,66 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.store.remote.filecache; + +import org.apache.lucene.store.IndexInput; + +import java.io.IOException; +import java.util.concurrent.atomic.AtomicBoolean; + +/** + * Wrapper implementation of the CachedIndexInput which takes in an IndexInput as parameter + */ +public class WrappedCachedIndexInput implements CachedIndexInput { + + IndexInput indexInput; + AtomicBoolean isClosed; + + /** + * Constructor - takes IndexInput as parameter + */ + public WrappedCachedIndexInput(IndexInput indexInput) { + this.indexInput = indexInput; + isClosed = new AtomicBoolean(false); + } + + /** + * Returns the wrapped indexInput + */ + @Override + public IndexInput getIndexInput() throws IOException { + return indexInput; + } + + /** + * Returns the length of the wrapped indexInput + */ + @Override + public long length() { + return indexInput.length(); + } + + /** + * Checks if the wrapped indexInput is closed + */ + @Override + public boolean isClosed() { + return isClosed.get(); + } + + /** + * Closes the wrapped indexInput + */ + @Override + public void close() throws Exception { + if (!isClosed()) { + indexInput.close(); + isClosed.set(true); + } + } +} diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java b/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java index 98cad7bfadb09..557a71abd2b3e 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java +++ b/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java @@ -39,11 +39,17 @@ public class TransferManager { private static final Logger logger = LogManager.getLogger(TransferManager.class); - private final BlobContainer blobContainer; + @FunctionalInterface + public interface BlobStreamReader { + InputStream read(String name, long position, long length) throws IOException; + } + + private final BlobStreamReader blobStreamReader; private final FileCache fileCache; - public TransferManager(final BlobContainer blobContainer, final FileCache fileCache) { - this.blobContainer = blobContainer; + + public TransferManager(final BlobStreamReader blobStreamReader, final FileCache fileCache) { + this.blobStreamReader = blobStreamReader; this.fileCache = fileCache; } @@ -59,7 +65,7 @@ public IndexInput fetchBlob(BlobFetchRequest blobFetchRequest) throws IOExceptio final CachedIndexInput cacheEntry = fileCache.compute(key, (path, cachedIndexInput) -> { if (cachedIndexInput == null || cachedIndexInput.isClosed()) { // Doesn't exist or is closed, either way create a new one - return new DelayedCreationCachedIndexInput(fileCache, blobContainer, blobFetchRequest); + return new DelayedCreationCachedIndexInput(fileCache, blobStreamReader, blobFetchRequest); } else { // already in the cache and ready to be used (open) return cachedIndexInput; @@ -77,7 +83,7 @@ public IndexInput fetchBlob(BlobFetchRequest blobFetchRequest) throws IOExceptio } @SuppressWarnings("removal") - private static FileCachedIndexInput createIndexInput(FileCache fileCache, BlobContainer blobContainer, BlobFetchRequest request) { + private static FileCachedIndexInput createIndexInput(FileCache fileCache, BlobStreamReader blobStreamReader, BlobFetchRequest request) { // We need to do a privileged action here in order to fetch from remote // and write to the local file cache in case this is invoked as a side // effect of a plugin (such as a scripted search) that doesn't have the @@ -91,7 +97,7 @@ private static FileCachedIndexInput createIndexInput(FileCache fileCache, BlobCo ) { for (BlobFetchRequest.BlobPart blobPart : request.blobParts()) { try ( - InputStream snapshotFileInputStream = blobContainer.readBlob( + InputStream snapshotFileInputStream = blobStreamReader.read( blobPart.getBlobName(), blobPart.getPosition(), blobPart.getLength() @@ -119,15 +125,15 @@ private static FileCachedIndexInput createIndexInput(FileCache fileCache, BlobCo */ private static class DelayedCreationCachedIndexInput implements CachedIndexInput { private final FileCache fileCache; - private final BlobContainer blobContainer; + private final BlobStreamReader blobStreamReader; private final BlobFetchRequest request; private final CompletableFuture result = new CompletableFuture<>(); private final AtomicBoolean isStarted = new AtomicBoolean(false); private final AtomicBoolean isClosed = new AtomicBoolean(false); - private DelayedCreationCachedIndexInput(FileCache fileCache, BlobContainer blobContainer, BlobFetchRequest request) { + private DelayedCreationCachedIndexInput(FileCache fileCache, BlobStreamReader blobStreamReader, BlobFetchRequest request) { this.fileCache = fileCache; - this.blobContainer = blobContainer; + this.blobStreamReader = blobStreamReader; this.request = request; } @@ -139,7 +145,7 @@ public IndexInput getIndexInput() throws IOException { if (isStarted.getAndSet(true) == false) { // We're the first one here, need to download the block try { - result.complete(createIndexInput(fileCache, blobContainer, request)); + result.complete(createIndexInput(fileCache, blobStreamReader, request)); } catch (Exception e) { result.completeExceptionally(e); fileCache.remove(request.getFilePath()); diff --git a/server/src/test/java/org/opensearch/index/store/remote/utils/TransferManagerTests.java b/server/src/test/java/org/opensearch/index/store/remote/utils/TransferManagerTests.java index 7ae3944eb6944..c0a5ea749b765 100644 --- a/server/src/test/java/org/opensearch/index/store/remote/utils/TransferManagerTests.java +++ b/server/src/test/java/org/opensearch/index/store/remote/utils/TransferManagerTests.java @@ -60,7 +60,7 @@ public void setUp() throws Exception { directory = new MMapDirectory(createTempDir(), SimpleFSLockFactory.INSTANCE); blobContainer = mock(BlobContainer.class); doAnswer(i -> new ByteArrayInputStream(createData())).when(blobContainer).readBlob(eq("blob"), anyLong(), anyLong()); - transferManager = new TransferManager(blobContainer, fileCache); + transferManager = new TransferManager(blobContainer::readBlob, fileCache); } @After From 90ace4a4863d5c8d070edee93158a10e57f5060a Mon Sep 17 00:00:00 2001 From: Shreyansh Ray Date: Thu, 2 May 2024 22:41:48 +0530 Subject: [PATCH 08/16] Reuse OnDemandBlockSnapshotIndexInput instead of OnDemandBlockCompositeIndexInput Signed-off-by: Shreyansh Ray --- .../index/store/CompositeDirectory.java | 47 +++++++++++++++++-- .../file/OnDemandBlockSnapshotIndexInput.java | 8 +++- .../store/remote/utils/TransferManager.java | 4 ++ .../OnDemandBlockSnapshotIndexInputTests.java | 4 +- 4 files changed, 56 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java index 26faf9d2039c0..740aee3092e57 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java @@ -17,11 +17,15 @@ import org.apache.lucene.store.IndexOutput; import org.apache.lucene.store.Lock; import org.apache.lucene.store.LockObtainFailedException; +import org.apache.lucene.util.Version; import org.opensearch.common.lucene.store.FilterIndexOutput; import org.opensearch.common.lucene.store.InputStreamIndexInput; +import org.opensearch.index.snapshots.blobstore.BlobStoreIndexShardSnapshot; +import org.opensearch.index.store.remote.file.OnDemandBlockSnapshotIndexInput; import org.opensearch.index.store.remote.file.OnDemandCompositeBlockIndexInput; import org.opensearch.index.store.remote.filecache.FileCache; import org.opensearch.index.store.remote.filecache.WrappedCachedIndexInput; +import org.opensearch.index.store.remote.metadata.RemoteSegmentMetadata; import org.opensearch.index.store.remote.utils.BlobFetchRequest; import org.opensearch.index.store.remote.utils.BlockIOContext; import org.opensearch.index.store.remote.utils.FileType; @@ -29,6 +33,7 @@ import java.io.FileNotFoundException; import java.io.IOException; +import java.io.InputStream; import java.nio.file.NoSuchFileException; import java.nio.file.Path; import java.util.ArrayList; @@ -54,6 +59,7 @@ public class CompositeDirectory extends FilterDirectory { private final ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock(); private final ReentrantReadWriteLock.WriteLock writeLock = readWriteLock.writeLock(); private final ReentrantReadWriteLock.ReadLock readLock = readWriteLock.readLock(); + private final RemoteDirectoryBlobStreamReader remoteDirectoryBlobStreamReader; /** * Constructor to initialise the composite directory @@ -66,8 +72,9 @@ public CompositeDirectory(FSDirectory localDirectory, RemoteSegmentStoreDirector this.localDirectory = localDirectory; this.remoteDirectory = remoteDirectory; this.fileCache = fileCache; + remoteDirectoryBlobStreamReader = new RemoteDirectoryBlobStreamReader(IOContext.DEFAULT, remoteDirectory); transferManager = new TransferManager( - (name, position, length) -> new InputStreamIndexInput(remoteDirectory.openInput(name, new BlockIOContext(IOContext.DEFAULT, position, length)), length), + remoteDirectoryBlobStreamReader, fileCache); } @@ -252,6 +259,7 @@ public IndexInput openInput(String name, IOContext context) throws IOException { logger.trace("openInput() called {}", name); writeLock.lock(); try { + remoteDirectoryBlobStreamReader.setContext(context); /** * We aren't tracking temporary files (created via createTempOutput) currently in FileCache as these are created and then deleted within a very short span of time * We will be reading them directory from the local directory @@ -277,7 +285,19 @@ else if (fileCache.get(localDirectory.getDirectory().resolve(name)) != null) { */ else { logger.trace("Complete file not in FileCache, to be fetched in Blocks from Remote"); - return new OnDemandCompositeBlockIndexInput(remoteDirectory, name, localDirectory, fileCache, context); + RemoteSegmentMetadata remoteSegmentMetadata = remoteDirectory.readLatestMetadataFile(); + RemoteSegmentStoreDirectory.UploadedSegmentMetadata uploadedSegmentMetadata = remoteSegmentMetadata.getMetadata().get(name); + /** + * TODO : Refactor FileInfo and OnDemandBlockSnapshotIndexInput to more generic names as they are not Remote Snapshot specific + */ + BlobStoreIndexShardSnapshot.FileInfo fileInfo = new BlobStoreIndexShardSnapshot.FileInfo( + name, + new StoreFileMetadata(name, uploadedSegmentMetadata.getLength(), + uploadedSegmentMetadata.getChecksum(), Version.LATEST), + null + ); + return new OnDemandBlockSnapshotIndexInput(fileInfo, localDirectory, transferManager); + //return new OnDemandCompositeBlockIndexInput(remoteDirectory, name, localDirectory, fileCache, context); } } finally { writeLock.unlock(); @@ -350,7 +370,7 @@ public void afterSyncToRemote(Collection files) throws IOException { * Uncomment the below commented line(to remove the file from cache once uploaded) to test block based functionality */ logger.trace("File {} uploaded to Remote Store and now can be eligible for eviction in FileCache", fileName); - // fileCache.remove(localDirectory.getDirectory().resolve(fileName)); + fileCache.remove(localDirectory.getDirectory().resolve(fileName)); } finally { writeLock.unlock(); } @@ -369,7 +389,7 @@ private String[] getRemoteFiles() throws IOException { try { remoteFiles = remoteDirectory.listAll(); } catch (NullPointerException e) { - /** + /* * There are two scenarios where the listAll() call on remote directory returns NullPointerException: * - When remote directory is not set * - When init() of remote directory has not yet been called @@ -401,4 +421,23 @@ public void close() throws IOException { cacheFile(fileName); } } + + private class RemoteDirectoryBlobStreamReader implements TransferManager.BlobStreamReader { + private IOContext context; + private final RemoteSegmentStoreDirectory remoteDirectory; + + RemoteDirectoryBlobStreamReader(IOContext context, RemoteSegmentStoreDirectory remoteDirectory) { + this.context = context; + this.remoteDirectory = remoteDirectory; + } + + void setContext(IOContext context) { + this.context = context; + } + + @Override + public InputStream read(String name, long position, long length) throws IOException { + return new InputStreamIndexInput(remoteDirectory.openInput(name, new BlockIOContext(context, position, length)), length); + } + } } diff --git a/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandBlockSnapshotIndexInput.java b/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandBlockSnapshotIndexInput.java index 8097fd08da50a..1f70fd132ce17 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandBlockSnapshotIndexInput.java +++ b/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandBlockSnapshotIndexInput.java @@ -8,9 +8,12 @@ package org.opensearch.index.store.remote.file; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.apache.lucene.store.FSDirectory; import org.apache.lucene.store.IndexInput; import org.opensearch.index.snapshots.blobstore.BlobStoreIndexShardSnapshot.FileInfo; +import org.opensearch.index.store.CompositeDirectory; import org.opensearch.index.store.remote.utils.BlobFetchRequest; import org.opensearch.index.store.remote.utils.TransferManager; @@ -26,6 +29,7 @@ * @opensearch.internal */ public class OnDemandBlockSnapshotIndexInput extends OnDemandBlockIndexInput { + private static final Logger logger = LogManager.getLogger(OnDemandBlockSnapshotIndexInput.class); /** * Where this class fetches IndexInput parts from */ @@ -133,10 +137,12 @@ protected OnDemandBlockSnapshotIndexInput buildSlice(String sliceDescription, lo @Override protected IndexInput fetchBlock(int blockId) throws IOException { - final String blockFileName = fileName + "." + blockId; + logger.trace("fetchBlock called with blockId -> {}", blockId); + final String blockFileName = fileName + "_block_" + blockId; final long blockStart = getBlockStart(blockId); final long blockEnd = blockStart + getActualBlockSize(blockId); + logger.trace("File: {} , Block File: {} , BlockStart: {} , BlockEnd: {} , OriginalFileSize: {}", fileName, blockFileName, blockStart, blockEnd, originalFileSize); // Block may be present on multiple chunks of a file, so we need // to fetch each chunk/blob part separately to fetch an entire block. diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java b/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java index 557a71abd2b3e..f123a923f5ef6 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java +++ b/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java @@ -61,12 +61,15 @@ public TransferManager(final BlobStreamReader blobStreamReader, final FileCache public IndexInput fetchBlob(BlobFetchRequest blobFetchRequest) throws IOException { final Path key = blobFetchRequest.getFilePath(); + logger.trace("fetchBlob called for {}", key.toString()); final CachedIndexInput cacheEntry = fileCache.compute(key, (path, cachedIndexInput) -> { if (cachedIndexInput == null || cachedIndexInput.isClosed()) { + logger.trace("Transfer Manager - IndexInput closed or not in cache"); // Doesn't exist or is closed, either way create a new one return new DelayedCreationCachedIndexInput(fileCache, blobStreamReader, blobFetchRequest); } else { + logger.trace("Transfer Manager - Already in cache"); // already in the cache and ready to be used (open) return cachedIndexInput; } @@ -91,6 +94,7 @@ private static FileCachedIndexInput createIndexInput(FileCache fileCache, BlobSt return AccessController.doPrivileged((PrivilegedAction) () -> { try { if (Files.exists(request.getFilePath()) == false) { + logger.trace("Fetching from Remote in createIndexInput of Transfer Manager"); try ( OutputStream fileOutputStream = Files.newOutputStream(request.getFilePath()); OutputStream localFileOutputStream = new BufferedOutputStream(fileOutputStream) diff --git a/server/src/test/java/org/opensearch/index/store/remote/file/OnDemandBlockSnapshotIndexInputTests.java b/server/src/test/java/org/opensearch/index/store/remote/file/OnDemandBlockSnapshotIndexInputTests.java index a135802c5f49c..c7d0cc0c5b96e 100644 --- a/server/src/test/java/org/opensearch/index/store/remote/file/OnDemandBlockSnapshotIndexInputTests.java +++ b/server/src/test/java/org/opensearch/index/store/remote/file/OnDemandBlockSnapshotIndexInputTests.java @@ -207,7 +207,7 @@ private void initBlockFiles(int blockSize, FSDirectory fsDirectory) { // write 48, -80 alternatively for (int i = 0; i < numOfBlocks; i++) { // create normal blocks - String blockName = BLOCK_FILE_PREFIX + "." + i; + String blockName = BLOCK_FILE_PREFIX + "_block_" + i; IndexOutput output = fsDirectory.createOutput(blockName, null); // since block size is always even number, safe to do division for (int j = 0; j < blockSize / 2; j++) { @@ -221,7 +221,7 @@ private void initBlockFiles(int blockSize, FSDirectory fsDirectory) { if (numOfBlocks > 1 && sizeOfLastBlock != 0) { // create last block - String lastBlockName = BLOCK_FILE_PREFIX + "." + numOfBlocks; + String lastBlockName = BLOCK_FILE_PREFIX + "_block_" + numOfBlocks; IndexOutput output = fsDirectory.createOutput(lastBlockName, null); for (int i = 0; i < sizeOfLastBlock; i++) { if ((i & 1) == 0) { From 93d1f2a76a0a3addcd58d1f99411d1d4bb215891 Mon Sep 17 00:00:00 2001 From: Shreyansh Ray Date: Wed, 8 May 2024 23:34:36 +0530 Subject: [PATCH 09/16] Modify constructors to avoid breaking public api contract and code review fixes Signed-off-by: Shreyansh Ray --- .../remotestore/CompositeDirectoryIT.java | 58 +++--- .../metadata/MetadataCreateIndexService.java | 4 +- .../org/opensearch/index/IndexModule.java | 23 +++ .../org/opensearch/index/IndexService.java | 78 +++++++- .../shard/RemoteStoreRefreshListener.java | 16 +- .../store/CloseableFilterIndexOutput.java | 45 +++++ .../index/store/CompositeDirectory.java | 170 ++++++---------- .../file/OnDemandBlockSnapshotIndexInput.java | 10 +- .../OnDemandCompositeBlockIndexInput.java | 137 ------------- ...put.java => NonBlockCachedIndexInput.java} | 12 +- .../store/remote/utils/TransferManager.java | 27 +-- .../opensearch/indices/IndicesService.java | 63 +++++- .../opensearch/index/IndexModuleTests.java | 12 +- .../BaseRemoteSegmentStoreDirectoryTests.java | 178 +++++++++++++++++ .../index/store/CompositeDirectoryTests.java | 189 ++++++++++++++++++ .../RemoteSegmentStoreDirectoryTests.java | 153 +------------- .../snapshots/SnapshotResiliencyTests.java | 3 +- .../test/OpenSearchIntegTestCase.java | 6 +- 18 files changed, 723 insertions(+), 461 deletions(-) create mode 100644 server/src/main/java/org/opensearch/index/store/CloseableFilterIndexOutput.java delete mode 100644 server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java rename server/src/main/java/org/opensearch/index/store/remote/filecache/{WrappedCachedIndexInput.java => NonBlockCachedIndexInput.java} (77%) create mode 100644 server/src/test/java/org/opensearch/index/store/BaseRemoteSegmentStoreDirectoryTests.java create mode 100644 server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java index 71255236a3809..a34b665561289 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java @@ -14,30 +14,40 @@ import org.apache.lucene.store.FilterDirectory; import org.opensearch.action.admin.indices.get.GetIndexRequest; import org.opensearch.action.admin.indices.get.GetIndexResponse; +import org.opensearch.action.search.SearchResponse; import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.common.settings.Settings; import org.opensearch.common.util.FeatureFlags; import org.opensearch.index.IndexModule; import org.opensearch.index.IndexService; +import org.opensearch.index.query.QueryBuilders; import org.opensearch.index.shard.IndexShard; import org.opensearch.index.store.CompositeDirectory; import org.opensearch.index.store.remote.file.CleanerDaemonThreadLeakFilter; import org.opensearch.indices.IndicesService; -import org.opensearch.indices.replication.common.ReplicationType; import org.opensearch.test.OpenSearchIntegTestCase; import org.opensearch.test.junit.annotations.TestLogging; -import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY; -import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED; -import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY; -import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REPLICATION_TYPE; +import java.util.Map; + import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; +import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertHitCount; @ThreadLeakFilters(filters = CleanerDaemonThreadLeakFilter.class) -@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST) -//@TestLogging(reason = "Getting trace logs from composite directory package", value = "org.opensearch.index.store:TRACE") +@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 1, numClientNodes = 0, supportsDedicatedMasters = false) +// Uncomment the below line to enable trace level logs for this test for better debugging +@TestLogging(reason = "Getting trace logs from composite directory package", value = "org.opensearch.index.store:TRACE") public class CompositeDirectoryIT extends RemoteStoreBaseIntegTestCase { + /* + Disabling MockFSIndexStore plugin as the MockFSDirectoryFactory wraps the FSDirectory over a OpenSearchMockDirectoryWrapper which extends FilterDirectory (whereas FSDirectory extends BaseDirectory) + As a result of this wrapping the local directory of Composite Directory does not satisfy the assertion that local directory must be of type FSDirectory + */ + @Override + protected boolean addMockIndexStorePlugin() { + return false; + } + @Override protected Settings featureFlagSettings() { Settings.Builder featureSettings = Settings.builder(); @@ -53,32 +63,28 @@ public void testCompositeDirectory() throws Exception { .put(IndexModule.INDEX_STORE_LOCALITY_SETTING.getKey(), "partial") .build(); assertAcked(client().admin().indices().prepareCreate("test-idx-1").setSettings(settings).get()); + + // Check if the Directory initialized for the IndexShard is of Composite Directory type + IndexService indexService = internalCluster().getDataNodeInstance(IndicesService.class).indexService(resolveIndex("test-idx-1")); + IndexShard shard = indexService.getShardOrNull(0); + Directory directory = (((FilterDirectory) (((FilterDirectory) (shard.store().directory())).getDelegate())).getDelegate()); + assertTrue(directory instanceof CompositeDirectory); + + // Verify from the cluster settings if the data locality is partial GetIndexResponse getIndexResponse = client().admin() .indices() .getIndex(new GetIndexRequest().indices("test-idx-1").includeDefaults(true)) .get(); - boolean indexServiceFound = false; - String[] nodes = internalCluster().getNodeNames(); - for (String node : nodes) { - IndexService indexService = internalCluster().getInstance(IndicesService.class, node).indexService(resolveIndex("test-idx-1")); - if (indexService == null) { - continue; - } - IndexShard shard = indexService.getShardOrNull(0); - Directory directory = (((FilterDirectory) (((FilterDirectory) (shard.store().directory())).getDelegate())).getDelegate()); - assertTrue(directory instanceof CompositeDirectory); - indexServiceFound = true; - } - assertTrue(indexServiceFound); Settings indexSettings = getIndexResponse.settings().get("test-idx-1"); - assertEquals(ReplicationType.SEGMENT.toString(), indexSettings.get(SETTING_REPLICATION_TYPE)); - assertEquals("true", indexSettings.get(SETTING_REMOTE_STORE_ENABLED)); - assertEquals(REPOSITORY_NAME, indexSettings.get(SETTING_REMOTE_SEGMENT_STORE_REPOSITORY)); - assertEquals(REPOSITORY_2_NAME, indexSettings.get(SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY)); assertEquals("partial", indexSettings.get("index.store.data_locality")); + // Index data and ensure cluster does not turn red while indexing + Map stats = indexData(10, false, "test-idx-1"); + refresh("test-idx-1"); ensureGreen("test-idx-1"); - indexData(10, false, "test-idx-1"); - ensureGreen("test-idx-1"); + + // Search and verify that the total docs indexed match the search hits + SearchResponse searchResponse3 = client().prepareSearch("test-idx-1").setQuery(QueryBuilders.matchAllQuery()).get(); + assertHitCount(searchResponse3, stats.get(TOTAL_OPERATIONS)); } } diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java index db6af7fffaeac..c8bbad2f0073b 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java @@ -1687,7 +1687,9 @@ public static void validateIndexStoreLocality(Settings indexSettings) { .equalsIgnoreCase(IndexModule.DataLocalityType.PARTIAL.toString()) && !FeatureFlags.isEnabled(FeatureFlags.WRITEABLE_REMOTE_INDEX_SETTING)) { throw new IllegalArgumentException( - "index.store.locality can be set to PARTIAL only if Feature Flag for Writable Remote Index is true" + "index.store.locality can be set to PARTIAL only if Feature Flag [" + + FeatureFlags.WRITEABLE_REMOTE_INDEX_SETTING.getKey() + + "] is set to true" ); } } diff --git a/server/src/main/java/org/opensearch/index/IndexModule.java b/server/src/main/java/org/opensearch/index/IndexModule.java index b4fd7666ba45a..d79f19f17e167 100644 --- a/server/src/main/java/org/opensearch/index/IndexModule.java +++ b/server/src/main/java/org/opensearch/index/IndexModule.java @@ -345,6 +345,29 @@ public IndexModule( this.fileCache = fileCache; } + public IndexModule( + final IndexSettings indexSettings, + final AnalysisRegistry analysisRegistry, + final EngineFactory engineFactory, + final EngineConfigFactory engineConfigFactory, + final Map directoryFactories, + final BooleanSupplier allowExpensiveQueries, + final IndexNameExpressionResolver expressionResolver, + final Map recoveryStateFactories + ) { + this( + indexSettings, + analysisRegistry, + engineFactory, + engineConfigFactory, + directoryFactories, + allowExpensiveQueries, + expressionResolver, + recoveryStateFactories, + null + ); + } + /** * Adds a Setting and it's consumer for this index. */ diff --git a/server/src/main/java/org/opensearch/index/IndexService.java b/server/src/main/java/org/opensearch/index/IndexService.java index 08a9779a23c02..78bfedb7b6d65 100644 --- a/server/src/main/java/org/opensearch/index/IndexService.java +++ b/server/src/main/java/org/opensearch/index/IndexService.java @@ -312,6 +312,81 @@ public IndexService( updateFsyncTaskIfNecessary(); } + public IndexService( + IndexSettings indexSettings, + IndexCreationContext indexCreationContext, + NodeEnvironment nodeEnv, + NamedXContentRegistry xContentRegistry, + SimilarityService similarityService, + ShardStoreDeleter shardStoreDeleter, + IndexAnalyzers indexAnalyzers, + EngineFactory engineFactory, + EngineConfigFactory engineConfigFactory, + CircuitBreakerService circuitBreakerService, + BigArrays bigArrays, + ThreadPool threadPool, + ScriptService scriptService, + ClusterService clusterService, + Client client, + QueryCache queryCache, + IndexStorePlugin.DirectoryFactory directoryFactory, + IndexStorePlugin.DirectoryFactory remoteDirectoryFactory, + IndexEventListener eventListener, + Function> wrapperFactory, + MapperRegistry mapperRegistry, + IndicesFieldDataCache indicesFieldDataCache, + List searchOperationListeners, + List indexingOperationListeners, + NamedWriteableRegistry namedWriteableRegistry, + BooleanSupplier idFieldDataEnabled, + BooleanSupplier allowExpensiveQueries, + IndexNameExpressionResolver expressionResolver, + ValuesSourceRegistry valuesSourceRegistry, + IndexStorePlugin.RecoveryStateFactory recoveryStateFactory, + BiFunction translogFactorySupplier, + Supplier clusterDefaultRefreshIntervalSupplier, + RecoverySettings recoverySettings, + RemoteStoreSettings remoteStoreSettings + ) { + this( + indexSettings, + indexCreationContext, + nodeEnv, + xContentRegistry, + similarityService, + shardStoreDeleter, + indexAnalyzers, + engineFactory, + engineConfigFactory, + circuitBreakerService, + bigArrays, + threadPool, + scriptService, + clusterService, + client, + queryCache, + directoryFactory, + remoteDirectoryFactory, + eventListener, + wrapperFactory, + mapperRegistry, + indicesFieldDataCache, + searchOperationListeners, + indexingOperationListeners, + namedWriteableRegistry, + idFieldDataEnabled, + allowExpensiveQueries, + expressionResolver, + valuesSourceRegistry, + recoveryStateFactory, + translogFactorySupplier, + clusterDefaultRefreshIntervalSupplier, + recoverySettings, + remoteStoreSettings, + null + ); + } + static boolean needsMapperService(IndexSettings indexSettings, IndexCreationContext indexCreationContext) { return false == (indexSettings.getIndexMetadata().getState() == IndexMetadata.State.CLOSE && indexCreationContext == IndexCreationContext.CREATE_INDEX); // metadata verification needs a mapper service @@ -542,7 +617,7 @@ public synchronized IndexShard createShard( if (FeatureFlags.isEnabled(FeatureFlags.WRITEABLE_REMOTE_INDEX_SETTING) && // TODO : Need to remove this check after support for hot indices is added in Composite Directory this.indexSettings.isStoreLocalityPartial()) { - /** + /* * Currently Composite Directory only supports local directory to be of type FSDirectory * The reason is that FileCache currently has it key type as Path * Composite Directory currently uses FSDirectory's getDirectory() method to fetch and use the Path for operating on FileCache @@ -550,6 +625,7 @@ public synchronized IndexShard createShard( */ Directory localDirectory = directoryFactory.newDirectory(this.indexSettings, path); assert localDirectory instanceof FSDirectory : "For Composite Directory, local directory must be of type FSDirectory"; + assert fileCache != null : "File Cache not initialized on this Node, cannot create Composite Directory without FileCache"; directory = new CompositeDirectory((FSDirectory) localDirectory, (RemoteSegmentStoreDirectory) remoteDirectory, fileCache); } else { directory = directoryFactory.newDirectory(this.indexSettings, path); diff --git a/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java b/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java index 0cd05721d4ff9..ca9d7379de4c3 100644 --- a/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java +++ b/server/src/main/java/org/opensearch/index/shard/RemoteStoreRefreshListener.java @@ -250,6 +250,12 @@ private boolean syncSegments() { Map localSegmentsSizeMap = updateLocalSizeMapAndTracker(localSegmentsPostRefresh).entrySet() .stream() .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); + + Collection segmentsToRefresh = localSegmentsPostRefresh.stream() + .filter(file -> !skipUpload(file)) + .collect(Collectors.toList()); + Directory directory = ((FilterDirectory) (((FilterDirectory) storeDirectory).getDelegate())).getDelegate(); + CountDownLatch latch = new CountDownLatch(1); ActionListener segmentUploadsCompletedListener = new LatchedActionListener<>(new ActionListener<>() { @Override @@ -271,6 +277,9 @@ public void onResponse(Void unused) { // At this point since we have uploaded new segments, segment infos and segment metadata file, // along with marking minSeqNoToKeep, upload has succeeded completely. successful.set(true); + if (directory instanceof CompositeDirectory) { + ((CompositeDirectory) directory).afterSyncToRemote(segmentsToRefresh); + } } catch (Exception e) { // We don't want to fail refresh if upload of new segments fails. The missed segments will be re-tried // as part of exponential back-off retry logic. This should not affect durability of the indexed data @@ -285,15 +294,8 @@ public void onFailure(Exception e) { } }, latch); - Collection segmentsToRefresh = localSegmentsPostRefresh.stream() - .filter(file -> !skipUpload(file)) - .collect(Collectors.toList()); // Start the segments files upload uploadNewSegments(localSegmentsPostRefresh, localSegmentsSizeMap, segmentUploadsCompletedListener); - Directory directory = ((FilterDirectory) (((FilterDirectory) storeDirectory).getDelegate())).getDelegate(); - if (directory instanceof CompositeDirectory) { - ((CompositeDirectory) directory).afterSyncToRemote(segmentsToRefresh); - } latch.await(); } catch (EngineException e) { logger.warn("Exception while reading SegmentInfosSnapshot", e); diff --git a/server/src/main/java/org/opensearch/index/store/CloseableFilterIndexOutput.java b/server/src/main/java/org/opensearch/index/store/CloseableFilterIndexOutput.java new file mode 100644 index 0000000000000..8df5d648b91f3 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/store/CloseableFilterIndexOutput.java @@ -0,0 +1,45 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.store; + +import org.apache.lucene.store.IndexOutput; +import org.opensearch.common.lucene.store.FilterIndexOutput; + +import java.io.IOException; + +/** + * FilterIndexOutput which takes in an additional FunctionalInterface as a parameter to perform required operations once the IndexOutput is closed + * + * @opensearch.internal + */ +public class CloseableFilterIndexOutput extends FilterIndexOutput { + + /** + * Functional Interface which takes the name of the file as input on which the required operations are to be performed + */ + @FunctionalInterface + public interface OnCloseListener { + void onClose(String name); + } + + OnCloseListener onCloseListener; + String fileName; + + public CloseableFilterIndexOutput(IndexOutput out, String fileName, OnCloseListener onCloseListener) { + super("CloseableFilterIndexOutput for file " + fileName, out); + this.fileName = fileName; + this.onCloseListener = onCloseListener; + } + + @Override + public void close() throws IOException { + super.close(); + onCloseListener.onClose(fileName); + } +} diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java index 740aee3092e57..5536be1212ccb 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java @@ -15,28 +15,23 @@ import org.apache.lucene.store.IOContext; import org.apache.lucene.store.IndexInput; import org.apache.lucene.store.IndexOutput; -import org.apache.lucene.store.Lock; -import org.apache.lucene.store.LockObtainFailedException; import org.apache.lucene.util.Version; -import org.opensearch.common.lucene.store.FilterIndexOutput; +import org.opensearch.common.annotation.ExperimentalApi; import org.opensearch.common.lucene.store.InputStreamIndexInput; import org.opensearch.index.snapshots.blobstore.BlobStoreIndexShardSnapshot; import org.opensearch.index.store.remote.file.OnDemandBlockSnapshotIndexInput; -import org.opensearch.index.store.remote.file.OnDemandCompositeBlockIndexInput; +import org.opensearch.index.store.remote.filecache.CachedIndexInput; import org.opensearch.index.store.remote.filecache.FileCache; -import org.opensearch.index.store.remote.filecache.WrappedCachedIndexInput; +import org.opensearch.index.store.remote.filecache.NonBlockCachedIndexInput; import org.opensearch.index.store.remote.metadata.RemoteSegmentMetadata; -import org.opensearch.index.store.remote.utils.BlobFetchRequest; import org.opensearch.index.store.remote.utils.BlockIOContext; import org.opensearch.index.store.remote.utils.FileType; import org.opensearch.index.store.remote.utils.TransferManager; import java.io.FileNotFoundException; import java.io.IOException; -import java.io.InputStream; import java.nio.file.NoSuchFileException; import java.nio.file.Path; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.HashSet; @@ -49,7 +44,10 @@ * Consumers of Composite directory need not worry whether file is in local or remote * All such abstractions will be handled by the Composite directory itself * Implements all required methods by Directory abstraction + * + * @opensearch.internal */ +@ExperimentalApi public class CompositeDirectory extends FilterDirectory { private static final Logger logger = LogManager.getLogger(CompositeDirectory.class); private final FSDirectory localDirectory; @@ -59,7 +57,6 @@ public class CompositeDirectory extends FilterDirectory { private final ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock(); private final ReentrantReadWriteLock.WriteLock writeLock = readWriteLock.writeLock(); private final ReentrantReadWriteLock.ReadLock readLock = readWriteLock.readLock(); - private final RemoteDirectoryBlobStreamReader remoteDirectoryBlobStreamReader; /** * Constructor to initialise the composite directory @@ -72,10 +69,13 @@ public CompositeDirectory(FSDirectory localDirectory, RemoteSegmentStoreDirector this.localDirectory = localDirectory; this.remoteDirectory = remoteDirectory; this.fileCache = fileCache; - remoteDirectoryBlobStreamReader = new RemoteDirectoryBlobStreamReader(IOContext.DEFAULT, remoteDirectory); transferManager = new TransferManager( - remoteDirectoryBlobStreamReader, - fileCache); + (name, position, length) -> new InputStreamIndexInput( + remoteDirectory.openInput(name, new BlockIOContext(IOContext.DEFAULT, position, length)), + length + ), + fileCache + ); } /** @@ -86,17 +86,15 @@ public CompositeDirectory(FSDirectory localDirectory, RemoteSegmentStoreDirector */ @Override public String[] listAll() throws IOException { - logger.trace("listAll() called ..."); + logger.trace("listAll() called"); readLock.lock(); try { String[] localFiles = localDirectory.listAll(); - logger.trace("LocalDirectory files : {}", () -> Arrays.toString(localFiles)); + logger.trace("Local Directory files : {}", () -> Arrays.toString(localFiles)); Set allFiles = new HashSet<>(Arrays.asList(localFiles)); - if (remoteDirectory != null) { - String[] remoteFiles = getRemoteFiles(); - allFiles.addAll(Arrays.asList(remoteFiles)); - logger.trace("Remote Directory files : {}", () -> Arrays.toString(remoteFiles)); - } + String[] remoteFiles = getRemoteFiles(); + allFiles.addAll(Arrays.asList(remoteFiles)); + logger.trace("Remote Directory files : {}", () -> Arrays.toString(remoteFiles)); Set localLuceneFiles = allFiles.stream() .filter(file -> !FileType.isBlockFile(file)) .collect(Collectors.toUnmodifiableSet()); @@ -112,7 +110,7 @@ public String[] listAll() throws IOException { /** * Removes an existing file in the directory. - * Throws {@link NoSuchFileException} or {@link FileNotFoundException} in case file is not present locally and in remote as well + * Currently deleting only from local directory as files from remote should not be deleted due to availability reasons * @param name the name of an existing file. * @throws IOException in case of I/O error */ @@ -121,11 +119,11 @@ public void deleteFile(String name) throws IOException { logger.trace("deleteFile() called {}", name); writeLock.lock(); try { - localDirectory.deleteFile(name); + /* + Not deleting from localDirectory directly since it causes a race condition when the localDirectory deletes a file, and it ends up in pendingDeletion state. + Meanwhile, fileCache on removal deletes the file directly via the Files class and later when the directory tries to delete the files pending for deletion (which happens before creating a new file), it causes NoSuchFileException and new file creation fails + */ fileCache.remove(localDirectory.getDirectory().resolve(name)); - } catch (NoSuchFileException | FileNotFoundException e) { - logger.trace("File {} not found in local, trying to delete from Remote", name); - remoteDirectory.deleteFile(name); } finally { writeLock.unlock(); } @@ -143,8 +141,10 @@ public long fileLength(String name) throws IOException { readLock.lock(); try { long fileLength; - if (isTempFile(name) || fileCache.get(localDirectory.getDirectory().resolve(name)) != null) { + Path key = localDirectory.getDirectory().resolve(name); + if (isTempFile(name) || fileCache.get(key) != null) { fileLength = localDirectory.fileLength(name); + fileCache.decRef(key); logger.trace("fileLength from Local {}", fileLength); } else { fileLength = remoteDirectory.fileLength(name); @@ -167,10 +167,16 @@ public IndexOutput createOutput(String name, IOContext context) throws IOExcepti logger.trace("createOutput() called {}", name); writeLock.lock(); try { - /** - * The CacheableIndexOutput will ensure that the file is added to FileCache once write is completed on this file + /* + * The CloseableFilterIndexOutput will ensure that the file is added to FileCache once write is completed on this file */ - return new CacheableIndexOutput(localDirectory.createOutput(name, context), name); + return new CloseableFilterIndexOutput(localDirectory.createOutput(name, context), name, (fileName) -> { + try { + cacheFile(fileName); + } catch (IOException e) { + throw new RuntimeException(e); + } + }); } finally { writeLock.unlock(); } @@ -203,13 +209,12 @@ public void sync(Collection names) throws IOException { logger.trace("sync() called {}", names); writeLock.lock(); try { - Collection newLocalFiles = new ArrayList<>(); Collection remoteFiles = Arrays.asList(getRemoteFiles()); - for (String name : names) { - if (remoteFiles.contains(name) == false) newLocalFiles.add(name); - } - logger.trace("Synced files : {}", newLocalFiles); - localDirectory.sync(newLocalFiles); + Collection filesToSync = names.stream() + .filter(name -> remoteFiles.contains(name) == false) + .collect(Collectors.toList()); + logger.trace("Synced files : {}", filesToSync); + localDirectory.sync(filesToSync); } finally { writeLock.unlock(); } @@ -259,69 +264,49 @@ public IndexInput openInput(String name, IOContext context) throws IOException { logger.trace("openInput() called {}", name); writeLock.lock(); try { - remoteDirectoryBlobStreamReader.setContext(context); - /** + /* * We aren't tracking temporary files (created via createTempOutput) currently in FileCache as these are created and then deleted within a very short span of time * We will be reading them directory from the local directory */ if (isTempFile(name)) { return localDirectory.openInput(name, context); } - /** + /* * Return directly from the FileCache (via TransferManager) if complete file is present */ - else if (fileCache.get(localDirectory.getDirectory().resolve(name)) != null) { + + Path key = localDirectory.getDirectory().resolve(name); + CachedIndexInput indexInput = fileCache.get(key); + if (indexInput != null) { logger.trace("Complete file found in FileCache"); - BlobFetchRequest blobFetchRequest = BlobFetchRequest.builder() - .directory(localDirectory) - .fileName(name) - // position and length are not required here since this is a complete file, just adding dummy values for validation - .blobParts(new ArrayList<>(Arrays.asList(new BlobFetchRequest.BlobPart(name, 0, 1)))) - .build(); - return transferManager.fetchBlob(blobFetchRequest); + try { + return indexInput.getIndexInput().clone(); + } finally { + fileCache.decRef(key); + } } - /** + /* * If file has been uploaded to the Remote Store, fetch it from the Remote Store in blocks via OnDemandCompositeBlockIndexInput */ else { logger.trace("Complete file not in FileCache, to be fetched in Blocks from Remote"); RemoteSegmentMetadata remoteSegmentMetadata = remoteDirectory.readLatestMetadataFile(); RemoteSegmentStoreDirectory.UploadedSegmentMetadata uploadedSegmentMetadata = remoteSegmentMetadata.getMetadata().get(name); - /** + /* * TODO : Refactor FileInfo and OnDemandBlockSnapshotIndexInput to more generic names as they are not Remote Snapshot specific */ BlobStoreIndexShardSnapshot.FileInfo fileInfo = new BlobStoreIndexShardSnapshot.FileInfo( name, - new StoreFileMetadata(name, uploadedSegmentMetadata.getLength(), - uploadedSegmentMetadata.getChecksum(), Version.LATEST), + new StoreFileMetadata(name, uploadedSegmentMetadata.getLength(), uploadedSegmentMetadata.getChecksum(), Version.LATEST), null ); return new OnDemandBlockSnapshotIndexInput(fileInfo, localDirectory, transferManager); - //return new OnDemandCompositeBlockIndexInput(remoteDirectory, name, localDirectory, fileCache, context); } } finally { writeLock.unlock(); } } - /** - * Acquires and returns a {@link Lock} for a file with the given name. - * @param name the name of the lock file - * @throws LockObtainFailedException (optional specific exception) if the lock could not be - * obtained because it is currently held elsewhere. - * @throws IOException in case of I/O error - */ - @Override - public Lock obtainLock(String name) throws IOException { - logger.trace("obtainLock() called {}", name); - writeLock.lock(); - try { - return localDirectory.obtainLock(name); - } finally { - writeLock.unlock(); - } - } - /** * Closes the directory * @throws IOException in case of I/O error @@ -330,7 +315,12 @@ public Lock obtainLock(String name) throws IOException { public void close() throws IOException { writeLock.lock(); try { + Arrays.stream(localDirectory.listAll()).forEach(f -> { + logger.trace("Removing file from cache {}", f); + fileCache.remove(localDirectory.getDirectory().resolve(f)); + }); localDirectory.close(); + remoteDirectory.close(); } finally { writeLock.unlock(); } @@ -366,11 +356,11 @@ public void afterSyncToRemote(Collection files) throws IOException { writeLock.lock(); try { /** - * TODO - Unpin the files here from FileCache so that they become eligible for eviction, once pinning/unpinning support is added in FileCache + * TODO - Unpin the files here from FileCache so that they become eligible for eviction, once pinning/unpinning support is added in FileCache * Uncomment the below commented line(to remove the file from cache once uploaded) to test block based functionality */ logger.trace("File {} uploaded to Remote Store and now can be eligible for eviction in FileCache", fileName); - fileCache.remove(localDirectory.getDirectory().resolve(fileName)); + // fileCache.remove(localDirectory.getDirectory().resolve(fileName)); } finally { writeLock.unlock(); } @@ -403,41 +393,11 @@ private String[] getRemoteFiles() throws IOException { private void cacheFile(String name) throws IOException { Path filePath = localDirectory.getDirectory().resolve(name); - fileCache.put(filePath, new WrappedCachedIndexInput(localDirectory.openInput(name, IOContext.READ))); - } - - private class CacheableIndexOutput extends FilterIndexOutput { - - String fileName; - - public CacheableIndexOutput(IndexOutput out, String fileName) { - super("CacheableIndexOutput for file : " + fileName, out); - this.fileName = fileName; - } - - @Override - public void close() throws IOException { - super.close(); - cacheFile(fileName); - } + fileCache.put(filePath, new NonBlockCachedIndexInput(localDirectory.openInput(name, IOContext.READ))); + // Decrementing ref here as above put call increments the ref of the key + fileCache.decRef(filePath); + // TODO : Pin the above filePath in the file cache once pinning support is added so that it cannot be evicted unless it has been + // successfully uploaded to Remote } - private class RemoteDirectoryBlobStreamReader implements TransferManager.BlobStreamReader { - private IOContext context; - private final RemoteSegmentStoreDirectory remoteDirectory; - - RemoteDirectoryBlobStreamReader(IOContext context, RemoteSegmentStoreDirectory remoteDirectory) { - this.context = context; - this.remoteDirectory = remoteDirectory; - } - - void setContext(IOContext context) { - this.context = context; - } - - @Override - public InputStream read(String name, long position, long length) throws IOException { - return new InputStreamIndexInput(remoteDirectory.openInput(name, new BlockIOContext(context, position, length)), length); - } - } } diff --git a/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandBlockSnapshotIndexInput.java b/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandBlockSnapshotIndexInput.java index 1f70fd132ce17..ad56127394779 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandBlockSnapshotIndexInput.java +++ b/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandBlockSnapshotIndexInput.java @@ -13,7 +13,6 @@ import org.apache.lucene.store.FSDirectory; import org.apache.lucene.store.IndexInput; import org.opensearch.index.snapshots.blobstore.BlobStoreIndexShardSnapshot.FileInfo; -import org.opensearch.index.store.CompositeDirectory; import org.opensearch.index.store.remote.utils.BlobFetchRequest; import org.opensearch.index.store.remote.utils.TransferManager; @@ -142,7 +141,14 @@ protected IndexInput fetchBlock(int blockId) throws IOException { final long blockStart = getBlockStart(blockId); final long blockEnd = blockStart + getActualBlockSize(blockId); - logger.trace("File: {} , Block File: {} , BlockStart: {} , BlockEnd: {} , OriginalFileSize: {}", fileName, blockFileName, blockStart, blockEnd, originalFileSize); + logger.trace( + "File: {} , Block File: {} , BlockStart: {} , BlockEnd: {} , OriginalFileSize: {}", + fileName, + blockFileName, + blockStart, + blockEnd, + originalFileSize + ); // Block may be present on multiple chunks of a file, so we need // to fetch each chunk/blob part separately to fetch an entire block. diff --git a/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java b/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java deleted file mode 100644 index f024592330512..0000000000000 --- a/server/src/main/java/org/opensearch/index/store/remote/file/OnDemandCompositeBlockIndexInput.java +++ /dev/null @@ -1,137 +0,0 @@ -/* - * SPDX-License-Identifier: Apache-2.0 - * - * The OpenSearch Contributors require contributions made to - * this file be licensed under the Apache-2.0 license or a - * compatible open source license. - */ - -package org.opensearch.index.store.remote.file; - -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; -import org.apache.lucene.store.FSDirectory; -import org.apache.lucene.store.IOContext; -import org.apache.lucene.store.IndexInput; -import org.opensearch.common.lucene.store.InputStreamIndexInput; -import org.opensearch.index.store.RemoteSegmentStoreDirectory; -import org.opensearch.index.store.remote.filecache.FileCache; -import org.opensearch.index.store.remote.utils.BlobFetchRequest; -import org.opensearch.index.store.remote.utils.BlockIOContext; -import org.opensearch.index.store.remote.utils.TransferManager; - -import java.io.IOException; -import java.util.ArrayList; -import java.util.List; - -/** - * OnDemandCompositeBlockIndexInput is used by the Composite Directory to read data in blocks from Remote and cache those blocks in FileCache - */ -public class OnDemandCompositeBlockIndexInput extends OnDemandBlockIndexInput { - - private static final Logger logger = LogManager.getLogger(OnDemandCompositeBlockIndexInput.class); - private final RemoteSegmentStoreDirectory remoteDirectory; - private final String fileName; - private final Long originalFileSize; - private final FSDirectory localDirectory; - private final IOContext context; - private final FileCache fileCache; - private final TransferManager transferManager; - - public OnDemandCompositeBlockIndexInput( - RemoteSegmentStoreDirectory remoteDirectory, - String fileName, - FSDirectory localDirectory, - FileCache fileCache, - IOContext context - ) throws IOException { - this( - OnDemandBlockIndexInput.builder() - .resourceDescription("OnDemandCompositeBlockIndexInput") - .isClone(false) - .offset(0L) - .length(remoteDirectory.fileLength(fileName)), - remoteDirectory, - fileName, - localDirectory, - fileCache, - context - ); - } - - public OnDemandCompositeBlockIndexInput( - Builder builder, - RemoteSegmentStoreDirectory remoteDirectory, - String fileName, - FSDirectory localDirectory, - FileCache fileCache, - IOContext context - ) throws IOException { - super(builder); - this.remoteDirectory = remoteDirectory; - this.localDirectory = localDirectory; - this.fileName = fileName; - this.fileCache = fileCache; - this.context = context; - this.transferManager = new TransferManager( - (name, position, length) -> new InputStreamIndexInput(remoteDirectory.openInput(name, new BlockIOContext(context, position, length)), length), - fileCache); - originalFileSize = remoteDirectory.fileLength(fileName); - } - - @Override - protected OnDemandCompositeBlockIndexInput buildSlice(String sliceDescription, long offset, long length) { - try { - return new OnDemandCompositeBlockIndexInput( - OnDemandBlockIndexInput.builder() - .blockSizeShift(blockSizeShift) - .isClone(true) - .offset(this.offset + offset) - .length(length) - .resourceDescription(sliceDescription), - remoteDirectory, - fileName, - localDirectory, - fileCache, - context - ); - } catch (IOException e) { - throw new RuntimeException(e); - } - } - - @Override - protected IndexInput fetchBlock(int blockId) throws IOException { - logger.trace("fetchBlock called with blockId -> {}", blockId); - final String blockFileName = fileName + "_block_" + blockId; - final long blockStart = getBlockStart(blockId); - final long length = getActualBlockSize(blockId); - logger.trace( - "File: {} , Block File: {} , Length: {} , BlockSize: {} , OriginalFileSize: {}", - fileName, - blockFileName, - blockStart, - length, - originalFileSize - ); - BlobFetchRequest blobFetchRequest = BlobFetchRequest.builder() - .directory(localDirectory) - .fileName(blockFileName) - .blobParts(new ArrayList<>(List.of(new BlobFetchRequest.BlobPart(fileName, blockStart, length)))) - .build(); - return transferManager.fetchBlob(blobFetchRequest); - } - - @Override - public OnDemandBlockIndexInput clone() { - OnDemandCompositeBlockIndexInput clone = buildSlice("clone", 0L, this.length); - // ensures that clones may be positioned at the same point as the blocked file they were cloned from - clone.cloneBlock(this); - return clone; - } - - private long getActualBlockSize(int blockId) { - return (blockId != getBlock(originalFileSize - 1)) ? blockSize : getBlockOffset(originalFileSize - 1) + 1; - } - -} diff --git a/server/src/main/java/org/opensearch/index/store/remote/filecache/WrappedCachedIndexInput.java b/server/src/main/java/org/opensearch/index/store/remote/filecache/NonBlockCachedIndexInput.java similarity index 77% rename from server/src/main/java/org/opensearch/index/store/remote/filecache/WrappedCachedIndexInput.java rename to server/src/main/java/org/opensearch/index/store/remote/filecache/NonBlockCachedIndexInput.java index 67b12a10810ed..ee520dc22964e 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/filecache/WrappedCachedIndexInput.java +++ b/server/src/main/java/org/opensearch/index/store/remote/filecache/NonBlockCachedIndexInput.java @@ -14,17 +14,17 @@ import java.util.concurrent.atomic.AtomicBoolean; /** - * Wrapper implementation of the CachedIndexInput which takes in an IndexInput as parameter + * Implementation of the CachedIndexInput for NON_BLOCK files which takes in an IndexInput as parameter */ -public class WrappedCachedIndexInput implements CachedIndexInput { +public class NonBlockCachedIndexInput implements CachedIndexInput { - IndexInput indexInput; - AtomicBoolean isClosed; + private final IndexInput indexInput; + private final AtomicBoolean isClosed; /** * Constructor - takes IndexInput as parameter */ - public WrappedCachedIndexInput(IndexInput indexInput) { + public NonBlockCachedIndexInput(IndexInput indexInput) { this.indexInput = indexInput; isClosed = new AtomicBoolean(false); } @@ -58,7 +58,7 @@ public boolean isClosed() { */ @Override public void close() throws Exception { - if (!isClosed()) { + if (!isClosed.getAndSet(true)) { indexInput.close(); isClosed.set(true); } diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java b/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java index f123a923f5ef6..df26f2f0925f6 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java +++ b/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java @@ -12,7 +12,6 @@ import org.apache.logging.log4j.Logger; import org.apache.lucene.store.IOContext; import org.apache.lucene.store.IndexInput; -import org.opensearch.common.blobstore.BlobContainer; import org.opensearch.index.store.remote.filecache.CachedIndexInput; import org.opensearch.index.store.remote.filecache.FileCache; import org.opensearch.index.store.remote.filecache.FileCachedIndexInput; @@ -39,17 +38,19 @@ public class TransferManager { private static final Logger logger = LogManager.getLogger(TransferManager.class); + /** + * Functional interface to get an InputStream for a file at a certain offset and size + */ @FunctionalInterface - public interface BlobStreamReader { + public interface StreamReader { InputStream read(String name, long position, long length) throws IOException; } - private final BlobStreamReader blobStreamReader; + private final StreamReader streamReader; private final FileCache fileCache; - - public TransferManager(final BlobStreamReader blobStreamReader, final FileCache fileCache) { - this.blobStreamReader = blobStreamReader; + public TransferManager(final StreamReader streamReader, final FileCache fileCache) { + this.streamReader = streamReader; this.fileCache = fileCache; } @@ -67,7 +68,7 @@ public IndexInput fetchBlob(BlobFetchRequest blobFetchRequest) throws IOExceptio if (cachedIndexInput == null || cachedIndexInput.isClosed()) { logger.trace("Transfer Manager - IndexInput closed or not in cache"); // Doesn't exist or is closed, either way create a new one - return new DelayedCreationCachedIndexInput(fileCache, blobStreamReader, blobFetchRequest); + return new DelayedCreationCachedIndexInput(fileCache, streamReader, blobFetchRequest); } else { logger.trace("Transfer Manager - Already in cache"); // already in the cache and ready to be used (open) @@ -86,7 +87,7 @@ public IndexInput fetchBlob(BlobFetchRequest blobFetchRequest) throws IOExceptio } @SuppressWarnings("removal") - private static FileCachedIndexInput createIndexInput(FileCache fileCache, BlobStreamReader blobStreamReader, BlobFetchRequest request) { + private static FileCachedIndexInput createIndexInput(FileCache fileCache, StreamReader streamReader, BlobFetchRequest request) { // We need to do a privileged action here in order to fetch from remote // and write to the local file cache in case this is invoked as a side // effect of a plugin (such as a scripted search) that doesn't have the @@ -101,7 +102,7 @@ private static FileCachedIndexInput createIndexInput(FileCache fileCache, BlobSt ) { for (BlobFetchRequest.BlobPart blobPart : request.blobParts()) { try ( - InputStream snapshotFileInputStream = blobStreamReader.read( + InputStream snapshotFileInputStream = streamReader.read( blobPart.getBlobName(), blobPart.getPosition(), blobPart.getLength() @@ -129,15 +130,15 @@ private static FileCachedIndexInput createIndexInput(FileCache fileCache, BlobSt */ private static class DelayedCreationCachedIndexInput implements CachedIndexInput { private final FileCache fileCache; - private final BlobStreamReader blobStreamReader; + private final StreamReader streamReader; private final BlobFetchRequest request; private final CompletableFuture result = new CompletableFuture<>(); private final AtomicBoolean isStarted = new AtomicBoolean(false); private final AtomicBoolean isClosed = new AtomicBoolean(false); - private DelayedCreationCachedIndexInput(FileCache fileCache, BlobStreamReader blobStreamReader, BlobFetchRequest request) { + private DelayedCreationCachedIndexInput(FileCache fileCache, StreamReader streamReader, BlobFetchRequest request) { this.fileCache = fileCache; - this.blobStreamReader = blobStreamReader; + this.streamReader = streamReader; this.request = request; } @@ -149,7 +150,7 @@ public IndexInput getIndexInput() throws IOException { if (isStarted.getAndSet(true) == false) { // We're the first one here, need to download the block try { - result.complete(createIndexInput(fileCache, blobStreamReader, request)); + result.complete(createIndexInput(fileCache, streamReader, request)); } catch (Exception e) { result.completeExceptionally(e); fileCache.remove(request.getFilePath()); diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index 7fc758f4bfcd6..3f8a3d20ce12d 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/server/src/main/java/org/opensearch/indices/IndicesService.java @@ -443,7 +443,6 @@ public void onRemoval(ShardId shardId, String fieldName, boolean wasEvicted, lon this.directoryFactories = directoryFactories; this.recoveryStateFactories = recoveryStateFactories; - this.fileCache = fileCache; // doClose() is called when shutting down a node, yet there might still be ongoing requests // that we need to wait for before closing some resources such as the caches. In order to // avoid closing these resources while ongoing requests are still being processed, we use a @@ -499,6 +498,68 @@ protected void closeInternal() { .addSettingsUpdateConsumer(CLUSTER_DEFAULT_INDEX_REFRESH_INTERVAL_SETTING, this::onRefreshIntervalUpdate); this.recoverySettings = recoverySettings; this.remoteStoreSettings = remoteStoreSettings; + this.fileCache = fileCache; + } + + public IndicesService( + Settings settings, + PluginsService pluginsService, + NodeEnvironment nodeEnv, + NamedXContentRegistry xContentRegistry, + AnalysisRegistry analysisRegistry, + IndexNameExpressionResolver indexNameExpressionResolver, + MapperRegistry mapperRegistry, + NamedWriteableRegistry namedWriteableRegistry, + ThreadPool threadPool, + IndexScopedSettings indexScopedSettings, + CircuitBreakerService circuitBreakerService, + BigArrays bigArrays, + ScriptService scriptService, + ClusterService clusterService, + Client client, + MetaStateService metaStateService, + Collection>> engineFactoryProviders, + Map directoryFactories, + ValuesSourceRegistry valuesSourceRegistry, + Map recoveryStateFactories, + IndexStorePlugin.DirectoryFactory remoteDirectoryFactory, + Supplier repositoriesServiceSupplier, + SearchRequestStats searchRequestStats, + @Nullable RemoteStoreStatsTrackerFactory remoteStoreStatsTrackerFactory, + RecoverySettings recoverySettings, + CacheService cacheService, + RemoteStoreSettings remoteStoreSettings + ) { + this( + settings, + pluginsService, + nodeEnv, + xContentRegistry, + analysisRegistry, + indexNameExpressionResolver, + mapperRegistry, + namedWriteableRegistry, + threadPool, + indexScopedSettings, + circuitBreakerService, + bigArrays, + scriptService, + clusterService, + client, + metaStateService, + engineFactoryProviders, + directoryFactories, + valuesSourceRegistry, + recoveryStateFactories, + remoteDirectoryFactory, + repositoriesServiceSupplier, + searchRequestStats, + remoteStoreStatsTrackerFactory, + recoverySettings, + cacheService, + remoteStoreSettings, + null + ); } /** diff --git a/server/src/test/java/org/opensearch/index/IndexModuleTests.java b/server/src/test/java/org/opensearch/index/IndexModuleTests.java index 6eddd8c4a9b4a..4ce4936c047d9 100644 --- a/server/src/test/java/org/opensearch/index/IndexModuleTests.java +++ b/server/src/test/java/org/opensearch/index/IndexModuleTests.java @@ -278,8 +278,7 @@ public void testWrapperIsBound() throws IOException { Collections.emptyMap(), () -> true, new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)), - Collections.emptyMap(), - null + Collections.emptyMap() ); module.setReaderWrapper(s -> new Wrapper()); @@ -305,8 +304,7 @@ public void testRegisterIndexStore() throws IOException { indexStoreFactories, () -> true, new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)), - Collections.emptyMap(), - null + Collections.emptyMap() ); final IndexService indexService = newIndexService(module); @@ -634,8 +632,7 @@ public void testRegisterCustomRecoveryStateFactory() throws IOException { Collections.emptyMap(), () -> true, new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)), - recoveryStateFactories, - null + recoveryStateFactories ); final IndexService indexService = newIndexService(module); @@ -667,8 +664,7 @@ private static IndexModule createIndexModule(IndexSettings indexSettings, Analys Collections.emptyMap(), () -> true, new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY)), - Collections.emptyMap(), - null + Collections.emptyMap() ); } diff --git a/server/src/test/java/org/opensearch/index/store/BaseRemoteSegmentStoreDirectoryTests.java b/server/src/test/java/org/opensearch/index/store/BaseRemoteSegmentStoreDirectoryTests.java new file mode 100644 index 0000000000000..ff9b62a341deb --- /dev/null +++ b/server/src/test/java/org/opensearch/index/store/BaseRemoteSegmentStoreDirectoryTests.java @@ -0,0 +1,178 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.store; + +import org.apache.lucene.index.SegmentInfos; +import org.opensearch.cluster.metadata.IndexMetadata; +import org.opensearch.common.settings.Settings; +import org.opensearch.common.util.concurrent.OpenSearchExecutors; +import org.opensearch.index.engine.NRTReplicationEngineFactory; +import org.opensearch.index.shard.IndexShard; +import org.opensearch.index.shard.IndexShardTestCase; +import org.opensearch.index.store.lockmanager.RemoteStoreMetadataLockManager; +import org.opensearch.indices.replication.common.ReplicationType; +import org.opensearch.threadpool.ThreadPool; +import org.junit.After; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ExecutorService; + +import static org.opensearch.index.store.RemoteSegmentStoreDirectory.METADATA_FILES_TO_FETCH; +import static org.opensearch.test.RemoteStoreTestUtils.createMetadataFileBytes; +import static org.opensearch.test.RemoteStoreTestUtils.getDummyMetadata; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class BaseRemoteSegmentStoreDirectoryTests extends IndexShardTestCase { + + protected RemoteDirectory remoteDataDirectory; + protected RemoteDirectory remoteMetadataDirectory; + protected RemoteStoreMetadataLockManager mdLockManager; + protected RemoteSegmentStoreDirectory remoteSegmentStoreDirectory; + protected TestUploadListener testUploadTracker; + protected IndexShard indexShard; + protected SegmentInfos segmentInfos; + protected ThreadPool threadPool; + + protected final String metadataFilename = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename( + 12, + 23, + 34, + 1, + 1, + "node-1" + ); + + protected final String metadataFilenameDup = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename( + 12, + 23, + 34, + 2, + 1, + "node-2" + ); + protected final String metadataFilename2 = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename( + 12, + 13, + 34, + 1, + 1, + "node-1" + ); + protected final String metadataFilename3 = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename( + 10, + 38, + 34, + 1, + 1, + "node-1" + ); + protected final String metadataFilename4 = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename( + 10, + 36, + 34, + 1, + 1, + "node-1" + ); + + public void setupRemoteSegmentStoreDirectory() throws IOException { + remoteDataDirectory = mock(RemoteDirectory.class); + remoteMetadataDirectory = mock(RemoteDirectory.class); + mdLockManager = mock(RemoteStoreMetadataLockManager.class); + threadPool = mock(ThreadPool.class); + testUploadTracker = new TestUploadListener(); + + Settings indexSettings = Settings.builder() + .put(IndexMetadata.SETTING_VERSION_CREATED, org.opensearch.Version.CURRENT) + .put(IndexMetadata.SETTING_REPLICATION_TYPE, ReplicationType.SEGMENT) + .build(); + ExecutorService executorService = OpenSearchExecutors.newDirectExecutorService(); + + indexShard = newStartedShard(false, indexSettings, new NRTReplicationEngineFactory()); + remoteSegmentStoreDirectory = new RemoteSegmentStoreDirectory( + remoteDataDirectory, + remoteMetadataDirectory, + mdLockManager, + threadPool, + indexShard.shardId() + ); + try (Store store = indexShard.store()) { + segmentInfos = store.readLastCommittedSegmentsInfo(); + } + + when(threadPool.executor(ThreadPool.Names.REMOTE_PURGE)).thenReturn(executorService); + when(threadPool.executor(ThreadPool.Names.REMOTE_RECOVERY)).thenReturn(executorService); + when(threadPool.executor(ThreadPool.Names.SAME)).thenReturn(executorService); + } + + protected Map> populateMetadata() throws IOException { + List metadataFiles = new ArrayList<>(); + + metadataFiles.add(metadataFilename); + metadataFiles.add(metadataFilename2); + metadataFiles.add(metadataFilename3); + + when( + remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX, + METADATA_FILES_TO_FETCH + ) + ).thenReturn(List.of(metadataFilename)); + when( + remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( + RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX, + Integer.MAX_VALUE + ) + ).thenReturn(metadataFiles); + + Map> metadataFilenameContentMapping = Map.of( + metadataFilename, + getDummyMetadata("_0", 1), + metadataFilename2, + getDummyMetadata("_0", 1), + metadataFilename3, + getDummyMetadata("_0", 1) + ); + + when(remoteMetadataDirectory.getBlobStream(metadataFilename)).thenAnswer( + I -> createMetadataFileBytes( + metadataFilenameContentMapping.get(metadataFilename), + indexShard.getLatestReplicationCheckpoint(), + segmentInfos + ) + ); + when(remoteMetadataDirectory.getBlobStream(metadataFilename2)).thenAnswer( + I -> createMetadataFileBytes( + metadataFilenameContentMapping.get(metadataFilename2), + indexShard.getLatestReplicationCheckpoint(), + segmentInfos + ) + ); + when(remoteMetadataDirectory.getBlobStream(metadataFilename3)).thenAnswer( + I -> createMetadataFileBytes( + metadataFilenameContentMapping.get(metadataFilename3), + indexShard.getLatestReplicationCheckpoint(), + segmentInfos + ) + ); + + return metadataFilenameContentMapping; + } + + @After + public void tearDown() throws Exception { + indexShard.close("test tearDown", true, false); + super.tearDown(); + } + +} diff --git a/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java b/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java new file mode 100644 index 0000000000000..56d12537bf679 --- /dev/null +++ b/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java @@ -0,0 +1,189 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.store; + +import com.carrotsearch.randomizedtesting.annotations.ThreadLeakFilters; + +import org.apache.lucene.store.FSDirectory; +import org.apache.lucene.store.IOContext; +import org.apache.lucene.store.IndexInput; +import org.apache.lucene.store.IndexOutput; +import org.opensearch.index.store.remote.file.CleanerDaemonThreadLeakFilter; +import org.opensearch.index.store.remote.file.OnDemandBlockSnapshotIndexInput; +import org.opensearch.index.store.remote.filecache.CachedIndexInput; +import org.opensearch.index.store.remote.filecache.FileCache; +import org.opensearch.index.store.remote.filecache.NonBlockCachedIndexInput; +import org.junit.Before; + +import java.io.IOException; +import java.nio.file.Path; +import java.util.Arrays; +import java.util.Collection; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.startsWith; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +@ThreadLeakFilters(filters = CleanerDaemonThreadLeakFilter.class) +public class CompositeDirectoryTests extends BaseRemoteSegmentStoreDirectoryTests { + private FileCache fileCache; + private FSDirectory localDirectory; + private CompositeDirectory compositeDirectory; + + @Before + public void setup() throws IOException { + setupRemoteSegmentStoreDirectory(); + localDirectory = mock(FSDirectory.class); + fileCache = mock(FileCache.class); + compositeDirectory = new CompositeDirectory(localDirectory, remoteSegmentStoreDirectory, fileCache); + } + + public void testListAll() throws IOException { + populateMetadata(); + when(localDirectory.listAll()).thenReturn(new String[] { "_1.cfe", "_2.cfe", "_0.cfe_block_7", "_0.cfs_block_7" }); + + String[] actualFileNames = compositeDirectory.listAll(); + String[] expectedFileNames = new String[] { "_0.cfe", "_0.cfs", "_0.si", "_1.cfe", "_2.cfe", "segments_1" }; + assertArrayEquals(expectedFileNames, actualFileNames); + } + + public void testDeleteFile() throws IOException { + Path basePath = mock(Path.class); + Path resolvedPath = mock(Path.class); + when(basePath.resolve("_0.si")).thenReturn(resolvedPath); + when(localDirectory.getDirectory()).thenReturn(basePath); + + compositeDirectory.deleteFile("_0.si"); + verify(fileCache).remove(resolvedPath); + } + + public void testFileLength() throws IOException { + populateMetadata(); + remoteSegmentStoreDirectory.init(); + Path basePath = mock(Path.class); + Path resolvedPath = mock(Path.class); + when(basePath.resolve("_0.si")).thenReturn(resolvedPath); + when(localDirectory.getDirectory()).thenReturn(basePath); + when(localDirectory.fileLength("_0.si")).thenReturn(7L); + + // File present locally + CachedIndexInput indexInput = mock(CachedIndexInput.class); + when(fileCache.get(resolvedPath)).thenReturn(indexInput); + assertEquals(compositeDirectory.fileLength("_0.si"), 7L); + verify(localDirectory).fileLength(startsWith("_0.si")); + + // File not present locally + Map uploadedSegments = remoteSegmentStoreDirectory + .getSegmentsUploadedToRemoteStore(); + assertTrue(uploadedSegments.containsKey("_0.si")); + when(fileCache.get(resolvedPath)).thenReturn(null); + assertEquals(compositeDirectory.fileLength("_0.si"), uploadedSegments.get("_0.si").getLength()); + } + + public void testCreateOutput() throws IOException { + IndexOutput indexOutput = mock(IndexOutput.class); + when(localDirectory.createOutput("_0.si", IOContext.DEFAULT)).thenReturn(indexOutput); + IndexOutput actualIndexOutput = compositeDirectory.createOutput("_0.si", IOContext.DEFAULT); + assert actualIndexOutput instanceof CloseableFilterIndexOutput; + verify(localDirectory).createOutput("_0.si", IOContext.DEFAULT); + } + + public void testCreateTempOutput() throws IOException { + IndexOutput indexOutput = mock(IndexOutput.class); + when(localDirectory.createTempOutput("prefix", "suffix", IOContext.DEFAULT)).thenReturn(indexOutput); + compositeDirectory.createTempOutput("prefix", "suffix", IOContext.DEFAULT); + verify(localDirectory).createTempOutput("prefix", "suffix", IOContext.DEFAULT); + } + + public void testSync() throws IOException { + populateMetadata(); + remoteSegmentStoreDirectory.init(); + Collection names = List.of("_0.cfe", "_0.cfs", "_1.cfe", "_1.cfs", "_2.nvm", "segments_1"); + compositeDirectory.sync(names); + verify(localDirectory).sync(List.of("_1.cfe", "_1.cfs", "_2.nvm")); + } + + public void testSyncMetaData() throws IOException { + compositeDirectory.syncMetaData(); + verify(localDirectory).syncMetaData(); + } + + public void testRename() throws IOException { + Path basePath = mock(Path.class); + Path resolvedPathOldFile = mock(Path.class); + Path resolvedPathNewFile = mock(Path.class); + when(basePath.resolve("old_file_name")).thenReturn(resolvedPathOldFile); + when(basePath.resolve("new_file_name")).thenReturn(resolvedPathNewFile); + when(localDirectory.getDirectory()).thenReturn(basePath); + CachedIndexInput indexInput = mock(CachedIndexInput.class); + when(fileCache.get(resolvedPathNewFile)).thenReturn(indexInput); + compositeDirectory.rename("old_file_name", "new_file_name"); + verify(localDirectory).rename("old_file_name", "new_file_name"); + verify(fileCache).remove(resolvedPathOldFile); + verify(fileCache).put(eq(resolvedPathNewFile), any(NonBlockCachedIndexInput.class)); + } + + public void testOpenInput() throws IOException { + populateMetadata(); + remoteSegmentStoreDirectory.init(); + Path basePath = mock(Path.class); + Path resolvedPathInCache = mock(Path.class); + Path resolvedPathNotInCache = mock(Path.class); + when(basePath.resolve("_0.si")).thenReturn(resolvedPathInCache); + when(basePath.resolve("_0.cfs")).thenReturn(resolvedPathNotInCache); + when(localDirectory.getDirectory()).thenReturn(basePath); + CachedIndexInput cachedIndexInput = mock(CachedIndexInput.class); + IndexInput localIndexInput = mock(IndexInput.class); + IndexInput indexInput = mock(IndexInput.class); + when(fileCache.get(resolvedPathInCache)).thenReturn(cachedIndexInput); + when(fileCache.compute(eq(resolvedPathInCache), any())).thenReturn(cachedIndexInput); + when(cachedIndexInput.getIndexInput()).thenReturn(indexInput); + when(indexInput.clone()).thenReturn(indexInput); + when(fileCache.get(resolvedPathNotInCache)).thenReturn(null); + + // Temp file, read directly form local directory + when(localDirectory.openInput("_0.tmp", IOContext.DEFAULT)).thenReturn(localIndexInput); + assertEquals(compositeDirectory.openInput("_0.tmp", IOContext.DEFAULT), localIndexInput); + verify(localDirectory).openInput("_0.tmp", IOContext.DEFAULT); + + // File present in file cache + assertEquals(compositeDirectory.openInput("_0.si", IOContext.DEFAULT), indexInput); + + // File present in Remote + IndexInput indexInput1 = compositeDirectory.openInput("_0.cfs", IOContext.DEFAULT); + assert indexInput1 instanceof OnDemandBlockSnapshotIndexInput; + } + + public void testClose() throws IOException { + Path basePath = mock(Path.class); + Path resolvedPath1 = mock(Path.class); + Path resolvedPath2 = mock(Path.class); + when(basePath.resolve("_0.si")).thenReturn(resolvedPath1); + when(basePath.resolve("_0.cfs")).thenReturn(resolvedPath2); + when(localDirectory.getDirectory()).thenReturn(basePath); + when(localDirectory.listAll()).thenReturn(new String[] { "_0.si", "_0.cfs" }); + compositeDirectory.close(); + verify(localDirectory).close(); + verify(fileCache).remove(resolvedPath1); + verify(fileCache).remove(resolvedPath2); + } + + public void testGetPendingDeletions() throws IOException { + Set pendingDeletions = new HashSet<>(Arrays.asList("_0.si", "_0.cfs", "_0.cfe")); + when(localDirectory.getPendingDeletions()).thenReturn(pendingDeletions); + assertEquals(pendingDeletions, compositeDirectory.getPendingDeletions()); + } +} diff --git a/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java b/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java index b1e2028d761f0..c738156411dd7 100644 --- a/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java @@ -10,7 +10,6 @@ import org.apache.logging.log4j.Level; import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; import org.apache.lucene.codecs.CodecUtil; import org.apache.lucene.index.CorruptIndexException; import org.apache.lucene.index.IndexFormatTooNewException; @@ -23,34 +22,25 @@ import org.apache.lucene.store.OutputStreamIndexOutput; import org.apache.lucene.tests.util.LuceneTestCase; import org.apache.lucene.util.Version; -import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.common.UUIDs; import org.opensearch.common.blobstore.AsyncMultiStreamBlobContainer; import org.opensearch.common.blobstore.stream.write.WriteContext; import org.opensearch.common.io.VersionedCodecStreamWrapper; import org.opensearch.common.io.stream.BytesStreamOutput; import org.opensearch.common.lucene.store.ByteArrayIndexInput; -import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.concurrent.OpenSearchExecutors; import org.opensearch.core.action.ActionListener; import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.index.Index; import org.opensearch.core.index.shard.ShardId; -import org.opensearch.index.engine.NRTReplicationEngineFactory; import org.opensearch.index.remote.RemoteStoreEnums.PathHashAlgorithm; import org.opensearch.index.remote.RemoteStoreEnums.PathType; import org.opensearch.index.remote.RemoteStorePathStrategy; import org.opensearch.index.remote.RemoteStoreUtils; -import org.opensearch.index.shard.IndexShard; -import org.opensearch.index.shard.IndexShardTestCase; -import org.opensearch.index.store.lockmanager.RemoteStoreMetadataLockManager; import org.opensearch.index.store.remote.metadata.RemoteSegmentMetadata; import org.opensearch.index.store.remote.metadata.RemoteSegmentMetadataHandler; -import org.opensearch.indices.replication.common.ReplicationType; import org.opensearch.test.MockLogAppender; import org.opensearch.test.junit.annotations.TestLogging; import org.opensearch.threadpool.ThreadPool; -import org.junit.After; import org.junit.Before; import java.io.ByteArrayInputStream; @@ -64,7 +54,6 @@ import java.util.Map; import java.util.Set; import java.util.concurrent.CountDownLatch; -import java.util.concurrent.ExecutorService; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; @@ -87,95 +76,11 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -public class RemoteSegmentStoreDirectoryTests extends IndexShardTestCase { - private static final Logger logger = LogManager.getLogger(RemoteSegmentStoreDirectoryTests.class); - private RemoteDirectory remoteDataDirectory; - private RemoteDirectory remoteMetadataDirectory; - private RemoteStoreMetadataLockManager mdLockManager; - - private RemoteSegmentStoreDirectory remoteSegmentStoreDirectory; - private TestUploadListener testUploadTracker; - private IndexShard indexShard; - private SegmentInfos segmentInfos; - private ThreadPool threadPool; - - private final String metadataFilename = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename( - 12, - 23, - 34, - 1, - 1, - "node-1" - ); - - private final String metadataFilenameDup = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename( - 12, - 23, - 34, - 2, - 1, - "node-2" - ); - private final String metadataFilename2 = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename( - 12, - 13, - 34, - 1, - 1, - "node-1" - ); - private final String metadataFilename3 = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename( - 10, - 38, - 34, - 1, - 1, - "node-1" - ); - private final String metadataFilename4 = RemoteSegmentStoreDirectory.MetadataFilenameUtils.getMetadataFilename( - 10, - 36, - 34, - 1, - 1, - "node-1" - ); +public class RemoteSegmentStoreDirectoryTests extends BaseRemoteSegmentStoreDirectoryTests { @Before public void setup() throws IOException { - remoteDataDirectory = mock(RemoteDirectory.class); - remoteMetadataDirectory = mock(RemoteDirectory.class); - mdLockManager = mock(RemoteStoreMetadataLockManager.class); - threadPool = mock(ThreadPool.class); - testUploadTracker = new TestUploadListener(); - - Settings indexSettings = Settings.builder() - .put(IndexMetadata.SETTING_VERSION_CREATED, org.opensearch.Version.CURRENT) - .put(IndexMetadata.SETTING_REPLICATION_TYPE, ReplicationType.SEGMENT) - .build(); - ExecutorService executorService = OpenSearchExecutors.newDirectExecutorService(); - - indexShard = newStartedShard(false, indexSettings, new NRTReplicationEngineFactory()); - remoteSegmentStoreDirectory = new RemoteSegmentStoreDirectory( - remoteDataDirectory, - remoteMetadataDirectory, - mdLockManager, - threadPool, - indexShard.shardId() - ); - try (Store store = indexShard.store()) { - segmentInfos = store.readLastCommittedSegmentsInfo(); - } - - when(threadPool.executor(ThreadPool.Names.REMOTE_PURGE)).thenReturn(executorService); - when(threadPool.executor(ThreadPool.Names.REMOTE_RECOVERY)).thenReturn(executorService); - when(threadPool.executor(ThreadPool.Names.SAME)).thenReturn(executorService); - } - - @After - public void tearDown() throws Exception { - indexShard.close("test tearDown", true, false); - super.tearDown(); + setupRemoteSegmentStoreDirectory(); } public void testUploadedSegmentMetadataToString() { @@ -256,60 +161,6 @@ public void testInitMultipleMetadataFile() throws IOException { assertThrows(IllegalStateException.class, () -> remoteSegmentStoreDirectory.init()); } - private Map> populateMetadata() throws IOException { - List metadataFiles = new ArrayList<>(); - - metadataFiles.add(metadataFilename); - metadataFiles.add(metadataFilename2); - metadataFiles.add(metadataFilename3); - - when( - remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( - RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX, - METADATA_FILES_TO_FETCH - ) - ).thenReturn(List.of(metadataFilename)); - when( - remoteMetadataDirectory.listFilesByPrefixInLexicographicOrder( - RemoteSegmentStoreDirectory.MetadataFilenameUtils.METADATA_PREFIX, - Integer.MAX_VALUE - ) - ).thenReturn(metadataFiles); - - Map> metadataFilenameContentMapping = Map.of( - metadataFilename, - getDummyMetadata("_0", 1), - metadataFilename2, - getDummyMetadata("_0", 1), - metadataFilename3, - getDummyMetadata("_0", 1) - ); - - when(remoteMetadataDirectory.getBlobStream(metadataFilename)).thenAnswer( - I -> createMetadataFileBytes( - metadataFilenameContentMapping.get(metadataFilename), - indexShard.getLatestReplicationCheckpoint(), - segmentInfos - ) - ); - when(remoteMetadataDirectory.getBlobStream(metadataFilename2)).thenAnswer( - I -> createMetadataFileBytes( - metadataFilenameContentMapping.get(metadataFilename2), - indexShard.getLatestReplicationCheckpoint(), - segmentInfos - ) - ); - when(remoteMetadataDirectory.getBlobStream(metadataFilename3)).thenAnswer( - I -> createMetadataFileBytes( - metadataFilenameContentMapping.get(metadataFilename3), - indexShard.getLatestReplicationCheckpoint(), - segmentInfos - ) - ); - - return metadataFilenameContentMapping; - } - public void testInit() throws IOException { populateMetadata(); diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java index 8bfd88d0a8e56..95a343f3b4025 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java @@ -2079,8 +2079,7 @@ public void onFailure(final Exception e) { new RemoteStoreStatsTrackerFactory(clusterService, settings), DefaultRecoverySettings.INSTANCE, new CacheModule(new ArrayList<>(), settings).getCacheService(), - DefaultRemoteStoreSettings.INSTANCE, - null + DefaultRemoteStoreSettings.INSTANCE ); final RecoverySettings recoverySettings = new RecoverySettings(settings, clusterSettings); snapshotShardsService = new SnapshotShardsService( diff --git a/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java b/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java index a9f6fdc86155d..603c063a3da47 100644 --- a/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java @@ -2077,6 +2077,10 @@ protected boolean addMockTransportService() { return true; } + protected boolean addMockIndexStorePlugin() { + return true; + } + /** Returns {@code true} iff this test cluster should use a dummy http transport */ protected boolean addMockHttpTransport() { return true; @@ -2119,7 +2123,7 @@ protected Collection> getMockPlugins() { if (randomBoolean() && addMockTransportService()) { mocks.add(MockTransportService.TestPlugin.class); } - if (randomBoolean()) { + if (randomBoolean() && addMockIndexStorePlugin()) { mocks.add(MockFSIndexStore.TestPlugin.class); } if (randomBoolean()) { From 3280a6db5e2e0eb15cb6ffadcae6e561f1c33fbe Mon Sep 17 00:00:00 2001 From: Shreyansh Ray Date: Thu, 9 May 2024 20:18:07 +0530 Subject: [PATCH 10/16] Add experimental annotations for newly created classes and review comment fixes Signed-off-by: Shreyansh Ray --- CHANGELOG.md | 1 + .../store/CloseableFilterIndexOutput.java | 10 ++++--- .../index/store/CompositeDirectory.java | 23 +++++++-------- .../filecache/FileCachedIndexInput.java | 5 ++-- ...put.java => FullFileCachedIndexInput.java} | 23 +++++++++++---- .../store/remote/utils/BlockIOContext.java | 29 +++++++------------ .../index/store/remote/utils/FileType.java | 19 +++++++++--- .../index/store/CompositeDirectoryTests.java | 4 +-- 8 files changed, 64 insertions(+), 50 deletions(-) rename server/src/main/java/org/opensearch/index/store/remote/filecache/{NonBlockCachedIndexInput.java => FullFileCachedIndexInput.java} (61%) diff --git a/CHANGELOG.md b/CHANGELOG.md index db44887a0e59f..cfbdf0f2bb5c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ## [Unreleased 2.x] ### Added - Add useCompoundFile index setting ([#13478](https://github.com/opensearch-project/OpenSearch/pull/13478)) +- Add composite directory implementation and integrate it with FileCache ([12782](https://github.com/opensearch-project/OpenSearch/pull/12782)) ### Dependencies - Bump `com.github.spullara.mustache.java:compiler` from 0.9.10 to 0.9.13 ([#13329](https://github.com/opensearch-project/OpenSearch/pull/13329), [#13559](https://github.com/opensearch-project/OpenSearch/pull/13559)) diff --git a/server/src/main/java/org/opensearch/index/store/CloseableFilterIndexOutput.java b/server/src/main/java/org/opensearch/index/store/CloseableFilterIndexOutput.java index 8df5d648b91f3..3a4309fe6ee6d 100644 --- a/server/src/main/java/org/opensearch/index/store/CloseableFilterIndexOutput.java +++ b/server/src/main/java/org/opensearch/index/store/CloseableFilterIndexOutput.java @@ -9,6 +9,7 @@ package org.opensearch.index.store; import org.apache.lucene.store.IndexOutput; +import org.opensearch.common.annotation.ExperimentalApi; import org.opensearch.common.lucene.store.FilterIndexOutput; import java.io.IOException; @@ -16,8 +17,9 @@ /** * FilterIndexOutput which takes in an additional FunctionalInterface as a parameter to perform required operations once the IndexOutput is closed * - * @opensearch.internal + * @opensearch.experimental */ +@ExperimentalApi public class CloseableFilterIndexOutput extends FilterIndexOutput { /** @@ -25,11 +27,11 @@ public class CloseableFilterIndexOutput extends FilterIndexOutput { */ @FunctionalInterface public interface OnCloseListener { - void onClose(String name); + void onClose(String name) throws IOException; } - OnCloseListener onCloseListener; - String fileName; + private final OnCloseListener onCloseListener; + private final String fileName; public CloseableFilterIndexOutput(IndexOutput out, String fileName, OnCloseListener onCloseListener) { super("CloseableFilterIndexOutput for file " + fileName, out); diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java index 5536be1212ccb..f66120c5753f7 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java @@ -22,7 +22,7 @@ import org.opensearch.index.store.remote.file.OnDemandBlockSnapshotIndexInput; import org.opensearch.index.store.remote.filecache.CachedIndexInput; import org.opensearch.index.store.remote.filecache.FileCache; -import org.opensearch.index.store.remote.filecache.NonBlockCachedIndexInput; +import org.opensearch.index.store.remote.filecache.FullFileCachedIndexInput; import org.opensearch.index.store.remote.metadata.RemoteSegmentMetadata; import org.opensearch.index.store.remote.utils.BlockIOContext; import org.opensearch.index.store.remote.utils.FileType; @@ -45,7 +45,7 @@ * All such abstractions will be handled by the Composite directory itself * Implements all required methods by Directory abstraction * - * @opensearch.internal + * @opensearch.experimental */ @ExperimentalApi public class CompositeDirectory extends FilterDirectory { @@ -143,9 +143,12 @@ public long fileLength(String name) throws IOException { long fileLength; Path key = localDirectory.getDirectory().resolve(name); if (isTempFile(name) || fileCache.get(key) != null) { - fileLength = localDirectory.fileLength(name); - fileCache.decRef(key); - logger.trace("fileLength from Local {}", fileLength); + try { + fileLength = localDirectory.fileLength(name); + logger.trace("fileLength from Local {}", fileLength); + } finally { + fileCache.decRef(key); + } } else { fileLength = remoteDirectory.fileLength(name); logger.trace("fileLength from Remote {}", fileLength); @@ -170,13 +173,7 @@ public IndexOutput createOutput(String name, IOContext context) throws IOExcepti /* * The CloseableFilterIndexOutput will ensure that the file is added to FileCache once write is completed on this file */ - return new CloseableFilterIndexOutput(localDirectory.createOutput(name, context), name, (fileName) -> { - try { - cacheFile(fileName); - } catch (IOException e) { - throw new RuntimeException(e); - } - }); + return new CloseableFilterIndexOutput(localDirectory.createOutput(name, context), name, this::cacheFile); } finally { writeLock.unlock(); } @@ -393,7 +390,7 @@ private String[] getRemoteFiles() throws IOException { private void cacheFile(String name) throws IOException { Path filePath = localDirectory.getDirectory().resolve(name); - fileCache.put(filePath, new NonBlockCachedIndexInput(localDirectory.openInput(name, IOContext.READ))); + fileCache.put(filePath, new FullFileCachedIndexInput(fileCache, filePath, localDirectory.openInput(name, IOContext.READ))); // Decrementing ref here as above put call increments the ref of the key fileCache.decRef(filePath); // TODO : Pin the above filePath in the file cache once pinning support is added so that it cannot be evicted unless it has been diff --git a/server/src/main/java/org/opensearch/index/store/remote/filecache/FileCachedIndexInput.java b/server/src/main/java/org/opensearch/index/store/remote/filecache/FileCachedIndexInput.java index 7d7c40be3a833..200a47e661ab4 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/filecache/FileCachedIndexInput.java +++ b/server/src/main/java/org/opensearch/index/store/remote/filecache/FileCachedIndexInput.java @@ -133,8 +133,9 @@ public FileCachedIndexInput clone() { @Override public IndexInput slice(String sliceDescription, long offset, long length) throws IOException { - // never reach here! - throw new UnsupportedOperationException("FileCachedIndexInput couldn't be sliced."); + IndexInput slicedIndexInput = luceneIndexInput.slice(sliceDescription, offset, length); + cache.incRef(filePath); + return new FileCachedIndexInput(cache, filePath, slicedIndexInput, true); } @Override diff --git a/server/src/main/java/org/opensearch/index/store/remote/filecache/NonBlockCachedIndexInput.java b/server/src/main/java/org/opensearch/index/store/remote/filecache/FullFileCachedIndexInput.java similarity index 61% rename from server/src/main/java/org/opensearch/index/store/remote/filecache/NonBlockCachedIndexInput.java rename to server/src/main/java/org/opensearch/index/store/remote/filecache/FullFileCachedIndexInput.java index ee520dc22964e..7b1163b18727e 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/filecache/NonBlockCachedIndexInput.java +++ b/server/src/main/java/org/opensearch/index/store/remote/filecache/FullFileCachedIndexInput.java @@ -8,24 +8,35 @@ package org.opensearch.index.store.remote.filecache; +import org.apache.lucene.store.AlreadyClosedException; import org.apache.lucene.store.IndexInput; +import org.opensearch.common.annotation.ExperimentalApi; -import java.io.IOException; +import java.nio.file.Path; import java.util.concurrent.atomic.AtomicBoolean; /** * Implementation of the CachedIndexInput for NON_BLOCK files which takes in an IndexInput as parameter + * + * @opensearch.experimental */ -public class NonBlockCachedIndexInput implements CachedIndexInput { +@ExperimentalApi +public class FullFileCachedIndexInput implements CachedIndexInput { private final IndexInput indexInput; + private final FileCache fileCache; + private final Path path; + private final FileCachedIndexInput fileCachedIndexInput; private final AtomicBoolean isClosed; /** * Constructor - takes IndexInput as parameter */ - public NonBlockCachedIndexInput(IndexInput indexInput) { + public FullFileCachedIndexInput(FileCache fileCache, Path path, IndexInput indexInput) { + this.fileCache = fileCache; + this.path = path; this.indexInput = indexInput; + fileCachedIndexInput = new FileCachedIndexInput(fileCache, path, indexInput); isClosed = new AtomicBoolean(false); } @@ -33,8 +44,9 @@ public NonBlockCachedIndexInput(IndexInput indexInput) { * Returns the wrapped indexInput */ @Override - public IndexInput getIndexInput() throws IOException { - return indexInput; + public IndexInput getIndexInput() { + if (isClosed.get()) throw new AlreadyClosedException("Index input is already closed"); + return fileCachedIndexInput; } /** @@ -60,7 +72,6 @@ public boolean isClosed() { public void close() throws Exception { if (!isClosed.getAndSet(true)) { indexInput.close(); - isClosed.set(true); } } } diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/BlockIOContext.java b/server/src/main/java/org/opensearch/index/store/remote/utils/BlockIOContext.java index da94e4a46d307..a78dd85d6f194 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/utils/BlockIOContext.java +++ b/server/src/main/java/org/opensearch/index/store/remote/utils/BlockIOContext.java @@ -9,43 +9,29 @@ package org.opensearch.index.store.remote.utils; import org.apache.lucene.store.IOContext; +import org.opensearch.common.annotation.ExperimentalApi; /** * BlockIOContext is an extension of IOContext which can be used to pass block related information to the openInput() method of any directory + * + * @opensearch.experimental */ +@ExperimentalApi public class BlockIOContext extends IOContext { - private final boolean isBlockRequest; private long blockStart; private long blockSize; - /** - * Default constructor - */ - BlockIOContext(IOContext ctx) { - super(ctx.context); - this.isBlockRequest = false; - this.blockStart = -1; - this.blockSize = -1; - } - /** * Constructor to initialise BlockIOContext with block related information */ public BlockIOContext(IOContext ctx, long blockStart, long blockSize) { super(ctx.context); - this.isBlockRequest = true; + verifyBlockStartAndSize(blockStart, blockSize); this.blockStart = blockStart; this.blockSize = blockSize; } - /** - * Function to check if the Context contains a block request or not - */ - public boolean isBlockRequest() { - return isBlockRequest; - } - /** * Getter for blockStart */ @@ -59,4 +45,9 @@ public long getBlockStart() { public long getBlockSize() { return blockSize; } + + private void verifyBlockStartAndSize(long blockStart, long blockSize) { + if (blockStart < 0) throw new IllegalArgumentException("blockStart must be greater than or equal to 0"); + if (blockSize <= 0) throw new IllegalArgumentException(("blockSize must be greater than 0")); + } } diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/FileType.java b/server/src/main/java/org/opensearch/index/store/remote/utils/FileType.java index 418f8a24a5f24..e340c82ba9ba3 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/utils/FileType.java +++ b/server/src/main/java/org/opensearch/index/store/remote/utils/FileType.java @@ -8,18 +8,29 @@ package org.opensearch.index.store.remote.utils; +import org.opensearch.common.annotation.ExperimentalApi; + /** * Enum to represent whether a file is block or not + * + * @opensearch.experimental */ +@ExperimentalApi public enum FileType { /** * Block file */ - BLOCK, + BLOCK(".*_block_.*"), /** - * Non block file + * Full file - Non-Block */ - NON_BLOCK; + FULL(".*"); + + private final String pattern; + + FileType(String pattern) { + this.pattern = pattern; + } /** * Returns if the fileType is a block file or not @@ -32,7 +43,7 @@ public static boolean isBlockFile(FileType fileType) { * Returns if the fileName is block file or not */ public static boolean isBlockFile(String fileName) { - if (fileName.contains("_block_")) return true; + if (fileName.matches(FileType.BLOCK.pattern)) return true; return false; } } diff --git a/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java b/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java index 56d12537bf679..16dd3ac7a68f7 100644 --- a/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java @@ -18,7 +18,7 @@ import org.opensearch.index.store.remote.file.OnDemandBlockSnapshotIndexInput; import org.opensearch.index.store.remote.filecache.CachedIndexInput; import org.opensearch.index.store.remote.filecache.FileCache; -import org.opensearch.index.store.remote.filecache.NonBlockCachedIndexInput; +import org.opensearch.index.store.remote.filecache.FullFileCachedIndexInput; import org.junit.Before; import java.io.IOException; @@ -133,7 +133,7 @@ public void testRename() throws IOException { compositeDirectory.rename("old_file_name", "new_file_name"); verify(localDirectory).rename("old_file_name", "new_file_name"); verify(fileCache).remove(resolvedPathOldFile); - verify(fileCache).put(eq(resolvedPathNewFile), any(NonBlockCachedIndexInput.class)); + verify(fileCache).put(eq(resolvedPathNewFile), any(FullFileCachedIndexInput.class)); } public void testOpenInput() throws IOException { From 4ee744d9103c7764c213da3bf85983c1964168ff Mon Sep 17 00:00:00 2001 From: Shreyansh Ray Date: Thu, 9 May 2024 23:39:55 +0530 Subject: [PATCH 11/16] Use ref count as a temporary measure to prevent file from eviction until uploaded to Remote Signed-off-by: Shreyansh Ray --- .../opensearch/index/store/CompositeDirectory.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java index f66120c5753f7..758a1f274820c 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java @@ -352,11 +352,13 @@ public void afterSyncToRemote(Collection files) throws IOException { for (String fileName : files) { writeLock.lock(); try { - /** + /* Decrementing the refCount here for the path so that it becomes eligible for eviction + * This is a temporary solution until pinning support is added * TODO - Unpin the files here from FileCache so that they become eligible for eviction, once pinning/unpinning support is added in FileCache * Uncomment the below commented line(to remove the file from cache once uploaded) to test block based functionality */ logger.trace("File {} uploaded to Remote Store and now can be eligible for eviction in FileCache", fileName); + fileCache.decRef(localDirectory.getDirectory().resolve(fileName)); // fileCache.remove(localDirectory.getDirectory().resolve(fileName)); } finally { writeLock.unlock(); @@ -390,11 +392,12 @@ private String[] getRemoteFiles() throws IOException { private void cacheFile(String name) throws IOException { Path filePath = localDirectory.getDirectory().resolve(name); - fileCache.put(filePath, new FullFileCachedIndexInput(fileCache, filePath, localDirectory.openInput(name, IOContext.READ))); - // Decrementing ref here as above put call increments the ref of the key - fileCache.decRef(filePath); + // put will increase the refCount for the path, making sure it is not evicted, wil decrease the ref after it is uploaded to Remote + // so that it can be evicted after that + // this is just a temporary solution, will pin the file once support for that is added in FileCache // TODO : Pin the above filePath in the file cache once pinning support is added so that it cannot be evicted unless it has been // successfully uploaded to Remote + fileCache.put(filePath, new FullFileCachedIndexInput(fileCache, filePath, localDirectory.openInput(name, IOContext.READ))); } } From 3292b36690ec9c619bee734b42e8e43961b3ce34 Mon Sep 17 00:00:00 2001 From: Shreyansh Ray Date: Fri, 10 May 2024 15:56:58 +0530 Subject: [PATCH 12/16] Remove method level locks Signed-off-by: Shreyansh Ray --- .../index/store/CompositeDirectory.java | 279 ++++++------------ .../index/store/CompositeDirectoryTests.java | 21 -- 2 files changed, 84 insertions(+), 216 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java index 758a1f274820c..583a181c4797a 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java @@ -36,7 +36,6 @@ import java.util.Collection; import java.util.HashSet; import java.util.Set; -import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.stream.Collectors; /** @@ -54,9 +53,6 @@ public class CompositeDirectory extends FilterDirectory { private final RemoteSegmentStoreDirectory remoteDirectory; private final FileCache fileCache; private final TransferManager transferManager; - private final ReentrantReadWriteLock readWriteLock = new ReentrantReadWriteLock(); - private final ReentrantReadWriteLock.WriteLock writeLock = readWriteLock.writeLock(); - private final ReentrantReadWriteLock.ReadLock readLock = readWriteLock.readLock(); /** * Constructor to initialise the composite directory @@ -87,25 +83,20 @@ public CompositeDirectory(FSDirectory localDirectory, RemoteSegmentStoreDirector @Override public String[] listAll() throws IOException { logger.trace("listAll() called"); - readLock.lock(); - try { - String[] localFiles = localDirectory.listAll(); - logger.trace("Local Directory files : {}", () -> Arrays.toString(localFiles)); - Set allFiles = new HashSet<>(Arrays.asList(localFiles)); - String[] remoteFiles = getRemoteFiles(); - allFiles.addAll(Arrays.asList(remoteFiles)); - logger.trace("Remote Directory files : {}", () -> Arrays.toString(remoteFiles)); - Set localLuceneFiles = allFiles.stream() - .filter(file -> !FileType.isBlockFile(file)) - .collect(Collectors.toUnmodifiableSet()); - String[] files = new String[localLuceneFiles.size()]; - localLuceneFiles.toArray(files); - Arrays.sort(files); - logger.trace("listAll() returns : {}", () -> Arrays.toString(files)); - return files; - } finally { - readLock.unlock(); - } + String[] localFiles = localDirectory.listAll(); + logger.trace("Local Directory files : {}", () -> Arrays.toString(localFiles)); + Set allFiles = new HashSet<>(Arrays.asList(localFiles)); + String[] remoteFiles = getRemoteFiles(); + allFiles.addAll(Arrays.asList(remoteFiles)); + logger.trace("Remote Directory files : {}", () -> Arrays.toString(remoteFiles)); + Set localLuceneFiles = allFiles.stream() + .filter(file -> !FileType.isBlockFile(file)) + .collect(Collectors.toUnmodifiableSet()); + String[] files = new String[localLuceneFiles.size()]; + localLuceneFiles.toArray(files); + Arrays.sort(files); + logger.trace("listAll() returns : {}", () -> Arrays.toString(files)); + return files; } /** @@ -117,16 +108,11 @@ public String[] listAll() throws IOException { @Override public void deleteFile(String name) throws IOException { logger.trace("deleteFile() called {}", name); - writeLock.lock(); - try { - /* - Not deleting from localDirectory directly since it causes a race condition when the localDirectory deletes a file, and it ends up in pendingDeletion state. - Meanwhile, fileCache on removal deletes the file directly via the Files class and later when the directory tries to delete the files pending for deletion (which happens before creating a new file), it causes NoSuchFileException and new file creation fails - */ - fileCache.remove(localDirectory.getDirectory().resolve(name)); - } finally { - writeLock.unlock(); - } + /* + Not deleting from localDirectory directly since it causes a race condition when the localDirectory deletes a file, and it ends up in pendingDeletion state. + Meanwhile, fileCache on removal deletes the file directly via the Files class and later when the directory tries to delete the files pending for deletion (which happens before creating a new file), it causes NoSuchFileException and new file creation fails + */ + fileCache.remove(localDirectory.getDirectory().resolve(name)); } /** @@ -138,25 +124,20 @@ Meanwhile, fileCache on removal deletes the file directly via the Files class an @Override public long fileLength(String name) throws IOException { logger.trace("fileLength() called {}", name); - readLock.lock(); - try { - long fileLength; - Path key = localDirectory.getDirectory().resolve(name); - if (isTempFile(name) || fileCache.get(key) != null) { - try { - fileLength = localDirectory.fileLength(name); - logger.trace("fileLength from Local {}", fileLength); - } finally { - fileCache.decRef(key); - } - } else { - fileLength = remoteDirectory.fileLength(name); - logger.trace("fileLength from Remote {}", fileLength); + long fileLength; + Path key = localDirectory.getDirectory().resolve(name); + if (isTempFile(name) || fileCache.get(key) != null) { + try { + fileLength = localDirectory.fileLength(name); + logger.trace("fileLength from Local {}", fileLength); + } finally { + fileCache.decRef(key); } - return fileLength; - } finally { - readLock.unlock(); + } else { + fileLength = remoteDirectory.fileLength(name); + logger.trace("fileLength from Remote {}", fileLength); } + return fileLength; } /** @@ -168,33 +149,8 @@ public long fileLength(String name) throws IOException { @Override public IndexOutput createOutput(String name, IOContext context) throws IOException { logger.trace("createOutput() called {}", name); - writeLock.lock(); - try { - /* - * The CloseableFilterIndexOutput will ensure that the file is added to FileCache once write is completed on this file - */ - return new CloseableFilterIndexOutput(localDirectory.createOutput(name, context), name, this::cacheFile); - } finally { - writeLock.unlock(); - } - } - - /** - * Creates a new, empty, temporary file in the directory and returns an {@link IndexOutput} - * instance for appending data to this file. - * - *

    The temporary file name (accessible via {@link IndexOutput#getName()}) will start with - * {@code prefix}, end with {@code suffix} and have a reserved file extension {@code .tmp}. - */ - @Override - public IndexOutput createTempOutput(String prefix, String suffix, IOContext context) throws IOException { - logger.trace("createTempOutput() called {} , {}", prefix, suffix); - writeLock.lock(); - try { - return localDirectory.createTempOutput(prefix, suffix, context); - } finally { - writeLock.unlock(); - } + // The CloseableFilterIndexOutput will ensure that the file is added to FileCache once write is completed on this file + return new CloseableFilterIndexOutput(localDirectory.createOutput(name, context), name, this::cacheFile); } /** @@ -204,32 +160,10 @@ public IndexOutput createTempOutput(String prefix, String suffix, IOContext cont @Override public void sync(Collection names) throws IOException { logger.trace("sync() called {}", names); - writeLock.lock(); - try { - Collection remoteFiles = Arrays.asList(getRemoteFiles()); - Collection filesToSync = names.stream() - .filter(name -> remoteFiles.contains(name) == false) - .collect(Collectors.toList()); - logger.trace("Synced files : {}", filesToSync); - localDirectory.sync(filesToSync); - } finally { - writeLock.unlock(); - } - } - - /** - * Ensures that directory metadata, such as recent file renames, are moved to stable storage. - * @throws IOException in case of I/O error - */ - @Override - public void syncMetaData() throws IOException { - logger.trace("syncMetaData() called "); - writeLock.lock(); - try { - localDirectory.syncMetaData(); - } finally { - writeLock.unlock(); - } + Collection remoteFiles = Arrays.asList(getRemoteFiles()); + Collection filesToSync = names.stream().filter(name -> remoteFiles.contains(name) == false).collect(Collectors.toList()); + logger.trace("Synced files : {}", filesToSync); + localDirectory.sync(filesToSync); } /** @@ -240,14 +174,9 @@ public void syncMetaData() throws IOException { @Override public void rename(String source, String dest) throws IOException { logger.trace("rename() called {}, {}", source, dest); - writeLock.lock(); - try { - localDirectory.rename(source, dest); - fileCache.remove(localDirectory.getDirectory().resolve(source)); - cacheFile(dest); - } finally { - writeLock.unlock(); - } + localDirectory.rename(source, dest); + fileCache.remove(localDirectory.getDirectory().resolve(source)); + cacheFile(dest); } /** @@ -259,48 +188,35 @@ public void rename(String source, String dest) throws IOException { @Override public IndexInput openInput(String name, IOContext context) throws IOException { logger.trace("openInput() called {}", name); - writeLock.lock(); - try { - /* - * We aren't tracking temporary files (created via createTempOutput) currently in FileCache as these are created and then deleted within a very short span of time - * We will be reading them directory from the local directory - */ - if (isTempFile(name)) { - return localDirectory.openInput(name, context); - } - /* - * Return directly from the FileCache (via TransferManager) if complete file is present - */ - - Path key = localDirectory.getDirectory().resolve(name); - CachedIndexInput indexInput = fileCache.get(key); - if (indexInput != null) { - logger.trace("Complete file found in FileCache"); - try { - return indexInput.getIndexInput().clone(); - } finally { - fileCache.decRef(key); - } - } - /* - * If file has been uploaded to the Remote Store, fetch it from the Remote Store in blocks via OnDemandCompositeBlockIndexInput - */ - else { - logger.trace("Complete file not in FileCache, to be fetched in Blocks from Remote"); - RemoteSegmentMetadata remoteSegmentMetadata = remoteDirectory.readLatestMetadataFile(); - RemoteSegmentStoreDirectory.UploadedSegmentMetadata uploadedSegmentMetadata = remoteSegmentMetadata.getMetadata().get(name); - /* - * TODO : Refactor FileInfo and OnDemandBlockSnapshotIndexInput to more generic names as they are not Remote Snapshot specific - */ - BlobStoreIndexShardSnapshot.FileInfo fileInfo = new BlobStoreIndexShardSnapshot.FileInfo( - name, - new StoreFileMetadata(name, uploadedSegmentMetadata.getLength(), uploadedSegmentMetadata.getChecksum(), Version.LATEST), - null - ); - return new OnDemandBlockSnapshotIndexInput(fileInfo, localDirectory, transferManager); + // We aren't tracking temporary files (created via createTempOutput) currently in FileCache as these are created and then deleted + // within a very short span of time + // We will be reading them directory from the local directory + if (isTempFile(name)) { + return localDirectory.openInput(name, context); + } + // Return directly from the FileCache (via TransferManager) if complete file is present + Path key = localDirectory.getDirectory().resolve(name); + CachedIndexInput indexInput = fileCache.get(key); + if (indexInput != null) { + logger.trace("Complete file found in FileCache"); + try { + return indexInput.getIndexInput().clone(); + } finally { + fileCache.decRef(key); } - } finally { - writeLock.unlock(); + } + // If file has been uploaded to the Remote Store, fetch it from the Remote Store in blocks via OnDemandCompositeBlockIndexInput + else { + logger.trace("Complete file not in FileCache, to be fetched in Blocks from Remote"); + RemoteSegmentMetadata remoteSegmentMetadata = remoteDirectory.readLatestMetadataFile(); + RemoteSegmentStoreDirectory.UploadedSegmentMetadata uploadedSegmentMetadata = remoteSegmentMetadata.getMetadata().get(name); + // TODO : Refactor FileInfo and OnDemandBlockSnapshotIndexInput to more generic names as they are not Remote Snapshot specific + BlobStoreIndexShardSnapshot.FileInfo fileInfo = new BlobStoreIndexShardSnapshot.FileInfo( + name, + new StoreFileMetadata(name, uploadedSegmentMetadata.getLength(), uploadedSegmentMetadata.getChecksum(), Version.LATEST), + null + ); + return new OnDemandBlockSnapshotIndexInput(fileInfo, localDirectory, transferManager); } } @@ -310,31 +226,9 @@ public IndexInput openInput(String name, IOContext context) throws IOException { */ @Override public void close() throws IOException { - writeLock.lock(); - try { - Arrays.stream(localDirectory.listAll()).forEach(f -> { - logger.trace("Removing file from cache {}", f); - fileCache.remove(localDirectory.getDirectory().resolve(f)); - }); - localDirectory.close(); - remoteDirectory.close(); - } finally { - writeLock.unlock(); - } - } - - /** - * Returns a set of files currently pending deletion in this directory. - * @throws IOException in case of I/O error - */ - @Override - public Set getPendingDeletions() throws IOException { - writeLock.lock(); - try { - return localDirectory.getPendingDeletions(); - } finally { - writeLock.unlock(); - } + Arrays.stream(localDirectory.listAll()).forEach(f -> fileCache.remove(localDirectory.getDirectory().resolve(f))); + localDirectory.close(); + remoteDirectory.close(); } /** @@ -350,19 +244,15 @@ public void afterSyncToRemote(Collection files) throws IOException { return; } for (String fileName : files) { - writeLock.lock(); - try { - /* Decrementing the refCount here for the path so that it becomes eligible for eviction - * This is a temporary solution until pinning support is added - * TODO - Unpin the files here from FileCache so that they become eligible for eviction, once pinning/unpinning support is added in FileCache - * Uncomment the below commented line(to remove the file from cache once uploaded) to test block based functionality - */ - logger.trace("File {} uploaded to Remote Store and now can be eligible for eviction in FileCache", fileName); - fileCache.decRef(localDirectory.getDirectory().resolve(fileName)); - // fileCache.remove(localDirectory.getDirectory().resolve(fileName)); - } finally { - writeLock.unlock(); - } + /* + Decrementing the refCount here for the path so that it becomes eligible for eviction + This is a temporary solution until pinning support is added + TODO - Unpin the files here from FileCache so that they become eligible for eviction, once pinning/unpinning support is added in FileCache + Uncomment the below commented line(to remove the file from cache once uploaded) to test block based functionality + */ + logger.trace("File {} uploaded to Remote Store and now can be eligible for eviction in FileCache", fileName); + fileCache.decRef(localDirectory.getDirectory().resolve(fileName)); + // fileCache.remove(localDirectory.getDirectory().resolve(fileName)); } } @@ -379,11 +269,10 @@ private String[] getRemoteFiles() throws IOException { remoteFiles = remoteDirectory.listAll(); } catch (NullPointerException e) { /* - * There are two scenarios where the listAll() call on remote directory returns NullPointerException: - * - When remote directory is not set - * - When init() of remote directory has not yet been called - * - * Returning an empty list in these scenarios + There are two scenarios where the listAll() call on remote directory returns NullPointerException: + - When remote directory is not set + - When init() of remote directory has not yet been called + Returning an empty list in the above scenarios */ remoteFiles = new String[0]; } diff --git a/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java b/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java index 16dd3ac7a68f7..771734ac23a9e 100644 --- a/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java @@ -23,12 +23,9 @@ import java.io.IOException; import java.nio.file.Path; -import java.util.Arrays; import java.util.Collection; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; @@ -101,13 +98,6 @@ public void testCreateOutput() throws IOException { verify(localDirectory).createOutput("_0.si", IOContext.DEFAULT); } - public void testCreateTempOutput() throws IOException { - IndexOutput indexOutput = mock(IndexOutput.class); - when(localDirectory.createTempOutput("prefix", "suffix", IOContext.DEFAULT)).thenReturn(indexOutput); - compositeDirectory.createTempOutput("prefix", "suffix", IOContext.DEFAULT); - verify(localDirectory).createTempOutput("prefix", "suffix", IOContext.DEFAULT); - } - public void testSync() throws IOException { populateMetadata(); remoteSegmentStoreDirectory.init(); @@ -116,11 +106,6 @@ public void testSync() throws IOException { verify(localDirectory).sync(List.of("_1.cfe", "_1.cfs", "_2.nvm")); } - public void testSyncMetaData() throws IOException { - compositeDirectory.syncMetaData(); - verify(localDirectory).syncMetaData(); - } - public void testRename() throws IOException { Path basePath = mock(Path.class); Path resolvedPathOldFile = mock(Path.class); @@ -180,10 +165,4 @@ public void testClose() throws IOException { verify(fileCache).remove(resolvedPath1); verify(fileCache).remove(resolvedPath2); } - - public void testGetPendingDeletions() throws IOException { - Set pendingDeletions = new HashSet<>(Arrays.asList("_0.si", "_0.cfs", "_0.cfe")); - when(localDirectory.getPendingDeletions()).thenReturn(pendingDeletions); - assertEquals(pendingDeletions, compositeDirectory.getPendingDeletions()); - } } From 5013be961c7f32f06ade59dd802a4e7b46ec48e3 Mon Sep 17 00:00:00 2001 From: Shreyansh Ray Date: Fri, 10 May 2024 17:21:02 +0530 Subject: [PATCH 13/16] Handle tmp file deletion Signed-off-by: Shreyansh Ray --- .../opensearch/index/store/CompositeDirectory.java | 14 +++++++++----- .../index/store/CompositeDirectoryTests.java | 6 +++++- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java index 583a181c4797a..597a64158be03 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java @@ -108,11 +108,15 @@ public String[] listAll() throws IOException { @Override public void deleteFile(String name) throws IOException { logger.trace("deleteFile() called {}", name); - /* - Not deleting from localDirectory directly since it causes a race condition when the localDirectory deletes a file, and it ends up in pendingDeletion state. - Meanwhile, fileCache on removal deletes the file directly via the Files class and later when the directory tries to delete the files pending for deletion (which happens before creating a new file), it causes NoSuchFileException and new file creation fails - */ - fileCache.remove(localDirectory.getDirectory().resolve(name)); + if (isTempFile(name)) { + localDirectory.deleteFile(name); + } else { + /* + Not deleting from localDirectory directly since it causes a race condition when the localDirectory deletes a file, and it ends up in pendingDeletion state. + Meanwhile, fileCache on removal deletes the file directly via the Files class and later when the directory tries to delete the files pending for deletion (which happens before creating a new file), it causes NoSuchFileException and new file creation fails + */ + fileCache.remove(localDirectory.getDirectory().resolve(name)); + } } /** diff --git a/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java b/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java index 771734ac23a9e..cfc6f0277d23d 100644 --- a/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java @@ -28,6 +28,7 @@ import java.util.Map; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.startsWith; import static org.mockito.Mockito.mock; @@ -60,9 +61,12 @@ public void testListAll() throws IOException { public void testDeleteFile() throws IOException { Path basePath = mock(Path.class); Path resolvedPath = mock(Path.class); - when(basePath.resolve("_0.si")).thenReturn(resolvedPath); + when(basePath.resolve(anyString())).thenReturn(resolvedPath); when(localDirectory.getDirectory()).thenReturn(basePath); + compositeDirectory.deleteFile("_0.tmp"); + verify(localDirectory).deleteFile("_0.tmp"); + compositeDirectory.deleteFile("_0.si"); verify(fileCache).remove(resolvedPath); } From 7491c62643b44760824ab82d6e117fb3c0c5609c Mon Sep 17 00:00:00 2001 From: Shreyansh Ray Date: Mon, 13 May 2024 19:59:36 +0530 Subject: [PATCH 14/16] Nit fixes Signed-off-by: Shreyansh Ray --- .../remotestore/CompositeDirectoryIT.java | 3 +-- .../org/opensearch/index/IndexModule.java | 2 +- .../org/opensearch/index/IndexService.java | 21 +++++++++++++------ .../index/store/CompositeDirectory.java | 16 ++++++-------- .../filecache/FullFileCachedIndexInput.java | 7 ++----- .../index/store/CompositeDirectoryTests.java | 21 +++++++++++++++++-- 6 files changed, 44 insertions(+), 26 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java index a34b665561289..3d52e6614f6a3 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/CompositeDirectoryIT.java @@ -26,7 +26,6 @@ import org.opensearch.index.store.remote.file.CleanerDaemonThreadLeakFilter; import org.opensearch.indices.IndicesService; import org.opensearch.test.OpenSearchIntegTestCase; -import org.opensearch.test.junit.annotations.TestLogging; import java.util.Map; @@ -36,7 +35,7 @@ @ThreadLeakFilters(filters = CleanerDaemonThreadLeakFilter.class) @OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 1, numClientNodes = 0, supportsDedicatedMasters = false) // Uncomment the below line to enable trace level logs for this test for better debugging -@TestLogging(reason = "Getting trace logs from composite directory package", value = "org.opensearch.index.store:TRACE") +// @TestLogging(reason = "Getting trace logs from composite directory package", value = "org.opensearch.index.store:TRACE") public class CompositeDirectoryIT extends RemoteStoreBaseIntegTestCase { /* diff --git a/server/src/main/java/org/opensearch/index/IndexModule.java b/server/src/main/java/org/opensearch/index/IndexModule.java index d79f19f17e167..4c494a6b35153 100644 --- a/server/src/main/java/org/opensearch/index/IndexModule.java +++ b/server/src/main/java/org/opensearch/index/IndexModule.java @@ -646,7 +646,7 @@ public static DataLocalityType getValueOf(final String localityType) { if (type != null) { return type; } - throw new IllegalArgumentException("Unknown Locality Type constant [" + localityType + "]."); + throw new IllegalArgumentException("Unknown locality type constant [" + localityType + "]."); } } diff --git a/server/src/main/java/org/opensearch/index/IndexService.java b/server/src/main/java/org/opensearch/index/IndexService.java index 78bfedb7b6d65..d021b290e585b 100644 --- a/server/src/main/java/org/opensearch/index/IndexService.java +++ b/server/src/main/java/org/opensearch/index/IndexService.java @@ -618,14 +618,23 @@ public synchronized IndexShard createShard( // TODO : Need to remove this check after support for hot indices is added in Composite Directory this.indexSettings.isStoreLocalityPartial()) { /* - * Currently Composite Directory only supports local directory to be of type FSDirectory - * The reason is that FileCache currently has it key type as Path - * Composite Directory currently uses FSDirectory's getDirectory() method to fetch and use the Path for operating on FileCache - * TODO : Refactor FileCache to have key in form of String instead of Path. Once that is done we can remove this assertion + Currently Composite Directory only supports local directory to be of type FSDirectory + The reason is that FileCache currently has it key type as Path + Composite Directory currently uses FSDirectory's getDirectory() method to fetch and use the Path for operating on FileCache + TODO : Refactor FileCache to have key in form of String instead of Path. Once that is done we can remove this assertion */ Directory localDirectory = directoryFactory.newDirectory(this.indexSettings, path); - assert localDirectory instanceof FSDirectory : "For Composite Directory, local directory must be of type FSDirectory"; - assert fileCache != null : "File Cache not initialized on this Node, cannot create Composite Directory without FileCache"; + + if (localDirectory instanceof FSDirectory == false) throw new IllegalStateException( + "For Composite Directory, local directory must be of type FSDirectory" + ); + else if (fileCache == null) throw new IllegalStateException( + "File Cache not initialized on this Node, cannot create Composite Directory without FileCache" + ); + else if (remoteDirectory == null) throw new IllegalStateException( + "Remote Directory must not be null for Composite Directory" + ); + directory = new CompositeDirectory((FSDirectory) localDirectory, (RemoteSegmentStoreDirectory) remoteDirectory, fileCache); } else { directory = directoryFactory.newDirectory(this.indexSettings, path); diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java index 597a64158be03..76f3bb868d43a 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java @@ -101,7 +101,7 @@ public String[] listAll() throws IOException { /** * Removes an existing file in the directory. - * Currently deleting only from local directory as files from remote should not be deleted due to availability reasons + * Currently deleting only from local directory as files from remote should not be deleted as that is taken care by garbage collection logic of remote directory * @param name the name of an existing file. * @throws IOException in case of I/O error */ @@ -181,6 +181,7 @@ public void rename(String source, String dest) throws IOException { localDirectory.rename(source, dest); fileCache.remove(localDirectory.getDirectory().resolve(source)); cacheFile(dest); + fileCache.decRef(localDirectory.getDirectory().resolve(dest)); } /** @@ -243,10 +244,6 @@ public void close() throws IOException { */ public void afterSyncToRemote(Collection files) throws IOException { logger.trace("afterSyncToRemote called for {}", files); - if (remoteDirectory == null) { - logger.trace("afterSyncToRemote called even though remote directory is not set"); - return; - } for (String fileName : files) { /* Decrementing the refCount here for the path so that it becomes eligible for eviction @@ -273,10 +270,9 @@ private String[] getRemoteFiles() throws IOException { remoteFiles = remoteDirectory.listAll(); } catch (NullPointerException e) { /* - There are two scenarios where the listAll() call on remote directory returns NullPointerException: - - When remote directory is not set - - When init() of remote directory has not yet been called - Returning an empty list in the above scenarios + We can encounter NPE when no data has been uploaded to remote store yet and as a result the metadata is empty + Empty metadata means that there are no files currently in remote, hence returning an empty list in this scenario + TODO : Catch the NPE in listAll of RemoteSegmentStoreDirectory itself instead of catching here */ remoteFiles = new String[0]; } @@ -285,7 +281,7 @@ There are two scenarios where the listAll() call on remote directory returns Nul private void cacheFile(String name) throws IOException { Path filePath = localDirectory.getDirectory().resolve(name); - // put will increase the refCount for the path, making sure it is not evicted, wil decrease the ref after it is uploaded to Remote + // put will increase the refCount for the path, making sure it is not evicted, will decrease the ref after it is uploaded to Remote // so that it can be evicted after that // this is just a temporary solution, will pin the file once support for that is added in FileCache // TODO : Pin the above filePath in the file cache once pinning support is added so that it cannot be evicted unless it has been diff --git a/server/src/main/java/org/opensearch/index/store/remote/filecache/FullFileCachedIndexInput.java b/server/src/main/java/org/opensearch/index/store/remote/filecache/FullFileCachedIndexInput.java index 7b1163b18727e..f8aed0432cba8 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/filecache/FullFileCachedIndexInput.java +++ b/server/src/main/java/org/opensearch/index/store/remote/filecache/FullFileCachedIndexInput.java @@ -22,8 +22,6 @@ */ @ExperimentalApi public class FullFileCachedIndexInput implements CachedIndexInput { - - private final IndexInput indexInput; private final FileCache fileCache; private final Path path; private final FileCachedIndexInput fileCachedIndexInput; @@ -35,7 +33,6 @@ public class FullFileCachedIndexInput implements CachedIndexInput { public FullFileCachedIndexInput(FileCache fileCache, Path path, IndexInput indexInput) { this.fileCache = fileCache; this.path = path; - this.indexInput = indexInput; fileCachedIndexInput = new FileCachedIndexInput(fileCache, path, indexInput); isClosed = new AtomicBoolean(false); } @@ -54,7 +51,7 @@ public IndexInput getIndexInput() { */ @Override public long length() { - return indexInput.length(); + return fileCachedIndexInput.length(); } /** @@ -71,7 +68,7 @@ public boolean isClosed() { @Override public void close() throws Exception { if (!isClosed.getAndSet(true)) { - indexInput.close(); + fileCachedIndexInput.close(); } } } diff --git a/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java b/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java index cfc6f0277d23d..64649978129c4 100644 --- a/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/CompositeDirectoryTests.java @@ -23,6 +23,7 @@ import java.io.IOException; import java.nio.file.Path; +import java.util.Arrays; import java.util.Collection; import java.util.List; import java.util.Map; @@ -32,6 +33,7 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.startsWith; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -50,11 +52,16 @@ public void setup() throws IOException { } public void testListAll() throws IOException { + when(localDirectory.listAll()).thenReturn(new String[]{}); + String[] actualFileNames = compositeDirectory.listAll(); + String[] expectedFileNames = new String[] {}; + assertArrayEquals(expectedFileNames, actualFileNames); + populateMetadata(); when(localDirectory.listAll()).thenReturn(new String[] { "_1.cfe", "_2.cfe", "_0.cfe_block_7", "_0.cfs_block_7" }); - String[] actualFileNames = compositeDirectory.listAll(); - String[] expectedFileNames = new String[] { "_0.cfe", "_0.cfs", "_0.si", "_1.cfe", "_2.cfe", "segments_1" }; + actualFileNames = compositeDirectory.listAll(); + expectedFileNames = new String[] { "_0.cfe", "_0.cfs", "_0.si", "_1.cfe", "_2.cfe", "segments_1" }; assertArrayEquals(expectedFileNames, actualFileNames); } @@ -169,4 +176,14 @@ public void testClose() throws IOException { verify(fileCache).remove(resolvedPath1); verify(fileCache).remove(resolvedPath2); } + + public void testAfterSyncToRemote() throws IOException { + Path basePath = mock(Path.class); + Path resolvedPath = mock(Path.class); + when(basePath.resolve(anyString())).thenReturn(resolvedPath); + when(localDirectory.getDirectory()).thenReturn(basePath); + Collection files = Arrays.asList("_0.si", "_0.cfs"); + compositeDirectory.afterSyncToRemote(files); + verify(fileCache, times(files.size())).decRef(resolvedPath); + } } From f7ed4ccaa144ede1b900a3af59d2e62924adef61 Mon Sep 17 00:00:00 2001 From: Nishant Goel Date: Wed, 29 May 2024 12:04:28 +0530 Subject: [PATCH 15/16] Replication/Recovery changes for writable warm index --- ...mIndexRemoteStoreSegmentReplicationIT.java | 297 ++++++++++++++++++ .../opensearch/index/shard/IndexShard.java | 20 +- .../RemoteStoreReplicationSource.java | 6 +- .../replication/SegmentReplicationTarget.java | 58 ++-- 4 files changed, 350 insertions(+), 31 deletions(-) create mode 100644 server/src/internalClusterTest/java/org/opensearch/indices/replication/WarmIndexRemoteStoreSegmentReplicationIT.java diff --git a/server/src/internalClusterTest/java/org/opensearch/indices/replication/WarmIndexRemoteStoreSegmentReplicationIT.java b/server/src/internalClusterTest/java/org/opensearch/indices/replication/WarmIndexRemoteStoreSegmentReplicationIT.java new file mode 100644 index 0000000000000..1964dc3136bb2 --- /dev/null +++ b/server/src/internalClusterTest/java/org/opensearch/indices/replication/WarmIndexRemoteStoreSegmentReplicationIT.java @@ -0,0 +1,297 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.indices.replication; + +import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_REPOSITORY_SETTINGS_ATTRIBUTE_KEY_PREFIX; +import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_REPOSITORY_TYPE_ATTRIBUTE_KEY_FORMAT; + +import com.carrotsearch.randomizedtesting.annotations.ThreadLeakFilters; +import java.io.IOException; +import java.nio.file.Path; +import java.util.Locale; +import java.util.Map; +import java.util.Objects; +import java.util.stream.Collectors; +import org.junit.After; +import org.junit.Before; +import org.opensearch.cluster.metadata.RepositoriesMetadata; +import org.opensearch.cluster.metadata.RepositoryMetadata; +import org.opensearch.cluster.node.DiscoveryNode; +import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.settings.Settings; +import org.opensearch.common.util.FeatureFlags; +import org.opensearch.index.IndexModule; +import org.opensearch.index.store.remote.file.CleanerDaemonThreadLeakFilter; +import org.opensearch.repositories.RepositoriesService; +import org.opensearch.repositories.blobstore.BlobStoreRepository; +import org.opensearch.test.OpenSearchIntegTestCase; + +@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0) +@ThreadLeakFilters(filters = CleanerDaemonThreadLeakFilter.class) +public class WarmIndexRemoteStoreSegmentReplicationIT extends SegmentReplicationIT { + + protected static final String REPOSITORY_NAME = "test-remote-store-repo"; + protected static final String REPOSITORY_2_NAME = "test-remote-store-repo-2"; + + protected Path segmentRepoPath; + protected Path translogRepoPath; + protected boolean clusterSettingsSuppliedByTest = false; + + @Before + private void setup() { + internalCluster().startClusterManagerOnlyNode(); + } + + @Override + public Settings indexSettings() { + return Settings.builder() + .put(super.indexSettings()) + .put(IndexModule.INDEX_STORE_LOCALITY_SETTING.getKey(), "partial") + .build(); + } + + @Override + protected Settings nodeSettings(int nodeOrdinal) { + if (segmentRepoPath == null || translogRepoPath == null) { + segmentRepoPath = randomRepoPath().toAbsolutePath(); + translogRepoPath = randomRepoPath().toAbsolutePath(); + } + if (clusterSettingsSuppliedByTest) { + return Settings.builder().put(super.nodeSettings(nodeOrdinal)).build(); + } else { + return Settings.builder() + .put(super.nodeSettings(nodeOrdinal)) + .put(remoteStoreClusterSettings(REPOSITORY_NAME, segmentRepoPath, REPOSITORY_2_NAME, translogRepoPath)) + //.put(RemoteStoreSettings.CLUSTER_REMOTE_INDEX_SEGMENT_METADATA_RETENTION_MAX_COUNT_SETTING.getKey(), -1) + .build(); + } + } + + @Override + protected Settings featureFlagSettings() { + Settings.Builder featureSettings = Settings.builder(); + featureSettings.put(FeatureFlags.WRITEABLE_REMOTE_INDEX, true); + + return featureSettings.build(); + } + + @Override + protected boolean addMockIndexStorePlugin() { + return false; + } + + public void testPrimaryStopped_ReplicaPromoted() throws Exception { + super.testPrimaryStopped_ReplicaPromoted(); + } + + public void testRestartPrimary() throws Exception { + super.testRestartPrimary(); + } + + public void testCancelPrimaryAllocation() throws Exception { + super.testCancelPrimaryAllocation(); + } + + public void testReplicationAfterPrimaryRefreshAndFlush() throws Exception { + super.testReplicationAfterPrimaryRefreshAndFlush(); + } + + public void testIndexReopenClose() throws Exception { + super.testIndexReopenClose(); + } + + public void testScrollWithConcurrentIndexAndSearch() throws Exception { + super.testScrollWithConcurrentIndexAndSearch(); + } + + public void testMultipleShards() throws Exception { + super.testMultipleShards(); + } + + public void testReplicationAfterForceMerge() throws Exception { + super.testReplicationAfterForceMerge(); + } + + public void testReplicationAfterForceMergeOnPrimaryShardsOnly() throws Exception { + super.testReplicationAfterForceMergeOnPrimaryShardsOnly(); + } + + public void testNodeDropWithOngoingReplication() throws Exception { + super.testNodeDropWithOngoingReplication(); + } + + public void testCancellation() throws Exception { + super.testCancellation(); + } + + public void testStartReplicaAfterPrimaryIndexesDocs() throws Exception { + super.testStartReplicaAfterPrimaryIndexesDocs(); + } + + public void testDeleteOperations() throws Exception { + super.testDeleteOperations(); + } + + public void testReplicationPostDeleteAndForceMerge() throws Exception { + super.testReplicationPostDeleteAndForceMerge(); + } + + public void testUpdateOperations() throws Exception { + super.testUpdateOperations(); + } + + public void testReplicaHasDiffFilesThanPrimary() throws Exception { + super.testReplicaHasDiffFilesThanPrimary(); + } + + public void testDropPrimaryDuringReplication() throws Exception { + super.testDropPrimaryDuringReplication(); + } + + public void testPressureServiceStats() throws Exception { + super.testPressureServiceStats(); + } + + public void testScrollCreatedOnReplica() throws Exception { + assumeFalse( + "Skipping the test as its not compatible with segment replication with remote store.", + warmIndexSegmentReplicationEnabled() + ); + super.testScrollCreatedOnReplica(); + } + + public void testScrollWithOngoingSegmentReplication() { + assumeFalse( + "Skipping the test as its not compatible with segment replication with remote store.", + warmIndexSegmentReplicationEnabled() + ); + } + + public void testPitCreatedOnReplica() throws Exception { + assumeFalse( + "Skipping the test as its not compatible with segment replication with remote store.", + warmIndexSegmentReplicationEnabled() + ); + super.testPitCreatedOnReplica(); + } + + public void testPrimaryReceivesDocsDuringReplicaRecovery() throws Exception { + super.testPrimaryReceivesDocsDuringReplicaRecovery(); + } + + public void testIndexWhileRecoveringReplica() throws Exception { + super.testIndexWhileRecoveringReplica(); + } + + public void testRestartPrimary_NoReplicas() throws Exception { + super.testRestartPrimary_NoReplicas(); + } + + public void testRealtimeGetRequestsSuccessful() { + super.testRealtimeGetRequestsSuccessful(); + } + + public void testRealtimeGetRequestsUnsuccessful() { + super.testRealtimeGetRequestsUnsuccessful(); + } + + public void testRealtimeMultiGetRequestsSuccessful() { + super.testRealtimeMultiGetRequestsSuccessful(); + } + + public void testRealtimeMultiGetRequestsUnsuccessful() { + super.testRealtimeMultiGetRequestsUnsuccessful(); + } + + public void testRealtimeTermVectorRequestsSuccessful() throws IOException { + super.testRealtimeTermVectorRequestsSuccessful(); + } + + public void testRealtimeTermVectorRequestsUnSuccessful() throws IOException { + super.testRealtimeTermVectorRequestsUnSuccessful(); + } + + public void testReplicaAlreadyAtCheckpoint() throws Exception { + super.testReplicaAlreadyAtCheckpoint(); + } + + public void testCancellationDuringGetCheckpointInfo() { + assumeFalse( + "Skipping the test as its not compatible with segment replication with remote store.", + warmIndexSegmentReplicationEnabled() + ); + } + + public void testCancellationDuringGetSegments() { + assumeFalse( + "Skipping the test as its not compatible with segment replication with remote store.", + warmIndexSegmentReplicationEnabled() + ); + } + + protected boolean warmIndexSegmentReplicationEnabled() { + return !Objects.equals(IndexModule.INDEX_STORE_LOCALITY_SETTING.get(indexSettings()).toString(), "partial"); + } + + @After + public void teardown() { + clusterSettingsSuppliedByTest = false; + assertRemoteStoreRepositoryOnAllNodes(REPOSITORY_NAME); + assertRemoteStoreRepositoryOnAllNodes(REPOSITORY_2_NAME); + clusterAdmin().prepareCleanupRepository(REPOSITORY_NAME).get(); + clusterAdmin().prepareCleanupRepository(REPOSITORY_2_NAME).get(); + } + + public RepositoryMetadata buildRepositoryMetadata(DiscoveryNode node, String name) { + Map nodeAttributes = node.getAttributes(); + String type = nodeAttributes.get(String.format(Locale.getDefault(), REMOTE_STORE_REPOSITORY_TYPE_ATTRIBUTE_KEY_FORMAT, name)); + + String settingsAttributeKeyPrefix = String.format(Locale.getDefault(), REMOTE_STORE_REPOSITORY_SETTINGS_ATTRIBUTE_KEY_PREFIX, name); + Map settingsMap = node.getAttributes() + .keySet() + .stream() + .filter(key -> key.startsWith(settingsAttributeKeyPrefix)) + .collect(Collectors.toMap(key -> key.replace(settingsAttributeKeyPrefix, ""), key -> node.getAttributes().get(key))); + + Settings.Builder settings = Settings.builder(); + settingsMap.entrySet().forEach(entry -> settings.put(entry.getKey(), entry.getValue())); + settings.put(BlobStoreRepository.SYSTEM_REPOSITORY_SETTING.getKey(), true); + + return new RepositoryMetadata(name, type, settings.build()); + } + + public void assertRemoteStoreRepositoryOnAllNodes(String repositoryName) { + RepositoriesMetadata repositories = internalCluster().getInstance(ClusterService.class, internalCluster().getNodeNames()[0]) + .state() + .metadata() + .custom(RepositoriesMetadata.TYPE); + RepositoryMetadata actualRepository = repositories.repository(repositoryName); + + final RepositoriesService repositoriesService = internalCluster().getClusterManagerNodeInstance(RepositoriesService.class); + final BlobStoreRepository repository = (BlobStoreRepository) repositoriesService.repository(repositoryName); + + for (String nodeName : internalCluster().getNodeNames()) { + ClusterService clusterService = internalCluster().getInstance(ClusterService.class, nodeName); + DiscoveryNode node = clusterService.localNode(); + RepositoryMetadata expectedRepository = buildRepositoryMetadata(node, repositoryName); + + // Validated that all the restricted settings are entact on all the nodes. + repository.getRestrictedSystemRepositorySettings() + .stream() + .forEach( + setting -> assertEquals( + String.format(Locale.ROOT, "Restricted Settings mismatch [%s]", setting.getKey()), + setting.get(actualRepository.settings()), + setting.get(expectedRepository.settings()) + ) + ); + } + } + +} diff --git a/server/src/main/java/org/opensearch/index/shard/IndexShard.java b/server/src/main/java/org/opensearch/index/shard/IndexShard.java index 18d4a2ca6d639..11f12a5dcbbf0 100644 --- a/server/src/main/java/org/opensearch/index/shard/IndexShard.java +++ b/server/src/main/java/org/opensearch/index/shard/IndexShard.java @@ -5022,6 +5022,8 @@ public void syncSegmentsFromRemoteSegmentStore(boolean overrideLocal) throws IOE */ public void syncSegmentsFromRemoteSegmentStore(boolean overrideLocal, final Runnable onFileSync) throws IOException { boolean syncSegmentSuccess = false; + + boolean shouldOverrideLocalFiles = overrideLocal && indexSettings.isStoreLocalityPartial() == false; long startTimeMs = System.currentTimeMillis(); assert indexSettings.isRemoteStoreEnabled() || this.isRemoteSeeded(); logger.trace("Downloading segments from remote segment store"); @@ -5044,7 +5046,7 @@ public void syncSegmentsFromRemoteSegmentStore(boolean overrideLocal, final Runn storeDirectory = new StoreRecovery.StatsDirectoryWrapper(store.directory(), recoveryState.getIndex()); for (String file : uploadedSegments.keySet()) { long checksum = Long.parseLong(uploadedSegments.get(file).getChecksum()); - if (overrideLocal || localDirectoryContains(storeDirectory, file, checksum) == false) { + if (shouldOverrideLocalFiles || localDirectoryContains(storeDirectory, file, checksum) == false) { recoveryState.getIndex().addFileDetail(file, uploadedSegments.get(file).getLength(), false); } else { recoveryState.getIndex().addFileDetail(file, uploadedSegments.get(file).getLength(), true); @@ -5053,7 +5055,9 @@ public void syncSegmentsFromRemoteSegmentStore(boolean overrideLocal, final Runn } else { storeDirectory = store.directory(); } - copySegmentFiles(storeDirectory, remoteDirectory, null, uploadedSegments, overrideLocal, onFileSync); + if (indexSettings.isStoreLocalityPartial() == false) { + copySegmentFiles(storeDirectory, remoteDirectory, null, uploadedSegments, overrideLocal, onFileSync); + } if (remoteSegmentMetadata != null) { final SegmentInfos infosSnapshot = store.buildSegmentInfos( @@ -5063,13 +5067,15 @@ public void syncSegmentsFromRemoteSegmentStore(boolean overrideLocal, final Runn long processedLocalCheckpoint = Long.parseLong(infosSnapshot.getUserData().get(LOCAL_CHECKPOINT_KEY)); // delete any other commits, we want to start the engine only from a new commit made with the downloaded infos bytes. // Extra segments will be wiped on engine open. - for (String file : List.of(store.directory().listAll())) { - if (file.startsWith(IndexFileNames.SEGMENTS)) { - store.deleteQuiet(file); + if (indexSettings.isStoreLocalityPartial() == false) { + for (String file : List.of(store.directory().listAll())) { + if (file.startsWith(IndexFileNames.SEGMENTS)) { + store.deleteQuiet(file); + } } + assert Arrays.stream(store.directory().listAll()).filter(f -> f.startsWith(IndexFileNames.SEGMENTS)).findAny().isEmpty() + : "There should not be any segments file in the dir"; } - assert Arrays.stream(store.directory().listAll()).filter(f -> f.startsWith(IndexFileNames.SEGMENTS)).findAny().isEmpty() - : "There should not be any segments file in the dir"; store.commitSegmentInfos(infosSnapshot, processedLocalCheckpoint, processedLocalCheckpoint); } syncSegmentSuccess = true; diff --git a/server/src/main/java/org/opensearch/indices/replication/RemoteStoreReplicationSource.java b/server/src/main/java/org/opensearch/indices/replication/RemoteStoreReplicationSource.java index b06b3e0497cf7..0bc762d1ec6de 100644 --- a/server/src/main/java/org/opensearch/indices/replication/RemoteStoreReplicationSource.java +++ b/server/src/main/java/org/opensearch/indices/replication/RemoteStoreReplicationSource.java @@ -117,9 +117,13 @@ public void getSegmentFiles( final List toDownloadSegmentNames = new ArrayList<>(); for (StoreFileMetadata fileMetadata : filesToFetch) { String file = fileMetadata.name(); - assert directoryFiles.contains(file) == false : "Local store already contains the file " + file; + assert directoryFiles.contains(file) == false || indexShard.indexSettings().isStoreLocalityPartial() : "Local store already contains the file " + file; toDownloadSegmentNames.add(file); } + if(indexShard.indexSettings().isStoreLocalityPartial()) { + listener.onResponse(new GetSegmentFilesResponse(filesToFetch)); + return; + } indexShard.getFileDownloader() .downloadAsync( cancellableThreads, diff --git a/server/src/main/java/org/opensearch/indices/replication/SegmentReplicationTarget.java b/server/src/main/java/org/opensearch/indices/replication/SegmentReplicationTarget.java index af764556b7549..7e9211233e358 100644 --- a/server/src/main/java/org/opensearch/indices/replication/SegmentReplicationTarget.java +++ b/server/src/main/java/org/opensearch/indices/replication/SegmentReplicationTarget.java @@ -8,6 +8,7 @@ package org.opensearch.indices.replication; +import java.util.Map; import org.apache.logging.log4j.message.ParameterizedMessage; import org.apache.lucene.codecs.CodecUtil; import org.apache.lucene.index.CorruptIndexException; @@ -169,15 +170,21 @@ public void startReplication(ActionListener listener) { state.setStage(SegmentReplicationState.Stage.REPLICATING); final StepListener checkpointInfoListener = new StepListener<>(); final StepListener getFilesListener = new StepListener<>(); - + Map replicaMd = null; + try { + replicaMd = indexShard.getSegmentMetadataMap(); + } catch (IOException e) { + listener.onFailure(new RuntimeException(e)); + } logger.trace(new ParameterizedMessage("Starting Replication Target: {}", description())); // Get list of files to copy from this checkpoint. state.setStage(SegmentReplicationState.Stage.GET_CHECKPOINT_INFO); cancellableThreads.checkForCancel(); source.getCheckpointMetadata(getId(), checkpoint, checkpointInfoListener); + Map finalReplicaMd = replicaMd; checkpointInfoListener.whenComplete(checkpointInfo -> { - final List filesToFetch = getFiles(checkpointInfo); + final List filesToFetch = getFiles(checkpointInfo, finalReplicaMd); state.setStage(SegmentReplicationState.Stage.GET_FILES); cancellableThreads.checkForCancel(); source.getSegmentFiles( @@ -196,31 +203,36 @@ public void startReplication(ActionListener listener) { }, listener::onFailure); } - private List getFiles(CheckpointInfoResponse checkpointInfo) throws IOException { + private List getFiles(CheckpointInfoResponse checkpointInfo, Map finalReplicaMd) throws IOException { cancellableThreads.checkForCancel(); state.setStage(SegmentReplicationState.Stage.FILE_DIFF); - final Store.RecoveryDiff diff = Store.segmentReplicationDiff(checkpointInfo.getMetadataMap(), indexShard.getSegmentMetadataMap()); + final Store.RecoveryDiff diff = Store.segmentReplicationDiff(checkpointInfo.getMetadataMap(), finalReplicaMd); // local files - final Set localFiles = Set.of(indexShard.store().directory().listAll()); - // set of local files that can be reused - final Set reuseFiles = diff.missing.stream() - .filter(storeFileMetadata -> localFiles.contains(storeFileMetadata.name())) - .filter(this::validateLocalChecksum) - .map(StoreFileMetadata::name) - .collect(Collectors.toSet()); - - final List missingFiles = diff.missing.stream() - .filter(md -> reuseFiles.contains(md.name()) == false) - .collect(Collectors.toList()); + final List missingFiles; + // Skip reuse logic for warm indices + if (indexShard.indexSettings().isStoreLocalityPartial() == true) { + missingFiles = diff.missing; + } else { + final Set localFiles = Set.of(indexShard.store().directory().listAll()); + // set of local files that can be reused + final Set reuseFiles = diff.missing.stream() + .filter(storeFileMetadata -> localFiles.contains(storeFileMetadata.name())) + .filter(this::validateLocalChecksum) + .map(StoreFileMetadata::name) + .collect(Collectors.toSet()); + missingFiles = diff.missing.stream() + .filter(md -> reuseFiles.contains(md.name()) == false) + .collect(Collectors.toList()); - logger.trace( - () -> new ParameterizedMessage( - "Replication diff for checkpoint {} {} {}", - checkpointInfo.getCheckpoint(), - missingFiles, - diff.different - ) - ); + logger.trace( + () -> new ParameterizedMessage( + "Replication diff for checkpoint {} {} {}", + checkpointInfo.getCheckpoint(), + missingFiles, + diff.different + ) + ); + } /* * Segments are immutable. So if the replica has any segments with the same name that differ from the one in the incoming * snapshot from source that means the local copy of the segment has been corrupted/changed in some way and we throw an From 70511683d3a1ca7e7f956c20ffd9d7a03fd9cb22 Mon Sep 17 00:00:00 2001 From: Nishant Goel Date: Wed, 29 May 2024 12:04:28 +0530 Subject: [PATCH 16/16] Replication/Recovery changes for writable warm index --- .../java/org/opensearch/index/store/CompositeDirectory.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java index 76f3bb868d43a..d4c5c7cce9c51 100644 --- a/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java +++ b/server/src/main/java/org/opensearch/index/store/CompositeDirectory.java @@ -212,9 +212,8 @@ public IndexInput openInput(String name, IOContext context) throws IOException { } // If file has been uploaded to the Remote Store, fetch it from the Remote Store in blocks via OnDemandCompositeBlockIndexInput else { - logger.trace("Complete file not in FileCache, to be fetched in Blocks from Remote"); - RemoteSegmentMetadata remoteSegmentMetadata = remoteDirectory.readLatestMetadataFile(); - RemoteSegmentStoreDirectory.UploadedSegmentMetadata uploadedSegmentMetadata = remoteSegmentMetadata.getMetadata().get(name); + logger.trace("Complete file not in FileCache, to be fetched in Blocks from Remote {}", name); + RemoteSegmentStoreDirectory.UploadedSegmentMetadata uploadedSegmentMetadata = remoteDirectory.getSegmentsUploadedToRemoteStore().get(name); // TODO : Refactor FileInfo and OnDemandBlockSnapshotIndexInput to more generic names as they are not Remote Snapshot specific BlobStoreIndexShardSnapshot.FileInfo fileInfo = new BlobStoreIndexShardSnapshot.FileInfo( name, @@ -233,7 +232,6 @@ public IndexInput openInput(String name, IOContext context) throws IOException { public void close() throws IOException { Arrays.stream(localDirectory.listAll()).forEach(f -> fileCache.remove(localDirectory.getDirectory().resolve(f))); localDirectory.close(); - remoteDirectory.close(); } /**