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

Move median_absolute_deviation aggregation to aggregations module. #92676

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

martijnvg
Copy link
Member

The InternalAggregationTestCase, AbstractNumericMetricTestCase and AggregationTestScriptsPlugin classes were moved to test/framework module, so that both server and aggregations module can use these base classes.

This also drops the MedianAbsoluteDeviation ceremonial interface.

Relates to #90283
Relates to #82273

The `InternalAggregationTestCase`, `AbstractNumericMetricTestCase` and
`AggregationTestScriptsPlugin` classes were moved to test/framework module,
so that both server and aggregations module can use these base classes.

This also drops the MedianAbsoluteDeviation ceremonial interface.

Relates to elastic#90283
Relates to elastic#82273
@martijnvg
Copy link
Member Author

Turns out that moving this aggregation hits an issue in sql plugin. This plugin isn't modularised, when it loads the MedianAbsoluteDeviationAggregationBuilder in MedianAbsoluteDeviationAgg class it loads the builder class using an unnamed module. This causes problems during the search, it can't lookup MedianAbsoluteDeviationAggregatorSupplier, since the key contains a class that is loaded with a class loader using unnamed module. So in MedianAbsoluteDeviationAggregatorSupplier#innerBuild(...) method the call to getAggregator(...) fails.

Also if the search request would be serialised to the search api then this problem wouldn't exist. The same instance that is created in the sql plugin is used in the search action causing these class loading issues.

I think the right solution would be to modularise the sql plugin. But then its dependencies like ql and mapper-version would need to be modularised. The sql-proto dependency is in particular an issue, since it is a java8 library that can't be modularised if it needs to stay compatible with java8. In anyway this change is too large to be done in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants