-
Notifications
You must be signed in to change notification settings - Fork 25.6k
ESQL: Speed up spec tests #131949
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
ESQL: Speed up spec tests #131949
Conversation
Speeds up the spec tests by: * Randomize `async`/`sync` instead of both * Check the took time rarely * Don't check if index exists every time * Reduce transfer when checking breaker * Cache capabilities checks The first one is most of the speed up but the rest save another couple of minutes. Time drops from 13m45s to 6m or so on my laptop.
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| protected static boolean oldClusterHasFeature(String featureId) { | ||
| assert oldClusterTestFeatureService != null; | ||
| return oldClusterTestFeatureService.clusterHasFeature(featureId); | ||
| } |
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.
Unused.
...ulti-clusters/src/javaRestTest/java/org/elasticsearch/xpack/esql/ccq/MultiClusterSpecIT.java
Show resolved
Hide resolved
| return true; | ||
| } | ||
|
|
||
| @Before |
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.
Saves some time at the cost of some paranoia.
| public static void assertRequestBreakerEmpty() throws Exception { | ||
| assertBusy(() -> { | ||
| HttpEntity entity = adminClient().performRequest(new Request("GET", "/_nodes/stats")).getEntity(); | ||
| HttpEntity entity = adminClient().performRequest(new Request("GET", "/_nodes/stats?metric=breaker")).getEntity(); |
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.
Saves a bunch of bytes across the wire. We only really want the breaker stats.
| } | ||
| } | ||
| return false; | ||
| } |
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.
Moved.
Also, we don't need this big comment. It was require when this test supported versions of elasticsearch without the capability api.
| } | ||
|
|
||
| Map<?, ?> prevTooks = supportsTook() ? tooks() : null; | ||
| boolean checkTook = supportsTook() && rarely(); |
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.
About 20% of the time we check took. It's actually pretty expensive to build. Not terribly so, but it shaved some load off of the test.
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.
It's not our numbers that are expensive - it's the fact that we have to fetch the entire x-pack usage api.
| capabilities.put(requiredCapabilities, cap); | ||
| } | ||
| return cap; | ||
| } |
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.
Moved this up to this class so others could share in the cache.
| */ | ||
| private static final Map<List<String>, Boolean> capabilities = new ConcurrentHashMap<>(); | ||
|
|
||
| public static boolean hasCapabilities(RestClient client, List<String> requiredCapabilities) throws IOException { |
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.
It looks like List<String> requiredCapabilities always have 1 item in it. Is it worth simplifying? May be I missed some usage?
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.
csv-spec files make a list actually.
...ugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java
Outdated
Show resolved
Hide resolved
…pack/esql/qa/rest/RestEsqlTestCase.java Co-authored-by: Ievgen Degtiarenko <[email protected]>
Speeds up the spec tests by: * Randomize `async`/`sync` instead of both * Check the took time rarely * Don't check if index exists every time * Reduce transfer when checking breaker * Cache capabilities checks The first one is most of the speed up but the rest save another couple of minutes. Time drops from 13m45s to 6m or so on my laptop.
Speeds up the spec tests by: * Randomize `async`/`sync` instead of both * Check the took time rarely * Don't check if index exists every time * Reduce transfer when checking breaker * Cache capabilities checks The first one is most of the speed up but the rest save another couple of minutes. Time drops from 13m45s to 6m or so on my laptop.
Speeds up the spec tests by:
async/syncinstead of bothThe first one is most of the speed up but the rest save another couple of minutes. Time drops from 13m45s to 6m or so on my laptop.