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

Avoid auto following leader system indices in CCR #72815

Merged
merged 10 commits into from Jul 9, 2021

Conversation

fcofdez
Copy link
Contributor

@fcofdez fcofdez commented May 6, 2021

Relates #67686

@fcofdez fcofdez added >bug :Distributed/CCR Issues around the Cross Cluster State Replication features v8.0.0 Team:Distributed Meta label for distributed team v7.14.0 labels May 6, 2021
@elasticmachine
Copy link
Collaborator

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

@henningandersen
Copy link
Contributor

Related #67686

@henningandersen
Copy link
Contributor

@jaymode I kind of regard this a bugfix, but to some extent it is only a bugfix towards a concept introduced in 7.x (system indices) so I suppose we could also regard this a breaking change requiring some level of BWC (like adding a flag to specify the behavior and remove this in 8.0). I seek your guidance on this subject given your experience with other system index related changes.

@jaymode
Copy link
Member

jaymode commented May 12, 2021

Is it possible to make this change 8.0 only and just have a deprecation log/warning emitted in 7.x? I think we may find some users doing this for system indices like Kibana even if we do not advise it and it would be best not to break them in 7.x.

@henningandersen
Copy link
Contributor

Yes, that should be possible, @fcofdez will you open a PR to add the deprecations for 7.x (or 8+7.x). Then we can continue on this when that is merged (I prefer to get the deprecation in before doing the breaking change here).

That also means this PR should have a breaking changes section.

Given that we are adding the exclusions option in a separate PR I see no reason to add a flag or similar.

fcofdez added a commit that referenced this pull request Jun 29, 2021
This commits deprecates Auto-Follow of system indices

Relates #72815
@henningandersen
Copy link
Contributor

@fcofdez with #73237 out of the way, I think all that is left here is to add a breaking changes section for the change?

@fcofdez
Copy link
Contributor Author

fcofdez commented Jul 5, 2021

Sorry I lost track of this. I'll update the branch and I'll add the breaking changes section. Thanks for the ping.

@fcofdez
Copy link
Contributor Author

fcofdez commented Jul 7, 2021

@elasticmachine run elasticsearch-ci/docs-check

@fcofdez
Copy link
Contributor Author

fcofdez commented Jul 7, 2021

@henningandersen could you review this when you have the chance? Thanks!

Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM.

assertThat(autoFollowResults.get(0).autoFollowExecutionResults, is(anEmptyMap()));
}

private List<AutoFollowCoordinator.AutoFollowResult> executeAutoFollow(String indexPattern, final ClusterState finalRemoteState) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to add a single test that also uses this method, that actually creates an index. It is sort of a test of the test, but given the complexity of this test method and the fact that if it just returns nothing, we are good, I think it is worthwhile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in d8a7c41

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking >bug :Distributed/CCR Issues around the Cross Cluster State Replication features Team:Distributed Meta label for distributed team v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants