Skip to content

Remove dangling index auto import functionality #59698

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

Merged

Conversation

pugnascotia
Copy link
Contributor

@pugnascotia pugnascotia commented Jul 16, 2020

Closes #48366. Remove all traces of automatically importing dangling indices. This change will not be backported, though this functionality is deprecated as of 7.9.0.

@pugnascotia pugnascotia added >breaking :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. v8.0.0 labels Jul 16, 2020
@pugnascotia pugnascotia requested a review from DaveCTurner July 16, 2020 13:32
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (:Distributed/Recovery)

@elasticmachine elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Jul 16, 2020
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.

Looks good, I left a few very minor comments.

truly represents the latest state of the data when the index was still part
of the cluster.

WARNING: You should avoid situations that result in dangling indices (e.g.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this warning any more; at least, you probably already know you're doing something wrong before you get to the question of importing dangling indices.

*/
void findNewAndAddDanglingIndices(final Metadata metadata) {
final IndexGraveyard graveyard = metadata.indexGraveyard();
// Extracted from getDanglingIndices() as a package-private method to allow easier testing testing
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the tests already mock out the ClusterService, so I think we could pass in the metadata via that mock, have them call getDanglingIndices() and thereby inline this. Or have DanglingIndicesState track a Supplier<ClusterState> rather than the full ClusterService and pass that in for the tests. Not sure which I prefer really, up to you.

private DanglingIndicesState createDanglingIndicesState(NodeEnvironment env, MetaStateService metaStateService) {
final Settings allocateSettings = Settings.builder().put(AUTO_IMPORT_DANGLING_INDICES_SETTING.getKey(), true).build();
private DanglingIndicesState createDanglingIndicesState(MetaStateService metaStateService) {
final Settings allocateSettings = Settings.builder().build();

final ClusterService clusterServiceMock = mock(ClusterService.class);
when(clusterServiceMock.getSettings()).thenReturn(allocateSettings);
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think we need to mock this any more.

Suggested change
when(clusterServiceMock.getSettings()).thenReturn(allocateSettings);

} catch (IOException e) {
logger.warn("failed to list dangling indices", e);
logger.warn("Failed to list dangling indices", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the original:

Suggested change
logger.warn("Failed to list dangling indices", e);
logger.warn("failed to list dangling indices", e);

*/
public class DanglingIndicesState implements ClusterStateListener {
public class DanglingIndicesState {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hurrah!

@pugnascotia
Copy link
Contributor Author

Thanks for the review @DaveCTurner, I've addressed your comments.

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.

Checkstyle seems unhappy but LGTM once CI is green.

@pugnascotia
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1

@pugnascotia pugnascotia merged commit 4db094c into elastic:master Jul 17, 2020
@pugnascotia pugnascotia deleted the remove-dangling-index-auto-import-v2 branch July 17, 2020 14:18
jrodewig added a commit that referenced this pull request Sep 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking :Distributed Indexing/Recovery Anything around constructing a new shard, either from a local or a remote source. Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle dangling indices more safely
4 participants