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

Increment version on system index settings change #84994

Merged
merged 1 commit into from
Mar 16, 2022

Conversation

idegtiarenko
Copy link
Contributor

The test was failing on assertion around index settings version change
in case actual settings were changed. This change adds missing version
increment and readable messages to the related assertions.

Closes: #84869

The test was failing on assertion around index settings version change
in case actual settings were changed. This change adds missing version
increment and readable messages to the related assertions.
@idegtiarenko idegtiarenko added >test-failure Triaged test failures from CI :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.2.0 labels Mar 15, 2022
@elasticmachine
Copy link
Collaborator

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

Copy link
Member

@original-brownbear original-brownbear left a comment

Choose a reason for hiding this comment

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

LGTM nice find

@idegtiarenko idegtiarenko merged commit 351a410 into elastic:master Mar 16, 2022
@idegtiarenko idegtiarenko deleted the 84869 branch March 16, 2022 07:34
@droberts195
Copy link
Contributor

droberts195 commented Mar 16, 2022

I think this should be backported to 8.1 too, because the change that caused so many build failures that led to this fix (#84780) was backported to 8.1.

The other thing is whether this should be considered a blocker for 8.1.1. That is due to be built today, but if it ships without this fix then production clusters will end up going through the code branch with a failing assertion. They won't trip the assertion as production clusters have assertions disabled, but is it still a major problem that the settings version is wrong? If so then 8.1.1 should be delayed until this fix is in the 8.1 branch.

@idegtiarenko
Copy link
Contributor Author

I think this should be backported to 8.1 too, because the change that caused so many build failures that led to this fix (#84780) was backported to 8.1.

Yes, that fix is merged into 8.1 as well. Let me open a back-port shortly

@idegtiarenko
Copy link
Contributor Author

I am trying to find exact commit/version where the issue was introduced, but I suspect it was prior 8.1.

I believe #84780 is not an actual cause, just a change that un-muted the test that discovered the issue.

@idegtiarenko
Copy link
Contributor Author

Looks like this behavior was originally introduced in #72768 (7.13.1, 7.14.0) and then updated in #79512 (8.0.0-rc, 8.1.0).

@droberts195 should I also back-port it to 7.17?

I am not sure how significant this issue is and if this should become a blocker.

idegtiarenko added a commit to idegtiarenko/elasticsearch that referenced this pull request Mar 16, 2022
The test was failing on assertion around index settings version change
in case actual settings were changed. This change adds missing version
increment and readable messages to the related assertions.

(cherry picked from commit 351a410)
@droberts195
Copy link
Contributor

I believe #84780 is not an actual cause, just a change that un-muted the test that discovered the issue.

OK, fair enough, I guess it's not a blocker then.

idegtiarenko added a commit to idegtiarenko/elasticsearch that referenced this pull request Mar 16, 2022
The test was failing on assertion around index settings version change
in case actual settings were changed. This change adds missing version
increment and readable messages to the related assertions.

(cherry picked from commit 351a410)
elasticsearchmachine pushed a commit that referenced this pull request Mar 16, 2022
The test was failing on assertion around index settings version change
in case actual settings were changed. This change adds missing version
increment and readable messages to the related assertions.

(cherry picked from commit 351a410)
elasticsearchmachine pushed a commit that referenced this pull request Mar 16, 2022
The test was failing on assertion around index settings version change
in case actual settings were changed. This change adds missing version
increment and readable messages to the related assertions.

(cherry picked from commit 351a410)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. >test-failure Triaged test failures from CI v7.17.2 v8.1.1 v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] ':x-pack:qa:full-cluster-restart:v7.5.0#upgradedClusterTest'
4 participants