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

Rewrite-to-filters optimization is very slow in FiltersAggregator #74480

Closed
rafisism opened this issue Jun 23, 2021 · 7 comments · Fixed by #74260
Closed

Rewrite-to-filters optimization is very slow in FiltersAggregator #74480

rafisism opened this issue Jun 23, 2021 · 7 comments · Fixed by #74260
Labels
:Analytics/Aggregations Aggregations >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v7.14.0 v8.0.0-alpha1

Comments

@rafisism
Copy link

rafisism commented Jun 23, 2021

For my use-cases, the searches with heavily used Filters aggregations became about 20 times slower comparing version 7.10.2 and 7.13.2. The slowdown happened in version 7.11.0. And it is not just slower, it starts to return errors at high load very soon, where the older version works with 0% errors. Looked very similar to #73426

I took branch 7.13 and changed line 178 in server/src/main/java/org/elasticsearch/search/aggregations/bucket/filter/FiltersAggregator.java from
return parent == null && otherBucketKey == null;
to
return false;
And now it works fast again, even a bit faster than in 7.10.2

Could you do something like #73620 here too, please?

I know, ideally I need to provide a code and a test creating an index with mappings, indexing the data, and running a stress-test. The code change is very similar to #73620, but everything else is quite a lot of work. Please, let me know if you need anything.

Thank you!

@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jun 23, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@nik9000 nik9000 added the >bug label Jun 23, 2021
@nik9000
Copy link
Member

nik9000 commented Jun 23, 2021

For my use-cases, the searches with heavily used Filters aggregations became about 20 times slower comparing version 7.10.2 and 7.13.2. The slowdown happened in version 7.11.0. And it is not just slower, it starts to return errors at high load very soon, where the older version works with 0% errors. Looked very similar to #73426

Yikes! I think #74260 is going to have you covered though. It'll switch it back to the old behavior if the top level query isn't empty.

I know, ideally I need to provide a code and a test creating an index with mappings, indexing the data, and running a stress-test. The code change is very similar to #73620, but everything else is quite a lot of work. Please, let me know if you need anything.

This one is pretty well known at this point. And the place you disabled sort of proves it. So no need for more, I think.

I'm going to mark #74260 as closing this one. If you have the ability to test this locally with your data I'd love it if you could. You'd have to restore a snapshot or start with a copy of the data directory or something.

Also, if you are so inclined to build a curl or Kibana dev CONSOLE recreation of your issue I'd happily add it as a test to that PR.

@rafisism
Copy link
Author

rafisism commented Jun 23, 2021

@nik9000 , thank you for the fix! Hopefully everything is good and it will be released soon.
I would love to try your fix but I stuck with my plugin incompatibility for a not-yet-released version
I was able to change elasticsearch version from 7.13.3 to 7.13.2 to match them, but it is not so obvious how to do it for 7.14.0 and 8.0.0.

@rafisism
Copy link
Author

@nik9000 , is filter_by_filter_disable_7_x a right branch to try your fix for version 7.14.0?
filter_by_filter_cache_kindness_take_two is for 8.0.0 and that requires a lot of changes in my plugin.

@nik9000
Copy link
Member

nik9000 commented Jun 24, 2021

filter_by_filter_cache_kindness_take_two is for 8.0.0 and that requires a lot of changes in my plugin.

Ah, sorry, yeah, I don't have a 7.x branch for that now. I expect I'll have one for it in a few hours though. I can ping you on the PR when I open it if you'd like.

@rafisism
Copy link
Author

I tested my app with 7.x branch. It works, thanks!

@nik9000
Copy link
Member

nik9000 commented Jun 30, 2021

I tested my app with 7.x branch. It works, thanks!

Wonderful! Sorry for causing you trouble. Thanks for reporting it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v7.14.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants