Make NodeEnvironment.indexPaths singular#72438
Merged
rjernst merged 2 commits intoelastic:masterfrom Apr 29, 2021
Merged
Conversation
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
Collaborator
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
DaveCTurner
approved these changes
Apr 29, 2021
Member
DaveCTurner
left a comment
There was a problem hiding this comment.
LGTM, one optional suggestion which could be done in a followup instead.
| 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))); |
Member
There was a problem hiding this comment.
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);
Member
Author
There was a problem hiding this comment.
I plan to change all of these consumers in a followup. There are several similar ones.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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