Remove deprecated wrapper from scripted_metric#57627
Conversation
This removes the deprecated `asMultiBucketAggregator` wrapper from `scripted_metric`. Unlike most other such removals, this isn't likely to save much memory. But it does make the internals of the aggregator slightly less twisted. Relates to elastic#56487
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
|
@elasticmachine run elasticsearch-ci/1 |
| * tracked by the circuit breakers properly. This is sad. So we pick a big | ||
| * number and estimate that each bucket costs that. It could be wildly | ||
| * inaccurate. We're sort of hoping that the real memory breaker saves | ||
| * us here. Or that folks just don't use the aggregation. |
| * inaccurate. We're sort of hoping that the real memory breaker saves | ||
| * us here. Or that folks just don't use the aggregation. | ||
| */ | ||
| private static final long BUCKET_COST_ESTIMATE = 1024 * 5; |
There was a problem hiding this comment.
what made you choose this number beyond just "make it large"?
There was a problem hiding this comment.
Drive-by comment: I wonder if it would make sense to expose this as a configurable option for advanced users? (perhaps in a followup PR)
Reasoning being that we can't figure it out easily, it's already an advanced agg, so a user might actually be able to give us a reasonable estimate. And if they don't set it, we fall back to Big Number.
Dunno, might be a terrible idea :)
There was a problem hiding this comment.
The number is the original default "weight" of an aggregator. So it is basically what we used to have. I didn't want to just set it to the default weight because it doesn't really have anything to do with it. Other than it is what we were using.
Making it configurable is certainly interesting! I don't folks are going to have a good idea of what to set it to without having done some java hacking though. We try not to leak stuff like that to our users. OTOH scripted_metric is pretty bonkers so if we are going to leak that sort of thing anywhere, here is it.
There was a problem hiding this comment.
Yeah I don't feel super great about the idea either. But then again, we're essentially allowing a user to write a custom map-reduce job that runs on ES, so exposing this kind of accounting isn't too crazy in that context.
There's probably a case for forcing the user to estimate it too, since I've definitely seen very bad scripts knock over clusters before (maps holding high cardinality IDs, etc). I think that's probably a bridge too far, alas :)
…7627) This removes the deprecated `asMultiBucketAggregator` wrapper from `scripted_metric`. Unlike most other such removals, this isn't likely to save much memory. But it does make the internals of the aggregator slightly less twisted. Relates to elastic#56487
scripts don't have permission to call `parallelStream` because it'll sometimes make a fork/join pool and script can't start threads. In this case I added it by accident in elastic#57627 and should have just used `stream`.
Fixes two bugs introduced by elastic#57627: 1. We were not properly letting go of memory from the request breaker when the aggregation finished. 2. We no longer supported totally arbitrary stuff produced by the init script because we *assumed* that it'd be ok to run the script once and clone its results. Sadly, cloning can't clone *anything* that the init script can make, like `String` arrays. This runs the init script once for every new bucket so we don't need to clone.
Fixes two bugs introduced by #57627: 1. We were not properly letting go of memory from the request breaker when the aggregation finished. 2. We no longer supported totally arbitrary stuff produced by the init script because we *assumed* that it'd be ok to run the script once and clone its results. Sadly, cloning can't clone *anything* that the init script can make, like `String` arrays. This runs the init script once for every new bucket so we don't need to clone.
Fixes two bugs introduced by elastic#57627: 1. We were not properly letting go of memory from the request breaker when the aggregation finished. 2. We no longer supported totally arbitrary stuff produced by the init script because we *assumed* that it'd be ok to run the script once and clone its results. Sadly, cloning can't clone *anything* that the init script can make, like `String` arrays. This runs the init script once for every new bucket so we don't need to clone.
Fixes two bugs introduced by #57627: 1. We were not properly letting go of memory from the request breaker when the aggregation finished. 2. We no longer supported totally arbitrary stuff produced by the init script because we *assumed* that it'd be ok to run the script once and clone its results. Sadly, cloning can't clone *anything* that the init script can make, like `String` arrays. This runs the init script once for every new bucket so we don't need to clone.
This removes the deprecated
asMultiBucketAggregatorwrapper fromscripted_metric. Unlike most other such removals, this isn't likely tosave much memory. But it does make the internals of the aggregator
slightly less twisted.
Relates to #56487