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

Don’t verify evictions in testFilterCacheStats #42091

Merged
merged 1 commit into from
May 15, 2019

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented May 10, 2019

If a background merge and refresh happens after a search (line 1032) but before a stats query (line 1034), then evictions will be non-zero. I removed these assertions since they are quite brittle.

Closes #32506

@dnhatn dnhatn added >test Issues or PRs that are addressing/adding tests :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. v8.0.0 v7.2.0 labels May 10, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@original-brownbear
Copy link
Member

@dnhatn makes perfect sense to me just removing these. I wonder though for the sake of consistency, can't we just do the same fix I did for the second half of the test in 9c0a429 at the beginning of this test as well and simply force merge to avoid the background merges? (the question is more for my education in case the answer is no, this fix is just fine with me :) as well)

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

@dnhatn
Copy link
Member Author

dnhatn commented May 10, 2019

@original-brownbear I considered your option first then I thought it's more interesting if we let merges happen. I am fine with either way.

@original-brownbear
Copy link
Member

@dnhatn let's keep yours :) no reason to forego the coverage of what happens with ongoing merges then.

@dnhatn
Copy link
Member Author

dnhatn commented May 15, 2019

Thanks @original-brownbear

@dnhatn dnhatn merged commit 230ae18 into elastic:master May 15, 2019
@dnhatn dnhatn deleted the test_evictions branch May 15, 2019 22:13
dnhatn added a commit that referenced this pull request May 15, 2019
If a background merge and refresh happens after a search but before a
stats query, then evictions will be non-zero.

Closes #32506
dnhatn added a commit that referenced this pull request May 15, 2019
If a background merge and refresh happens after a search but before a
stats query, then evictions will be non-zero.

Closes #32506
dnhatn added a commit that referenced this pull request May 16, 2019
If a background merge and refresh happens after a search but before a
stats query, then evictions will be non-zero.

Closes #32506
dnhatn added a commit that referenced this pull request May 16, 2019
If a background merge and refresh happens after a search but before a
stats query, then evictions will be non-zero.

Closes #32506
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
If a background merge and refresh happens after a search but before a
stats query, then evictions will be non-zero.

Closes elastic#32506
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. >test Issues or PRs that are addressing/adding tests v6.8.0 v7.2.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IndexStatsIT#testFilterCacheStats fails
4 participants