Skip to content

Increment version on system index settings change#84994

Merged
idegtiarenko merged 1 commit intoelastic:masterfrom
idegtiarenko:84869
Mar 16, 2022
Merged

Increment version on system index settings change#84994
idegtiarenko merged 1 commit intoelastic:masterfrom
idegtiarenko:84869

Conversation

@idegtiarenko
Copy link
Copy Markdown
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/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. Team:Distributed Meta label for distributed team. v8.2.0 labels Mar 15, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Contributor

@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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. Team:Distributed Meta label for distributed team. >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'

5 participants