-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Fix testCancelUnnecessaryRequests #124864
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
Fix testCancelUnnecessaryRequests #124864
Conversation
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| } catch (AssertionError e) { | ||
| client().admin().indices().stats(new IndicesStatsRequest()).actionGet().asMap().forEach((shard, stats) -> { | ||
| logger.info( | ||
| "Shard {} node {} status {} docs {}", | ||
| shard.shardId(), | ||
| shard.currentNodeId(), | ||
| shard.state(), | ||
| stats.getStats().getDocs().getCount() | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not bubbling up exception if/when that happens
| stats.getStats().getDocs().getCount() | ||
| ); | ||
| }); | ||
| assertThat(exchanges.get(), lessThanOrEqualTo(2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once above was fixed I noticed that 1-8% depending on the load of the times the test was failing.
Since processing pages and sending more requests happens on different threads of different threadpools, we can not guarantee that incoming page is processed before the next request is sent.
It looks like in practice we accumulate enough results (1 row) before the third request is sent.
This still shows that we do not send request to every node (there are at minimum 3 data nodes in this test).
This test was silently skipping assertion.
Please see inline comments for more details.