Improvements to the IndicesService class#17638
Improvements to the IndicesService class#17638abeyad wants to merge 4 commits intoelastic:masterfrom
Conversation
This commit contains the following improvements/fixes: 1. When a shadow replica index is deleted, its index contents is also deleted. 2. Renaming method names and variables to better reflect the purpose of the method and the semantics of the variable.
| * This method returns true if the current node is allowed to delete the | ||
| * given index. If the index uses a shared filesystem this method always | ||
| * returns false. | ||
| * This method returns true if the current node is allowed to delete the given index. |
There was a problem hiding this comment.
we should also say we return false if the folder doesn't exist
39fec17 to
89aa0ad
Compare
89aa0ad to
62b8eca
Compare
| IOUtils.rm(customLocation); | ||
| // shared shard data on shared file systems might have already been deleted by another node, | ||
| // this can only happen if a shared file system index was closed, then deleted on all nodes | ||
| if (Files.exists(customLocation)) { |
There was a problem hiding this comment.
I rather not make this change (and if we do, let's name the method "delete*IfExists). If I understand correctly it's only needed for shadow indices, which we agreed to tackle in another PR.
|
@abeyad left some initial feedback |
4768f99 to
cc929c8
Compare
cc929c8 to
8dcc6eb
Compare
| assertIndicesDirsDeleted(nodes); | ||
| } | ||
|
|
||
| public void testDeletingIndexWithDedicatedMasterNodes() throws Exception { |
There was a problem hiding this comment.
@bleskes To really test the scenario of dedicated master nodes only with a shadow replica index, it would seem we should simulate the situation where the master nodes don't have the shared data path mounted on its file system. Any thoughts on how to simulate this? Or is it not worth the effort here?
There was a problem hiding this comment.
I don't think we need to get into this now. For now we can remove the test and just keep a test where we have some nodes with no shards assigned to them.
|
@bleskes PR has been updated based on your feedback and our discussion. Mainly, I've just added some tests to |
| //assertIndicesDirsDeleted(nodes); | ||
| } | ||
|
|
||
| public void testNodeJoinsWithoutShadowReplicaConfigured() throws Exception { |
|
Looking good. Left some comments w.r.t tests. |
a12f582 to
51b55c9
Compare
|
@bleskes Updated the PR, and also added this test: https://github.com/elastic/elasticsearch/pull/17638/files#diff-2bb1232b0406b77c64e5285178afb63aR204 |
51b55c9 to
1a3dced
Compare
This commit contains the following improvements/fixes:
of the method and the semantics of the variable.
closedparameter passed to the delete index/store methods with obtaining the index's state from theIndexSettingsthat is already passed in.IndexWithShadowReplicaITsuite, some of which show issues in the shadow replica delete process that are captured in Shadow Replica indexes do not delete properly #17695