Skip to content

Conversation

@ywelsch
Copy link
Contributor

@ywelsch ywelsch commented Dec 20, 2019

Fixes the muted test "testAutoExpandIndicesDuringRollingUpgrade". We can't wait in the test for the index to be green, as we have put a filter exclusion into place that prevents all shards from being allocated after a node rejoins. Instead we check whether the correct auto-expansion has taken place. We need to do this in an assertBusy, as older versions of ES are not auto-expanding indices while a node is joining, but only doing this as a follow-up step.

Closes #50426

@ywelsch ywelsch added >test Issues or PRs that are addressing/adding tests :Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) v7.6.0 labels Dec 20, 2019
@elasticmachine
Copy link
Collaborator

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

@ywelsch ywelsch requested a review from DaveCTurner December 20, 2019 12:22
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.

I left a question

assertBusy(() -> {
final int numberOfReplicas = Integer.parseInt(
getIndexSettingsAsMap(indexName).get(IndexMetaData.SETTING_NUMBER_OF_REPLICAS).toString());
if (minimumNodeVersion.onOrAfter(Version.V_7_6_0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the assertBusy() if minimumNodeVersion.onOrAfter(Version.V_7_6_0) or can we ensureGreen() in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the question. We actually don't need assertBusy at all (#30423 was already introduced in 6.4, I misremembered this).

The ensureGreen is mostly for cosmetics. I've added it for the case where we are on 7.6.0+

@ywelsch ywelsch requested a review from DaveCTurner December 20, 2019 15:08
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.

LGTM

@ywelsch ywelsch merged commit c37c53a into elastic:7.x Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed Coordination/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) >test Issues or PRs that are addressing/adding tests v7.6.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants