Fix bug in circuit-breaker check for geoshape grid aggregations

[talevy]

There was a bug in the geoshape circuit-breaker check where the
hash values array was being allocated before its new size was
accounted for by the circuit breaker.

Fixes #57847.

[elasticmachine]

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

[nik9000]

I'm not really a big fan of guessing that the array will expand to using oversize. This is right now but there isn't anything that makes sure this stays right.

[talevy]

to reproduce OOM, download this USA Elasticsearch document:

usa.txt

curl -X PUT "user:pass@localhost:9200/test?pretty" -H 'Content-Type: application/json' -d'
{
    "mappings" : {
        "properties" : {
            "geometry" : { "type" : "geo_shape" }
        }
    }
}
'

curl -X PUT "user:pass@localhost:9200/test/_doc/usa?pretty" -H 'Content-Type: application/json' -d'@/path/to/usa.txt'

curl -X GET "user:pass@localhost:9200/test/_search" -H 'Content-Type: application/json' -d'
{
  "size": 0,
  "aggs": {
    "gridSplit": {
      "geotile_grid": {
        "field": "geometry",
        "precision": 12
      },
      "aggs": {
        "gridCentroid": {
          "geo_centroid": {
            "field": "geometry"
          }
        }
      }
    }
  }
}
'

[iverase]

I agree with Nick's point, I wonder if we should add a method to SortingNumericDocValues that gives the amount is going to add to a resize so we can be accurate and be tight to the implementation, wdyt?

[talevy]

is the concern that that there is nothing tying the usage of oversize as an estimate with the actual implementation of growing the array? And that this should be coupled better with the resizing so that the two depend on one another?

Looking at SortingNumericDocValues again now, I'm surprised there is no circuit-breaking usage there since it leverages the same resizing techniques and can, similarly, run into memory pressure. I suspect this has never been a problem because of the bucket size limit?

[nik9000]

Looking at SortingNumericDocValues again now, I'm surprised there is no circuit-breaking usage there since it leverages the same resizing techniques and can, similarly, run into memory pressure. I suspect this has never been a problem because of the bucket size limit?

I bet we just don't hit it because we don't frequently end up with very many values so the array never grows that large. I wonder if we should just pass a LongConsumer to the ctor for the breaker so we always factor it into the breaker

[iverase]

@nik9000 Note that the case for grid aggregations on geo shapes is a bit special in the sense that one value can generate potentially millions of buckets. That is not the case for other bucket aggregations where typically one doc value generates maybe a handful of buckets. (Probably the other aggregations that can generate lots of buckets are related with range field types #50109)

Adding a LongConsumer to the constructor will touch lots of other implementations so I would like to avoid it for this PR. Maybe we can find a middle ground here and add a TODO to design a more resilient implementation?

[nik9000]

That makes sense to me! Would having a mutable `LongConsumer` that we register be ok? It could default to noop and this agg could link it to the breaker.

…

On Fri, Jun 12, 2020, 05:02 Ignacio Vera ***@***.***> wrote: @nik9000 <https://github.com/nik9000> Note that the case for grid aggregations on geo shapes is a bit special in the sense that one value can generate potentially millions of tickets. That is not the case for other bucket aggregations where typically one doc value generates maybe a handful of buckets. (Probably the other aggregations that can generate lots of buckets are related with range field types #50109 <#50109>) Adding a LongConsumer to the constructor will touch lots of other implementations so I would like to avoid it for this PR. Maybe we can find a middle ground here and add a TODO to design a more resilient implementation? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#57962 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABUXIT3VUS4AAY66CWUUS3RWHVKDANCNFSM4N24FRIQ> .

[iverase]

Would having a mutable LongConsumer that we
register be ok? It could default to noop and this agg could link it to the
breaker.

+1

[iverase]

@tal @nik9000 I think we should move this PR forward to see if we can achieve 7.9?

[talevy]

👍 yes. I had planned to push this forward this week

[iverase]

lgtm

[talevy]

I re-ran this using gradle run -Drun.license_type=trial

and this branch resulted in a circuit-breaker response

{"error":{"root_cause":[{"type":"circuit_breaking_exception","reason":"[request] Data too large, data for [<agg [gridSplit]>] would be [430729928/410.7mb], which is larger than the limit of [322122547/307.1mb]","bytes_wanted":430729928,"bytes_limit":322122547,"durability":"TRANSIENT"}],"type":"search_phase_execution_exception","reason":"all shards failed","phase":"query","grouped":true,"failed_shards":[{"shard":0,"index":"test","node":"Alaz6WFOQJ-4qP8Rrb1Mhw","reason":{"type":"circuit_breaking_exception","reason":"[request] Data too large, data for [<agg [gridSplit]>] would be [430729928/410.7mb], which is larger than the limit of [322122547/307.1mb]","bytes_wanted":430729928,"bytes_limit":322122547,"durability":"TRANSIENT"}}]},"status":429}

while master OOMd

[talevy]

it may also not be clear, but the aggregator keeps track of these bytes added, and cleans them up upon closing of the aggregation here

elasticsearch/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java

Line 268 in dc91a30

this.breakerService.getBreaker(CircuitBreaker.REQUEST).addWithoutBreaking(-this.requestBytesUsed);

[talevy]

thank you @nik9000. your suggestions were real great. updated!

[nik9000]

LGTM