Re-enable some performance updates to ES|QL spec tests after resilience improvement to EsqlSpecTestCase#136781
Conversation
This reverts commit 207787a.
…lizing index creation (elastic#134090)" (elastic#134511) This reverts commit d915090.
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
| } | ||
| } | ||
|
|
||
| private static final Protected INGEST = new Protected(); |
There was a problem hiding this comment.
In the Views PR we use a separate VIEWS protector, semi-decoupling views from index creation. We could break the index parts into finer grained stuff too, but I see risks with that.
|
Sadly, we already see failures from the parallel data loading. I think the changes to EsqlSpecTestCase do give slightly better error messages, but not enough to determine the underlying cause. I'll do some local investigation to see if I can figure this out, otherwise revert that particular commit, and we deal with parallel data loading separately. |
| }); | ||
| try { | ||
| Response remote = remoteResponse.get(); | ||
| Response local = localResponse.get(); |
There was a problem hiding this comment.
NIT: you could use PlainActionFuture instead of anonymous listeners implementations here.
There was a problem hiding this comment.
Since this code comes from a revert of a revert of an approved PR, I do not want to fiddle with it.
astefan
left a comment
There was a problem hiding this comment.
I can't tell what exactly is the novelty in this PR compared to the two previous ones it reverts. You had this commit initially, but the code didn't make it in the final variant of the PR.
It would help me, at least, to compare what we did before and what this PR does new now, if you could point out in your changes or with a comment the relevant bits. Thanks.
Very good point. It turns out that the first commit you pointed out was actually also part of another PR, the one at #136780, which was merged two weeks ago, and this this PR got those updates when it merged main, effectively erasing the first commit, since it replaced it. So the real fix has been in for a few weeks now, and this PR now represents only the two optimizations being reverted. |
|
|
||
| private static <T> void executeInParallel(List<T> items, IOConsumer<T> consumer, String errorMessage) { | ||
| Semaphore semaphore = new Semaphore(PARALLEL_THREADS); | ||
| ESTestCase.runInParallel(items.size(), i -> { |
There was a problem hiding this comment.
ESTestCase.runInParallel internally creates a thread per each task.
Today we have 47 data sets but we only want to populate 10 at the time. I believe it would be much cheaper to use Executors.newFixedThreadPool(threads) rather than create a thread per task, especially if we want to limit parallelism.
…resilience improvement to EsqlSpecTestCase (elastic#136781)" This reverts commit 4d3e27f.
…resilience improvement to EsqlSpecTestCase (elastic#136781)" This reverts commit 4d3e27f.
…resilience improvement to EsqlSpecTestCase (#136781)" (#137589) This reverts commit 4d3e27f. Yesterday we merged two performance updates to ES|QL Spec tests that had been reverted two months back after noticing some increased test flakiness. However, we immediately saw increased failures due to one of them, so we reverted it. Having done that we continue to see other failures, that may or may not be related to the remaining performance update. This PR reverts that too.
BASE=9770442d77ceac0dda6f6e6a07d0d386885ab52a HEAD=955fbccc5328132bd5bcbb95bab60ca5b355e8ca Branch=main
BASE=9770442d77ceac0dda6f6e6a07d0d386885ab52a HEAD=955fbccc5328132bd5bcbb95bab60ca5b355e8ca Branch=main
… resilience improvement to EsqlSpecTestCase (elastic#136781)" (elastic#137589) This reverts commit 8e4ad76.
Initial support for ES|QL Views. It is based on the underlying sub-query support added in ..., and builds on the Views REST API work done in #137818. Checklist: - [x] Core prototype - [x] Extract key parts of Nik's original prototype - [x] Write testing infrastructure to test views within csv-spec tests (single and multi-node tests) - [x] Expand REST API to cover get, list, create/update, delete views - [x] Add size and count limitations to stored view definitions - [x] Write testing infrastructure to test views in CsvTest (csv-spec unit testing) - [x] Expand scope to nested views - [x] Expand scope to multiple views and views+indexes - [x] Split into multiple PRs for easy review - [x] Multi-relation index resolution (gives CCS/CPS support) - [x] #136780 - [x] More resilient EsqlSpecTestCase - [x] #136781 - [x] View CRUD with REST - [x] #137818 - [x] Rebase prototype on the above to get cleaner, simple feature PR - [ ] Enhancements - [x] Prototype CCS and CPS - [x] Enforce view/index name uniqueness (done in separate PR) - [x] Add serialization tests to test source deserialization in re-written plans - [x] Ensure views are gated behind SNAPSHOT and/or feature flag
Some recent test performance updates flushed out test fragility due to non-thread safety and other concurrency issues with test setup, and so two optimizations were reverted. A recent PR at #136780 added improved resiliency to concurrency issues in test setup which should allow us to bring back the performance optimizations.
The resiliency fix included in #136780 was originally extracted from the Views PR at #134995, which, due to early issues in view creation/deleting during test setup was reliably exposing the same errors seen in #134736, and so it seems likely it will also fix the issues seen in the optimizations.
Checklist:
Fixes #134890