Skip to content

IndexShard state should also be started.#38630

Merged
martijnvg merged 5 commits intoelastic:masterfrom
martijnvg:fixCcrRestIndexFallBehindTest2
Feb 11, 2019
Merged

IndexShard state should also be started.#38630
martijnvg merged 5 commits intoelastic:masterfrom
martijnvg:fixCcrRestIndexFallBehindTest2

Conversation

@martijnvg
Copy link
Copy Markdown
Member

Fixes #38617 (the test also failed in 8.0 / 7.x, but happens less likely)

@martijnvg martijnvg added >test Issues or PRs that are addressing/adding tests :Distributed/CCR Issues around the Cross Cluster State Replication features v8.0.0 labels Feb 8, 2019
@martijnvg martijnvg requested a review from dnhatn February 8, 2019 15:15
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-distributed

@martijnvg
Copy link
Copy Markdown
Member Author

Note this change should be backported to 7.x, 7.0 and 6.7 branches. The IndexFollowingIT#testIndexFallBehind() test has been muted on 7.0 and 6.7 branches, because it failed too often there.

Copy link
Copy Markdown
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

Thanks @martijnvg. I left a comment.

.indexServiceSafe(shardRouting.index()).getShard(shardRouting.id());
if (indexShard.state() != IndexShardState.STARTED) {
continue;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should catch "AlreadyClosedException" when calling IndexShardTestCase.getDocIdAndSeqNos(indexShard) instead?

@martijnvg
Copy link
Copy Markdown
Member Author

@dnhatn I've updated the PR.

Copy link
Copy Markdown
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @martijnvg.

@jasontedor
Copy link
Copy Markdown
Member

@martijnvg Does this need to be backported to 6.7?

@martijnvg
Copy link
Copy Markdown
Member Author

@jasontedor Yes, and it was backported: 232d2e0

@martijnvg
Copy link
Copy Markdown
Member Author

I just forgot to update the version labels here.

@jasontedor
Copy link
Copy Markdown
Member

Okay, I think that I see what happened. The test is still marked as awaits fix in the 6.7 and 7.0 branch. I think that is the case because they were never marked as awaits fix in master, so that when you developed the fix you didn't have to remove the awaits fix, and hence no removal of the awaits fix when you cherry picked back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/CCR Issues around the Cross Cluster State Replication features >test Issues or PRs that are addressing/adding tests v6.7.0 v7.0.0-rc1 v7.2.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] IndexFollowingIT.testIndexFallBehind some times failes on 6.7

6 participants