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

Aggregations: Prevent negative intervals in date_histogram #9634

Closed
cbuescher opened this issue Feb 10, 2015 · 5 comments

Comments

@cbuescher
Copy link
Member

commented Feb 10, 2015

Using a negative numeric interval setting in date_histogram can lead to OOM erros on 1.4.2:

DELETE /_all

PUT /index/doc/1
{
  "date":  "2014/01/01"
}

PUT /index/doc/2
{
  "date":  "2014/03/01"
}

GET /index/doc/_search?search_type=count
{
    "query" : {
        "filtered" : {
            "query" : {
                "match_all" : {}
            }
        }
    },
    "aggs" : {
        "by_time" : {
            "date_histogram" : {
                "field" : "date",
                "interval" : "-60000",
                "min_doc_count" : 0,
                "format" : "yyyy-MM-dd--HH:mm:ss.SSSZ"
            }
        }
    }
}

-->
{
   "error": "ReduceSearchPhaseException[Failed to execute phase [query], [reduce] ]; nested: OutOfMemoryError[Java heap space]; ",
   "status": 503
}
@cbuescher

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2015

Without the "min_doc_count" there seems to be no problem. Still, negative intervals don't make much sense in date_histogram, so I'll make it raise IllegalArgument exception.

@cbuescher

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2015

For histogram there is already a check for negative intervals in place in HistogramParser, will just change it so also 0 is not allowed.

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2015

Without the "min_doc_count" there seems to be no problem. Still, negative intervals don't make much sense in date_histogram, so I'll make it raise IllegalArgument exception.

FYI we prefer using ElasticsearchIllegalArgumentException instead of IllegalArgument whenever possible. The reason is that these exceptions are converted to 400 - Bad request on the rest layer.

@cbuescher

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2015

Yes, that's what I actually meant. Is there a good way to test for exceptions in the ElasticsearchIntegrationTest? All the exceptions on shard level seem to get wrapped into a SearchPhaseExecutionException, the only way to check the undelying cause seems to me to check the message string. At least that's what I see a lot in other tests, e.g. SimpleChildQuerySearchTests:

...
catch (SearchPhaseExecutionException e) {
            assertThat(e.getMessage(), containsString("[has_child] 'max_children' is less than 'min_children'"));
        }
...
@jpountz

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2015

Yeah, unfortunately I don't think we can do better (or I'm not aware of it).

cbuescher added a commit that referenced this issue Feb 13, 2015
Aggregations: Prevent negative intervals in date_histogram
Negative settings for interval in date_histogram could lead to OOM errors in conjunction
with min_doc_count=0. This fix raises exceptions in the histogram builder and the
 TimeZoneRounding classes so that the query fails before this can happen.

Closes #9634
Closes #9690

@cbuescher cbuescher closed this in c597d8d Feb 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.