Remove dangling indices settings, always import it #10016
Conversation
kimchy
force-pushed
the
always_import_dangled
branch
from
6b56ca5
to
8935710
Compare
| } | ||
| for (String danglingIndex : danglingIndices) { | ||
| if (newMetaData.hasIndex(danglingIndex)) { | ||
| logger.debug("[{}] no longer dangling (created), removing", danglingIndex); |
There was a problem hiding this comment.
I think while we're changing this, it would be nice to change this from "removing" to "removing from dangling index set", otherwise it is kind of confusing about whether data is actually being deleted (which it's not)
|
@dakrone addressed your comments |
| logger.info("[{}] dangling index, exists on local file system, but not in cluster metadata, auto import to cluster state", indexName); | ||
| danglingIndices.add(indexName); | ||
| } else { | ||
| logger.debug("[{}] dangling index directory detected, but no state found"); |
There was a problem hiding this comment.
Missing the indexName argument to the debug log here
|
Left one comment about a missing argument, other than that LGTM |
|
i like the simplifications here. Yet, I think we need to work on how these huge methods are structured and tested. In other classes I already started to add simple package private methods that are individually tested in unittests reducing the size of the loops dramatically. I think we have to break things up into small chunks when we do these kind of refactorings and add real unittests. It's pretty simple to do IMO such that the main look really only processes the end result and loops over the dangeling indices. Can you please simplify, test and document that would be awesome. |
|
@s1monw I was thinking about refactoring it, was thinking of doing it as a next step to keep the change smaller. I updated the pull request with refactoring out the handling of dangling indices into its own class, and it has specific tests for it. Needed to refactor out also the persistence logic of meta state as well. |
| * Iterates over the existing marked dangled indices, and returns the outstanding ones that | ||
| * need to be allocated. | ||
| */ | ||
| IndexMetaData[] outstandingDanglingIndicesToProcess() { |
There was a problem hiding this comment.
Can we just return a List here?
|
left some comments |
kimchy
force-pushed
the
always_import_dangled
branch
from
ac87e9f
to
8fc1172
Compare
kimchy
force-pushed
the
always_import_dangled
branch
from
8fc1172
to
02518a6
Compare
|
@s1monw applied comments, and did another round of simplifications |
| return; | ||
| } | ||
| try { | ||
| allocateDangledIndices.allocateDangled(danglingIndices.values().toArray(new IndexMetaData[danglingIndices.size()]), new LocalAllocateDangledIndices.Listener() { |
There was a problem hiding this comment.
while we are at it - any chance we can just pass danglingIndices.values() to this?
|
left one comment - LGTM |
|
thanks for doing this! |
Remove the settings around dangling indices, such as no import and timeout for deletion, we always want to import dangling indices for safety, and we should not allow to change the behavior. This also cleans up the code quite a bit. closes elastic#10016
kimchy
force-pushed
the
always_import_dangled
branch
from
57c278d
to
13630d8
Compare
clintongormley
added
:Core/Infra/Settings
Remove the settings around dangling indices, such as no import and timeout for deletion, we always want to import dangling indices for safety, and we should not allow to change the behavior. This also cleans up the code quite a bit.