Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Make NodeEnvironment.indexPaths singular #72438

Merged
merged 2 commits into from
Apr 29, 2021
Merged

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Apr 29, 2021

This commit renames the indexPaths method to be singular and return a
single Path instead of an array. This is done in isolation from other
NodeEnvironment methods to make it reviewable.

relates #71205

This commit renames the indexPaths method to be singular and return a
single Path instead of an array. This is done in isolation from other
NodeEnvironment methods to make it reviewable.

relates elastic#71205
@rjernst rjernst added :Core/Infra/Core Core issues without another label >refactoring v8.0.0 labels Apr 29, 2021
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Apr 29, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

Copy link
Contributor

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one optional suggestion which could be done in a followup instead.

@@ -274,12 +265,11 @@ protected void doRun() throws Exception {

final SetOnce<Path[]> listener = new SetOnce<>();
env.deleteIndexDirectorySafe(index, 5000, idxSettings, listener::set);
assertArrayEquals(env.indexPaths(index), listener.get());
assertThat(listener.get()[0], equalTo(env.indexPath(index)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to make this listener a Consumer<Path> too to get rid of this subscript (or else assert listener.get().length == 1 maybe?). It's actually only used in this one place, all other consumers just ignore it:

diff --git a/server/src/internalClusterTest/java/org/elasticsearch/plugins/IndexFoldersDeletionListenerIT.java b/server/src/internalClusterTest/java/org/elasticsearch/plugins/IndexFoldersDeletionListenerIT.java
index 7f44fe47a19..8490e0d6c47 100644
--- a/server/src/internalClusterTest/java/org/elasticsearch/plugins/IndexFoldersDeletionListenerIT.java
+++ b/server/src/internalClusterTest/java/org/elasticsearch/plugins/IndexFoldersDeletionListenerIT.java
@@ -325,7 +325,7 @@ public class IndexFoldersDeletionListenerIT extends ESIntegTestCase {
         public List<IndexFoldersDeletionListener> getIndexFoldersDeletionListeners() {
             return List.of(new IndexFoldersDeletionListener() {
                 @Override
-                public void beforeIndexFoldersDeleted(Index index, IndexSettings indexSettings, Path[] indexPaths) {
+                public void beforeIndexFoldersDeleted(Index index, IndexSettings indexSettings) {
                     deletedIndices.add(index);
                 }
 
diff --git a/server/src/main/java/org/elasticsearch/env/NodeEnvironment.java b/server/src/main/java/org/elasticsearch/env/NodeEnvironment.java
index b0686adf8cc..d970fe138e7 100644
--- a/server/src/main/java/org/elasticsearch/env/NodeEnvironment.java
+++ b/server/src/main/java/org/elasticsearch/env/NodeEnvironment.java
@@ -670,7 +670,7 @@ public final class NodeEnvironment  implements Closeable {
         Index index,
         long lockTimeoutMS,
         IndexSettings indexSettings,
-        Consumer<Path[]> listener
+        Consumer<Path> listener
     ) throws IOException, ShardLockObtainFailedException {
         final List<ShardLock> locks = lockAllForIndex(index, indexSettings, "deleting index directory", lockTimeoutMS);
         try {
@@ -687,15 +687,15 @@ public final class NodeEnvironment  implements Closeable {
      * @param index the index to delete
      * @param indexSettings settings for the index being deleted
      */
-    public void deleteIndexDirectoryUnderLock(Index index, IndexSettings indexSettings, Consumer<Path[]> listener) throws IOException {
+    public void deleteIndexDirectoryUnderLock(Index index, IndexSettings indexSettings, Consumer<Path> listener) throws IOException {
         final Path indexPath = indexPath(index);
         logger.trace("deleting index {} directory: [{}]", index, indexPath);
-        listener.accept(new Path[] { indexPath });
+        listener.accept(indexPath);
         IOUtils.rm(indexPath);
         if (indexSettings.hasCustomDataPath()) {
             Path customLocation = resolveIndexCustomLocation(indexSettings.customDataPath(), index.getUUID());
             logger.trace("deleting custom index {} directory [{}]", index, customLocation);
-            listener.accept(new Path[]{customLocation});
+            listener.accept(customLocation);
             IOUtils.rm(customLocation);
         }
     }
diff --git a/server/src/main/java/org/elasticsearch/indices/IndicesService.java b/server/src/main/java/org/elasticsearch/indices/IndicesService.java
index 786db1990a0..0e0f19bc49b 100644
--- a/server/src/main/java/org/elasticsearch/indices/IndicesService.java
+++ b/server/src/main/java/org/elasticsearch/indices/IndicesService.java
@@ -921,7 +921,7 @@ public class IndicesService extends AbstractLifecycleComponent
             if (predicate.apply(index, indexSettings)) {
                 // its safe to delete all index metadata and shard data
                 nodeEnv.deleteIndexDirectorySafe(index, 0, indexSettings,
-                    paths -> indexFoldersDeletionListeners.beforeIndexFoldersDeleted(index, indexSettings, paths));
+                    paths -> indexFoldersDeletionListeners.beforeIndexFoldersDeleted(index, indexSettings));
             }
             success = true;
         } catch (ShardLockObtainFailedException ex) {
@@ -1216,7 +1216,7 @@ public class IndicesService extends AbstractLifecycleComponent
                             logger.debug("{} deleting index store reason [{}]", index, "pending delete");
                             try {
                                 nodeEnv.deleteIndexDirectoryUnderLock(index, indexSettings,
-                                    paths -> indexFoldersDeletionListeners.beforeIndexFoldersDeleted(index, indexSettings, paths));
+                                    paths -> indexFoldersDeletionListeners.beforeIndexFoldersDeleted(index, indexSettings));
                                 iterator.remove();
                             } catch (IOException ex) {
                                 logger.debug(() -> new ParameterizedMessage("{} retry pending delete", index), ex);
diff --git a/server/src/main/java/org/elasticsearch/indices/store/CompositeIndexFoldersDeletionListener.java b/server/src/main/java/org/elasticsearch/indices/store/CompositeIndexFoldersDeletionListener.java
index 16ea2601e48..7d9142db8c1 100644
--- a/server/src/main/java/org/elasticsearch/indices/store/CompositeIndexFoldersDeletionListener.java
+++ b/server/src/main/java/org/elasticsearch/indices/store/CompositeIndexFoldersDeletionListener.java
@@ -30,10 +30,10 @@ public class CompositeIndexFoldersDeletionListener implements IndexStorePlugin.I
     }
 
     @Override
-    public void beforeIndexFoldersDeleted(Index index, IndexSettings indexSettings, Path[] indexPaths) {
+    public void beforeIndexFoldersDeleted(Index index, IndexSettings indexSettings) {
         for (IndexStorePlugin.IndexFoldersDeletionListener listener : listeners) {
             try {
-                listener.beforeIndexFoldersDeleted(index, indexSettings, indexPaths);
+                listener.beforeIndexFoldersDeleted(index, indexSettings);
             } catch (Exception e) {
                 assert false : new AssertionError(e);
                 throw e;
diff --git a/server/src/main/java/org/elasticsearch/plugins/IndexStorePlugin.java b/server/src/main/java/org/elasticsearch/plugins/IndexStorePlugin.java
index 36b644b882f..f116a9a833b 100644
--- a/server/src/main/java/org/elasticsearch/plugins/IndexStorePlugin.java
+++ b/server/src/main/java/org/elasticsearch/plugins/IndexStorePlugin.java
@@ -88,9 +88,8 @@ public interface IndexStorePlugin {
          *
          * @param index         the {@link Index} of the index whose folders are going to be deleted
          * @param indexSettings settings for the index whose folders are going to be deleted
-         * @param indexPaths    the paths of the folders that are going to be deleted
          */
-        void beforeIndexFoldersDeleted(Index index, IndexSettings indexSettings, Path[] indexPaths);
+        void beforeIndexFoldersDeleted(Index index, IndexSettings indexSettings);
 
         /**
          * Invoked before the folders of a shard are deleted from disk. The list of folders contains {@link Path}s that may or may not
diff --git a/server/src/test/java/org/elasticsearch/env/NodeEnvironmentTests.java b/server/src/test/java/org/elasticsearch/env/NodeEnvironmentTests.java
index d8a10232420..44ed1e95ff3 100644
--- a/server/src/test/java/org/elasticsearch/env/NodeEnvironmentTests.java
+++ b/server/src/test/java/org/elasticsearch/env/NodeEnvironmentTests.java
@@ -263,9 +263,9 @@ public class NodeEnvironmentTests extends ESTestCase {
         start.countDown();
         blockLatch.await();
 
-        final SetOnce<Path[]> listener = new SetOnce<>();
+        final SetOnce<Path> listener = new SetOnce<>();
         env.deleteIndexDirectorySafe(index, 5000, idxSettings, listener::set);
-        assertThat(listener.get()[0], equalTo(env.indexPath(index)));
+        assertThat(listener.get(), equalTo(env.indexPath(index)));
         assertNull(threadException.get());
 
         path = env.indexPath(index);
diff --git a/server/src/test/java/org/elasticsearch/index/IndexModuleTests.java b/server/src/test/java/org/elasticsearch/index/IndexModuleTests.java
index 879943ad8cf..b46b46a9b5e 100644
--- a/server/src/test/java/org/elasticsearch/index/IndexModuleTests.java
+++ b/server/src/test/java/org/elasticsearch/index/IndexModuleTests.java
@@ -122,7 +122,7 @@ public class IndexModuleTests extends ESTestCase {
 
     private IndexStorePlugin.IndexFoldersDeletionListener indexDeletionListener = new IndexStorePlugin.IndexFoldersDeletionListener() {
         @Override
-        public void beforeIndexFoldersDeleted(Index index, IndexSettings indexSettings, Path[] indexPaths) {
+        public void beforeIndexFoldersDeleted(Index index, IndexSettings indexSettings) {
         }
 
         @Override
diff --git a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/allocation/SearchableSnapshotIndexFoldersDeletionListener.java b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/allocation/SearchableSnapshotIndexFoldersDeletionListener.java
index 3833b402364..c8a6762ec42 100644
--- a/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/allocation/SearchableSnapshotIndexFoldersDeletionListener.java
+++ b/x-pack/plugin/searchable-snapshots/src/main/java/org/elasticsearch/xpack/searchablesnapshots/allocation/SearchableSnapshotIndexFoldersDeletionListener.java
@@ -45,7 +45,7 @@ public class SearchableSnapshotIndexFoldersDeletionListener implements IndexStor
     }
 
     @Override
-    public void beforeIndexFoldersDeleted(Index index, IndexSettings indexSettings, Path[] indexPaths) {
+    public void beforeIndexFoldersDeleted(Index index, IndexSettings indexSettings) {
         if (SearchableSnapshotsConstants.isSearchableSnapshotStore(indexSettings.getSettings())) {
             for (int shard = 0; shard < indexSettings.getNumberOfShards(); shard++) {
                 markShardAsEvictedInCache(new ShardId(index, shard), indexSettings);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to change all of these consumers in a followup. There are several similar ones.

@rjernst rjernst merged commit 2a446ad into elastic:master Apr 29, 2021
@rjernst rjernst deleted the mdp17 branch April 29, 2021 18:57
@rjernst rjernst mentioned this pull request Sep 30, 2021
17 tasks
rjernst added a commit to rjernst/elasticsearch that referenced this pull request Oct 12, 2021
This reverts commit 2a446ad.

This revert was conflict free.

relates elastic#78525
relates elastic#71205
rjernst added a commit that referenced this pull request Oct 13, 2021
This reverts commit 2a446ad.

This revert was conflict free.

relates #78525
relates #71205
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >refactoring Team:Core/Infra Meta label for core/infra team v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants