From 5eead5e5f4d69eec133a81f19d074b988a0627bf Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Tue, 2 May 2023 21:52:43 +0000 Subject: [PATCH 1/5] Cleanup interfaces around ExtensionManager ExtensionsManager has a whole lot of functionality that shouldn't be exposed to external classes, marked much of this package protected for a cleaner interface. Then updated the Noop version to override all public functions to remove extranious feature flag checks. Note; I didn't go so far to remove all getters/setters that were unused but that could be a good follow up in the future. Signed-off-by: Peter Nied --- .../extensions/ExtensionsManager.java | 148 ++++++------------ .../extensions/NoopExtensionsManager.java | 55 ++++++- .../opensearch/indices/IndicesService.java | 5 +- .../main/java/org/opensearch/node/Node.java | 24 ++- .../snapshots/SnapshotResiliencyTests.java | 106 ++++--------- 5 files changed, 141 insertions(+), 197 deletions(-) diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java index 0edc215e536a3..e099098d9c231 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java @@ -19,6 +19,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.TimeUnit; @@ -107,8 +108,7 @@ public static enum OpenSearchRequestType { private final Path extensionsPath; private ExtensionTransportActionsHandler extensionTransportActionsHandler; - // A list of initialized extensions, a subset of the values of map below which includes all extensions - private List extensions; + private Map initializedExtensions; private Map extensionIdMap; private RestActionsRequestHandler restActionsRequestHandler; private CustomSettingsRequestHandler customSettingsRequestHandler; @@ -118,21 +118,16 @@ public static enum OpenSearchRequestType { private AddSettingsUpdateConsumerRequestHandler addSettingsUpdateConsumerRequestHandler; private NodeClient client; - public ExtensionsManager() { - this.extensionsPath = Path.of(""); - } - /** * Instantiate a new ExtensionsManager object to handle requests and responses from extensions. This is called during Node bootstrap. * - * @param settings Settings from the node the orchestrator is running on. * @param extensionsPath Path to a directory containing extensions. * @throws IOException If the extensions discovery file is not properly retrieved. */ - public ExtensionsManager(Settings settings, Path extensionsPath) throws IOException { + public ExtensionsManager(Path extensionsPath) throws IOException { logger.info("ExtensionsManager initialized"); this.extensionsPath = extensionsPath; - this.extensions = new ArrayList(); + this.initializedExtensions = new HashMap(); this.extensionIdMap = new HashMap(); // will be initialized in initializeServicesAndRestHandler which is called after the Node is initialized this.transportService = null; @@ -187,6 +182,16 @@ public void initializeServicesAndRestHandler( registerRequestHandler(); } + /** + * Lookup an initialized extension by its unique id + * + * @param extensionId The unique extension identifier + * @return An optional of the DiscoveryExtensionNode instance for the matching extension + */ + public Optional lookupInitializedExtensionById(final String extensionId) { + return Optional.ofNullable(this.initializedExtensions.get(extensionId)); + } + /** * Handles Transport Request from {@link org.opensearch.extensions.action.ExtensionTransportAction} which was invoked by an extension via {@link ExtensionTransportActionsHandler}. * @@ -342,7 +347,7 @@ private void loadExtension(Extension extension) throws IOException { } /** - * Iterate through all extensions and initialize them. Initialized extensions will be added to the {@link #extensions}. + * Iterate through all extensions and initialize them. Initialized extensions will be added to the {@link #initializedExtensions}. */ public void initialize() { for (DiscoveryExtensionNode extension : extensionIdMap.values()) { @@ -366,7 +371,7 @@ public void handleResponse(InitializeExtensionResponse response) { for (DiscoveryExtensionNode extension : extensionIdMap.values()) { if (extension.getName().equals(response.getName())) { extension.setImplementedInterfaces(response.getImplementedInterfaces()); - extensions.add(extension); + initializedExtensions.put(extension.getId(), extension); logger.info("Initialized extension: " + extension.getName()); break; } @@ -426,11 +431,17 @@ TransportResponse handleExtensionRequest(ExtensionRequest extensionRequest) thro case REQUEST_EXTENSION_DEPENDENCY_INFORMATION: String uniqueId = extensionRequest.getUniqueId(); if (uniqueId == null) { - return new ExtensionDependencyResponse(extensions); + return new ExtensionDependencyResponse( + initializedExtensions.entrySet().stream().map(e -> e.getValue()).collect(Collectors.toList()) + ); } else { ExtensionDependency matchingId = new ExtensionDependency(uniqueId, Version.CURRENT); return new ExtensionDependencyResponse( - extensions.stream().filter(e -> e.dependenciesContain(matchingId)).collect(Collectors.toList()) + initializedExtensions.entrySet() + .stream() + .map(e -> e.getValue()) + .filter(e -> e.dependenciesContain(matchingId)) + .collect(Collectors.toList()) ); } default: @@ -623,154 +634,83 @@ private ExtensionsSettings readFromExtensionsYml(Path filePath) throws IOExcepti } } - public static String getRequestExtensionActionName() { + static String getRequestExtensionActionName() { return REQUEST_EXTENSION_ACTION_NAME; } - public static String getIndicesExtensionPointActionName() { + static String getIndicesExtensionPointActionName() { return INDICES_EXTENSION_POINT_ACTION_NAME; } - public static String getIndicesExtensionNameActionName() { + static String getIndicesExtensionNameActionName() { return INDICES_EXTENSION_NAME_ACTION_NAME; } - public static String getRequestExtensionClusterState() { + static String getRequestExtensionClusterState() { return REQUEST_EXTENSION_CLUSTER_STATE; } - public static String getRequestExtensionClusterSettings() { + static String getRequestExtensionClusterSettings() { return REQUEST_EXTENSION_CLUSTER_SETTINGS; } - public static Logger getLogger() { + static Logger getLogger() { return logger; } - public Path getExtensionsPath() { + Path getExtensionsPath() { return extensionsPath; } - public List getExtensions() { - return extensions; - } - - public TransportService getTransportService() { + TransportService getTransportService() { return transportService; } - public ClusterService getClusterService() { + ClusterService getClusterService() { return clusterService; } - public static String getRequestExtensionRegisterRestActions() { - return REQUEST_EXTENSION_REGISTER_REST_ACTIONS; - } - - public static String getRequestRestExecuteOnExtensionAction() { - return REQUEST_REST_EXECUTE_ON_EXTENSION_ACTION; - } - - public Map getExtensionIdMap() { + Map getExtensionIdMap() { return extensionIdMap; } - public RestActionsRequestHandler getRestActionsRequestHandler() { + RestActionsRequestHandler getRestActionsRequestHandler() { return restActionsRequestHandler; } - public void setExtensions(List extensions) { - this.extensions = extensions; - } - - public void setExtensionIdMap(Map extensionIdMap) { + void setExtensionIdMap(Map extensionIdMap) { this.extensionIdMap = extensionIdMap; } - public void setRestActionsRequestHandler(RestActionsRequestHandler restActionsRequestHandler) { + void setRestActionsRequestHandler(RestActionsRequestHandler restActionsRequestHandler) { this.restActionsRequestHandler = restActionsRequestHandler; } - public void setTransportService(TransportService transportService) { + void setTransportService(TransportService transportService) { this.transportService = transportService; } - public void setClusterService(ClusterService clusterService) { + void setClusterService(ClusterService clusterService) { this.clusterService = clusterService; } - public static String getRequestExtensionRegisterTransportActions() { - return REQUEST_EXTENSION_REGISTER_TRANSPORT_ACTIONS; - } - - public static String getRequestExtensionRegisterCustomSettings() { - return REQUEST_EXTENSION_REGISTER_CUSTOM_SETTINGS; - } - - public CustomSettingsRequestHandler getCustomSettingsRequestHandler() { + CustomSettingsRequestHandler getCustomSettingsRequestHandler() { return customSettingsRequestHandler; } - public void setCustomSettingsRequestHandler(CustomSettingsRequestHandler customSettingsRequestHandler) { + void setCustomSettingsRequestHandler(CustomSettingsRequestHandler customSettingsRequestHandler) { this.customSettingsRequestHandler = customSettingsRequestHandler; } - public static String getRequestExtensionEnvironmentSettings() { - return REQUEST_EXTENSION_ENVIRONMENT_SETTINGS; - } - - public static String getRequestExtensionAddSettingsUpdateConsumer() { - return REQUEST_EXTENSION_ADD_SETTINGS_UPDATE_CONSUMER; - } - - public static String getRequestExtensionUpdateSettings() { - return REQUEST_EXTENSION_UPDATE_SETTINGS; - } - - public AddSettingsUpdateConsumerRequestHandler getAddSettingsUpdateConsumerRequestHandler() { + AddSettingsUpdateConsumerRequestHandler getAddSettingsUpdateConsumerRequestHandler() { return addSettingsUpdateConsumerRequestHandler; } - public void setAddSettingsUpdateConsumerRequestHandler( - AddSettingsUpdateConsumerRequestHandler addSettingsUpdateConsumerRequestHandler - ) { + void setAddSettingsUpdateConsumerRequestHandler(AddSettingsUpdateConsumerRequestHandler addSettingsUpdateConsumerRequestHandler) { this.addSettingsUpdateConsumerRequestHandler = addSettingsUpdateConsumerRequestHandler; } - public Settings getEnvironmentSettings() { + Settings getEnvironmentSettings() { return environmentSettings; } - - public void setEnvironmentSettings(Settings environmentSettings) { - this.environmentSettings = environmentSettings; - } - - public static String getRequestExtensionHandleTransportAction() { - return REQUEST_EXTENSION_HANDLE_TRANSPORT_ACTION; - } - - public static String getTransportActionRequestFromExtension() { - return TRANSPORT_ACTION_REQUEST_FROM_EXTENSION; - } - - public static int getExtensionRequestWaitTimeout() { - return EXTENSION_REQUEST_WAIT_TIMEOUT; - } - - public ExtensionTransportActionsHandler getExtensionTransportActionsHandler() { - return extensionTransportActionsHandler; - } - - public void setExtensionTransportActionsHandler(ExtensionTransportActionsHandler extensionTransportActionsHandler) { - this.extensionTransportActionsHandler = extensionTransportActionsHandler; - } - - public NodeClient getClient() { - return client; - } - - public void setClient(NodeClient client) { - this.client = client; - } - } diff --git a/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java index 24f71476dcb1e..0f069d5395e8c 100644 --- a/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java @@ -8,6 +8,23 @@ package org.opensearch.extensions; +import java.io.IOException; +import java.net.UnknownHostException; +import java.nio.file.Path; +import java.util.Optional; + +import org.opensearch.action.ActionModule; +import org.opensearch.client.node.NodeClient; +import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsModule; + +import org.opensearch.extensions.action.ExtensionActionRequest; +import org.opensearch.extensions.action.ExtensionActionResponse; +import org.opensearch.extensions.action.RemoteExtensionActionResponse; +import org.opensearch.index.IndexModule; +import org.opensearch.transport.TransportService; + /** * Noop class for ExtensionsManager * @@ -15,7 +32,41 @@ */ public class NoopExtensionsManager extends ExtensionsManager { - public NoopExtensionsManager() { - super(); + public NoopExtensionsManager() throws IOException { + super(Path.of("")); + } + + public void initializeServicesAndRestHandler( + ActionModule actionModule, + SettingsModule settingsModule, + TransportService transportService, + ClusterService clusterService, + Settings initialEnvironmentSettings, + NodeClient client + ) { + // no-op + } + + public RemoteExtensionActionResponse handleRemoteTransportRequest(ExtensionActionRequest request) throws Exception { + // no-op empty response + return new RemoteExtensionActionResponse(true, new byte[0]); + } + + public ExtensionActionResponse handleTransportRequest(ExtensionActionRequest request) throws Exception { + // no-op empty response + return new ExtensionActionResponse(new byte[0]); + } + + public void initialize() { + // no-op + } + + public void onIndexModule(IndexModule indexModule) throws UnknownHostException { + // no-op + } + + public Optional lookupInitializedExtensionById(final String extensionId) { + // no-op not found + return Optional.empty(); } } diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index 60df48800f921..713ca69a72b08 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/server/src/main/java/org/opensearch/indices/IndicesService.java @@ -82,7 +82,6 @@ import org.opensearch.common.util.concurrent.OpenSearchExecutors; import org.opensearch.core.concurrency.OpenSearchRejectedExecutionException; import org.opensearch.common.util.concurrent.OpenSearchThreadPoolExecutor; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.iterable.Iterables; import org.opensearch.common.util.set.Sets; import org.opensearch.common.xcontent.LoggingDeprecationHandler; @@ -926,9 +925,7 @@ private synchronized IndexService createIndexService( indexModule.addIndexOperationListener(operationListener); } pluginsService.onIndexModule(indexModule); - if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS)) { - extensionsManager.onIndexModule(indexModule); - } + extensionsManager.onIndexModule(indexModule); for (IndexEventListener listener : builtInListeners) { indexModule.addIndexEventListener(listener); } diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index fc65d2e9b5d08..761cea003707d 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -466,7 +466,7 @@ protected Node( final IdentityService identityService = new IdentityService(settings, identityPlugins); if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS)) { - this.extensionsManager = new ExtensionsManager(tmpSettings, initialEnvironment.extensionDir()); + this.extensionsManager = new ExtensionsManager(initialEnvironment.extensionDir()); } else { this.extensionsManager = new NoopExtensionsManager(); } @@ -875,16 +875,14 @@ protected Node( ); TopNSearchTasksLogger taskConsumer = new TopNSearchTasksLogger(settings, settingsModule.getClusterSettings()); transportService.getTaskManager().registerTaskResourceConsumer(taskConsumer); - if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS)) { - this.extensionsManager.initializeServicesAndRestHandler( - actionModule, - settingsModule, - transportService, - clusterService, - environment.settings(), - client - ); - } + this.extensionsManager.initializeServicesAndRestHandler( + actionModule, + settingsModule, + transportService, + clusterService, + environment.settings(), + client + ); final GatewayMetaState gatewayMetaState = new GatewayMetaState(); final ResponseCollectorService responseCollectorService = new ResponseCollectorService(clusterService); final SearchTransportService searchTransportService = new SearchTransportService( @@ -1317,9 +1315,7 @@ public Node start() throws NodeValidationException { assert clusterService.localNode().equals(localNodeFactory.getNode()) : "clusterService has a different local node than the factory provided"; transportService.acceptIncomingRequests(); - if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS)) { - extensionsManager.initialize(); - } + extensionsManager.initialize(); discovery.startInitialJoin(); final TimeValue initialStateTimeout = DiscoverySettings.INITIAL_STATE_TIMEOUT_SETTING.get(settings()); configureNodeAndClusterIdStateListener(clusterService); diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java index f218895754b7f..3f5ef4b824afa 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java @@ -157,7 +157,6 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.transport.TransportAddress; import org.opensearch.common.util.BigArrays; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.PageCacheRecycler; import org.opensearch.common.util.concurrent.AbstractRunnable; import org.opensearch.common.util.concurrent.PrioritizedOpenSearchThreadPoolExecutor; @@ -1804,82 +1803,43 @@ public void onFailure(final Exception e) { final SetOnce repositoriesServiceReference = new SetOnce<>(); repositoriesServiceReference.set(repositoriesService); FileCacheCleaner fileCacheCleaner = new FileCacheCleaner(nodeEnv, null); - if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS)) { - indicesService = new IndicesService( - settings, - mock(PluginsService.class), - mock(ExtensionsManager.class), - nodeEnv, - namedXContentRegistry, - new AnalysisRegistry( - environment, - emptyMap(), - emptyMap(), - emptyMap(), - emptyMap(), - emptyMap(), - emptyMap(), - emptyMap(), - emptyMap(), - emptyMap() - ), - indexNameExpressionResolver, - mapperRegistry, - namedWriteableRegistry, - threadPool, - indexScopedSettings, - new NoneCircuitBreakerService(), - bigArrays, - scriptService, - clusterService, - client, - new MetaStateService(nodeEnv, namedXContentRegistry), - Collections.emptyList(), + indicesService = new IndicesService( + settings, + mock(PluginsService.class), + mock(ExtensionsManager.class), + nodeEnv, + namedXContentRegistry, + new AnalysisRegistry( + environment, emptyMap(), - null, emptyMap(), - new RemoteSegmentStoreDirectoryFactory(() -> repositoriesService), - repositoriesServiceReference::get, - fileCacheCleaner - ); - } else { - indicesService = new IndicesService( - settings, - mock(PluginsService.class), - nodeEnv, - namedXContentRegistry, - new AnalysisRegistry( - environment, - emptyMap(), - emptyMap(), - emptyMap(), - emptyMap(), - emptyMap(), - emptyMap(), - emptyMap(), - emptyMap(), - emptyMap() - ), - indexNameExpressionResolver, - mapperRegistry, - namedWriteableRegistry, - threadPool, - indexScopedSettings, - new NoneCircuitBreakerService(), - bigArrays, - scriptService, - clusterService, - client, - new MetaStateService(nodeEnv, namedXContentRegistry), - Collections.emptyList(), emptyMap(), - null, emptyMap(), - new RemoteSegmentStoreDirectoryFactory(() -> repositoriesService), - repositoriesServiceReference::get, - fileCacheCleaner - ); - } + emptyMap(), + emptyMap(), + emptyMap(), + emptyMap(), + emptyMap() + ), + indexNameExpressionResolver, + mapperRegistry, + namedWriteableRegistry, + threadPool, + indexScopedSettings, + new NoneCircuitBreakerService(), + bigArrays, + scriptService, + clusterService, + client, + new MetaStateService(nodeEnv, namedXContentRegistry), + Collections.emptyList(), + emptyMap(), + null, + emptyMap(), + new RemoteSegmentStoreDirectoryFactory(() -> repositoriesService), + repositoriesServiceReference::get, + fileCacheCleaner + ); final RecoverySettings recoverySettings = new RecoverySettings(settings, clusterSettings); snapshotShardsService = new SnapshotShardsService( settings, From ef079d8da142109443b2000a1a3c95e1d7423a2c Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Tue, 2 May 2023 23:39:55 +0000 Subject: [PATCH 2/5] Remove unneeded duplicate constructors Signed-off-by: Peter Nied --- .../extensions/ExtensionsManager.java | 2 +- .../extensions/NoopExtensionsManager.java | 11 ++ .../opensearch/indices/IndicesService.java | 116 ------------------ .../main/java/org/opensearch/node/Node.java | 81 ++++-------- 4 files changed, 38 insertions(+), 172 deletions(-) diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java index e099098d9c231..94ad9ff84cfdb 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java @@ -294,7 +294,7 @@ private void registerRequestHandler() { /* * Load and populate all extensions */ - private void discover() throws IOException { + protected void discover() throws IOException { logger.info("Loading extensions : {}", extensionsPath); if (!FileSystemUtils.isAccessibleDirectory(extensionsPath, logger)) { return; diff --git a/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java index 0f069d5395e8c..6165423b767ce 100644 --- a/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java @@ -36,6 +36,7 @@ public NoopExtensionsManager() throws IOException { super(Path.of("")); } + @Override public void initializeServicesAndRestHandler( ActionModule actionModule, SettingsModule settingsModule, @@ -47,24 +48,34 @@ public void initializeServicesAndRestHandler( // no-op } + @Override public RemoteExtensionActionResponse handleRemoteTransportRequest(ExtensionActionRequest request) throws Exception { // no-op empty response return new RemoteExtensionActionResponse(true, new byte[0]); } + @Override public ExtensionActionResponse handleTransportRequest(ExtensionActionRequest request) throws Exception { // no-op empty response return new ExtensionActionResponse(new byte[0]); } + @Override + protected void discover() throws IOException { + // no-op + } + + @Override public void initialize() { // no-op } + @Override public void onIndexModule(IndexModule indexModule) throws UnknownHostException { // no-op } + @Override public Optional lookupInitializedExtensionById(final String extensionId) { // no-op not found return Optional.empty(); diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index 713ca69a72b08..b3843dfd114a9 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/server/src/main/java/org/opensearch/indices/IndicesService.java @@ -338,122 +338,6 @@ protected void doStart() { threadPool.schedule(this.cacheCleaner, this.cleanInterval, ThreadPool.Names.SAME); } - 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, - FileCacheCleaner fileCacheCleaner - ) { - this.settings = settings; - this.threadPool = threadPool; - this.pluginsService = pluginsService; - this.extensionsManager = null; - this.nodeEnv = nodeEnv; - this.xContentRegistry = xContentRegistry; - this.valuesSourceRegistry = valuesSourceRegistry; - this.shardsClosedTimeout = settings.getAsTime(INDICES_SHARDS_CLOSED_TIMEOUT, new TimeValue(1, TimeUnit.DAYS)); - this.analysisRegistry = analysisRegistry; - this.indexNameExpressionResolver = indexNameExpressionResolver; - this.indicesRequestCache = new IndicesRequestCache(settings); - this.indicesQueryCache = new IndicesQueryCache(settings); - this.mapperRegistry = mapperRegistry; - this.namedWriteableRegistry = namedWriteableRegistry; - indexingMemoryController = new IndexingMemoryController( - settings, - threadPool, - // ensure we pull an iter with new shards - flatten makes a copy - () -> Iterables.flatten(this).iterator() - ); - this.indexScopedSettings = indexScopedSettings; - this.circuitBreakerService = circuitBreakerService; - this.bigArrays = bigArrays; - this.scriptService = scriptService; - this.clusterService = clusterService; - this.client = client; - this.idFieldDataEnabled = INDICES_ID_FIELD_DATA_ENABLED_SETTING.get(clusterService.getSettings()); - clusterService.getClusterSettings().addSettingsUpdateConsumer(INDICES_ID_FIELD_DATA_ENABLED_SETTING, this::setIdFieldDataEnabled); - this.indicesFieldDataCache = new IndicesFieldDataCache(settings, new IndexFieldDataCache.Listener() { - @Override - public void onRemoval(ShardId shardId, String fieldName, boolean wasEvicted, long sizeInBytes) { - assert sizeInBytes >= 0 : "When reducing circuit breaker, it should be adjusted with a number higher or " - + "equal to 0 and not [" - + sizeInBytes - + "]"; - circuitBreakerService.getBreaker(CircuitBreaker.FIELDDATA).addWithoutBreaking(-sizeInBytes); - } - }); - this.cleanInterval = INDICES_CACHE_CLEAN_INTERVAL_SETTING.get(settings); - this.cacheCleaner = new CacheCleaner(indicesFieldDataCache, indicesRequestCache, logger, threadPool, this.cleanInterval); - this.metaStateService = metaStateService; - this.engineFactoryProviders = engineFactoryProviders; - - this.directoryFactories = directoryFactories; - this.recoveryStateFactories = recoveryStateFactories; - this.fileCacheCleaner = fileCacheCleaner; - // 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 - // ref count which will only close them when both this service and all index services are - // actually closed - indicesRefCount = new AbstractRefCounted("indices") { - @Override - protected void closeInternal() { - try { - IOUtils.close( - analysisRegistry, - indexingMemoryController, - indicesFieldDataCache, - cacheCleaner, - indicesRequestCache, - indicesQueryCache - ); - } catch (IOException e) { - throw new UncheckedIOException(e); - } finally { - closeLatch.countDown(); - } - } - }; - - final String nodeName = Objects.requireNonNull(Node.NODE_NAME_SETTING.get(settings)); - nodeWriteDanglingIndicesInfo = WRITE_DANGLING_INDICES_INFO_SETTING.get(settings); - danglingIndicesThreadPoolExecutor = nodeWriteDanglingIndicesInfo - ? OpenSearchExecutors.newScaling( - nodeName + "/" + DANGLING_INDICES_UPDATE_THREAD_NAME, - 1, - 1, - 0, - TimeUnit.MILLISECONDS, - daemonThreadFactory(nodeName, DANGLING_INDICES_UPDATE_THREAD_NAME), - threadPool.getThreadContext() - ) - : null; - - this.allowExpensiveQueries = ALLOW_EXPENSIVE_QUERIES.get(clusterService.getSettings()); - clusterService.getClusterSettings().addSettingsUpdateConsumer(ALLOW_EXPENSIVE_QUERIES, this::setAllowExpensiveQueries); - this.remoteDirectoryFactory = remoteDirectoryFactory; - this.translogFactorySupplier = getTranslogFactorySupplier(repositoriesServiceSupplier, threadPool); - } - public IndicesService( Settings settings, PluginsService pluginsService, diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 761cea003707d..3827041a60aa3 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -704,61 +704,32 @@ protected Node( repositoriesServiceReference::get ); - final IndicesService indicesService; - if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS)) { - indicesService = new IndicesService( - settings, - pluginsService, - extensionsManager, - nodeEnvironment, - xContentRegistry, - analysisModule.getAnalysisRegistry(), - clusterModule.getIndexNameExpressionResolver(), - indicesModule.getMapperRegistry(), - namedWriteableRegistry, - threadPool, - settingsModule.getIndexScopedSettings(), - circuitBreakerService, - bigArrays, - scriptService, - clusterService, - client, - metaStateService, - engineFactoryProviders, - Map.copyOf(directoryFactories), - searchModule.getValuesSourceRegistry(), - recoveryStateFactories, - remoteDirectoryFactory, - repositoriesServiceReference::get, - fileCacheCleaner - ); - } else { - indicesService = new IndicesService( - settings, - pluginsService, - nodeEnvironment, - xContentRegistry, - analysisModule.getAnalysisRegistry(), - clusterModule.getIndexNameExpressionResolver(), - indicesModule.getMapperRegistry(), - namedWriteableRegistry, - threadPool, - settingsModule.getIndexScopedSettings(), - circuitBreakerService, - bigArrays, - scriptService, - clusterService, - client, - metaStateService, - engineFactoryProviders, - Map.copyOf(directoryFactories), - searchModule.getValuesSourceRegistry(), - recoveryStateFactories, - remoteDirectoryFactory, - repositoriesServiceReference::get, - fileCacheCleaner - ); - } + final IndicesService indicesService = new IndicesService( + settings, + pluginsService, + extensionsManager, + nodeEnvironment, + xContentRegistry, + analysisModule.getAnalysisRegistry(), + clusterModule.getIndexNameExpressionResolver(), + indicesModule.getMapperRegistry(), + namedWriteableRegistry, + threadPool, + settingsModule.getIndexScopedSettings(), + circuitBreakerService, + bigArrays, + scriptService, + clusterService, + client, + metaStateService, + engineFactoryProviders, + Map.copyOf(directoryFactories), + searchModule.getValuesSourceRegistry(), + recoveryStateFactories, + remoteDirectoryFactory, + repositoriesServiceReference::get, + fileCacheCleaner + ); final AliasValidator aliasValidator = new AliasValidator(); From d087609b99dcda72a6df601089c05b2a8f3726d5 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Tue, 2 May 2023 23:48:34 +0000 Subject: [PATCH 3/5] Update constructor usage on ExtensionManager Signed-off-by: Peter Nied --- .../extensions/ExtensionsManagerTests.java | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index 579b6dbf3f08f..ec8e0c9676574 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -201,7 +201,7 @@ public void tearDown() throws Exception { public void testDiscover() throws Exception { Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(settings, extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); List expectedExtensions = new ArrayList(); @@ -253,7 +253,7 @@ public void testNonUniqueExtensionsDiscovery() throws Exception { .collect(Collectors.toList()); Files.write(emptyExtensionDir.resolve("extensions.yml"), nonUniqueYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(settings, emptyExtensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(emptyExtensionDir); List expectedExtensions = new ArrayList(); @@ -302,7 +302,7 @@ public void testMissingRequiredFieldsInExtensionDiscovery() throws Exception { ) ); - extensionsManager = new ExtensionsManager(settings, emptyExtensionDir); + extensionsManager = new ExtensionsManager(emptyExtensionDir); mockLogAppender.assertAllExpectationsMatched(); } @@ -387,7 +387,7 @@ public void testNonAccessibleDirectory() throws Exception { AccessControlException e = expectThrows( AccessControlException.class, - () -> new ExtensionsManager(settings, PathUtils.get("")) + () -> new ExtensionsManager(PathUtils.get("")) ); assertEquals("access denied (\"java.io.FilePermission\" \"\" \"read\")", e.getMessage()); } @@ -406,7 +406,7 @@ public void testNoExtensionsFile() throws Exception { ) ); - new ExtensionsManager(settings, extensionDir); + new ExtensionsManager(extensionDir); mockLogAppender.assertAllExpectationsMatched(); } @@ -420,12 +420,12 @@ public void testEmptyExtensionsFile() throws Exception { Settings settings = Settings.builder().build(); - expectThrows(IOException.class, () -> new ExtensionsManager(settings, emptyExtensionDir)); + expectThrows(IOException.class, () -> new ExtensionsManager( emptyExtensionDir)); } public void testInitialize() throws Exception { Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(settings, extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); initialize(extensionsManager); @@ -468,7 +468,7 @@ public void testInitialize() throws Exception { public void testHandleRegisterRestActionsRequest() throws Exception { Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(settings, extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); initialize(extensionsManager); String uniqueIdStr = "uniqueid1"; @@ -483,7 +483,7 @@ public void testHandleRegisterRestActionsRequest() throws Exception { public void testHandleRegisterSettingsRequest() throws Exception { Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(settings, extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); initialize(extensionsManager); String uniqueIdStr = "uniqueid1"; @@ -499,7 +499,7 @@ public void testHandleRegisterSettingsRequest() throws Exception { } public void testHandleRegisterRestActionsRequestWithInvalidMethod() throws Exception { - ExtensionsManager extensionsManager = new ExtensionsManager(settings, extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); initialize(extensionsManager); String uniqueIdStr = "uniqueid1"; @@ -513,7 +513,7 @@ public void testHandleRegisterRestActionsRequestWithInvalidMethod() throws Excep } public void testHandleRegisterRestActionsRequestWithInvalidDeprecatedMethod() throws Exception { - ExtensionsManager extensionsManager = new ExtensionsManager(settings, extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); initialize(extensionsManager); String uniqueIdStr = "uniqueid1"; @@ -527,7 +527,7 @@ public void testHandleRegisterRestActionsRequestWithInvalidDeprecatedMethod() th } public void testHandleRegisterRestActionsRequestWithInvalidUri() throws Exception { - ExtensionsManager extensionsManager = new ExtensionsManager(settings, extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); initialize(extensionsManager); String uniqueIdStr = "uniqueid1"; List actionsList = List.of("GET", "PUT /bar", "POST /baz"); @@ -540,7 +540,7 @@ public void testHandleRegisterRestActionsRequestWithInvalidUri() throws Exceptio } public void testHandleRegisterRestActionsRequestWithInvalidDeprecatedUri() throws Exception { - ExtensionsManager extensionsManager = new ExtensionsManager(settings, extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); initialize(extensionsManager); String uniqueIdStr = "uniqueid1"; List actionsList = List.of("GET /foo", "PUT /bar", "POST /baz"); @@ -553,7 +553,7 @@ public void testHandleRegisterRestActionsRequestWithInvalidDeprecatedUri() throw } public void testHandleExtensionRequest() throws Exception { - ExtensionsManager extensionsManager = new ExtensionsManager(settings, extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); initialize(extensionsManager); ExtensionRequest clusterStateRequest = new ExtensionRequest(ExtensionRequestProto.RequestType.REQUEST_EXTENSION_CLUSTER_STATE); @@ -709,7 +709,7 @@ public void testEnvironmentSettingsDefaultValue() throws Exception { public void testAddSettingsUpdateConsumerRequest() throws Exception { Path extensionDir = createTempDir(); Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(settings, extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); initialize(extensionsManager); List> componentSettings = List.of( @@ -756,7 +756,7 @@ public void testHandleAddSettingsUpdateConsumerRequest() throws Exception { Path extensionDir = createTempDir(); Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(settings, extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); initialize(extensionsManager); List> componentSettings = List.of( @@ -778,7 +778,7 @@ public void testHandleAddSettingsUpdateConsumerRequest() throws Exception { public void testUpdateSettingsRequest() throws Exception { Path extensionDir = createTempDir(); Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(settings, extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); initialize(extensionsManager); Setting componentSetting = Setting.boolSetting("falseSetting", false, Property.Dynamic); @@ -807,7 +807,7 @@ public void testUpdateSettingsRequest() throws Exception { public void testRegisterHandler() throws Exception { - ExtensionsManager extensionsManager = new ExtensionsManager(settings, extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); TransportService mockTransportService = spy( new TransportService( @@ -834,7 +834,7 @@ public void testRegisterHandler() throws Exception { public void testOnIndexModule() throws Exception { Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(settings, extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); initialize(extensionsManager); Environment environment = TestEnvironment.newEnvironment(settings); @@ -891,7 +891,7 @@ public void testIncompatibleExtensionRegistration() throws IOException, IllegalA ); Files.write(extensionDir.resolve("extensions.yml"), incompatibleExtension, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(settings, extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); assertEquals(0, extensionsManager.getExtensionIdMap().values().size()); mockLogAppender.assertAllExpectationsMatched(); } From 480b8e87ea448412836f436b34c651f3a9321521 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Tue, 2 May 2023 23:53:28 +0000 Subject: [PATCH 4/5] Add changelog entry Signed-off-by: Peter Nied --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b2d46e7c50d94..a5c7184461822 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -94,6 +94,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Changed - Enable `./gradlew build` on MacOS by disabling bcw tests ([#7303](https://github.com/opensearch-project/OpenSearch/pull/7303)) +- Cleanup interfaces around ExtensionManager ([#7374](https://github.com/opensearch-project/OpenSearch/pull/7374)) ### Deprecated From 51cf07cc956b2e9f709057004c7616629e5b0c49 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Wed, 3 May 2023 00:19:02 +0000 Subject: [PATCH 5/5] Fix style issue Signed-off-by: Peter Nied --- .../java/org/opensearch/extensions/ExtensionsManagerTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index ec8e0c9676574..0a9fd2e6b94fe 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -420,7 +420,7 @@ public void testEmptyExtensionsFile() throws Exception { Settings settings = Settings.builder().build(); - expectThrows(IOException.class, () -> new ExtensionsManager( emptyExtensionDir)); + expectThrows(IOException.class, () -> new ExtensionsManager(emptyExtensionDir)); } public void testInitialize() throws Exception {