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.

Sign up for GitHub

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

Jump to bottom

Surface HTML errors #121

Merged
merged 6 commits into from
Feb 21, 2018
Merged

Surface HTML errors #121

merged 6 commits into from
Feb 21, 2018

Conversation

betodealmeida
Copy link
Contributor

Our Druid cluster is returning an HTML error message when OOMing:

<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=ISO-8859-1"/>
<title>Error 500 </title>
</head>
<body>
<h2>HTTP ERROR: 500</h2>
<p>Problem accessing /druid/v2/. Reason:
<pre>    javax.servlet.ServletException: java.lang.OutOfMemoryError: GC overhead limit exceeded</pre></p>
<hr /><a href="http://eclipse.org/jetty">Powered by Jetty:// 9.3.19.v20170502</a><hr/>
</body>
</html>

This message is not surfaced correctly to the client, since the client expects a JSON response from Druid. Here's how it's currently surfaced:

{"error": "HTTP Error 500: Server Error \n Druid Error: None \n Query is: {\n    \"queryType\": \"scan\",\n    \"dataSource\": \"driver_locations\",\n    \"dimensions\": [\n        \"location\"\n    ],\n    \"granularity\": \"all\",\n    \"intervals\": \"2011-02-13T00:00:00+00:00/2101-01-01T00:00:00+00:00\",\n    \"metrics\": [],\n    \"limit\": 50000\n}"}

I modified the code that checks for errors in the client so that if an HTML response is returned the error message is correctly extracted from the <PRE></PRE> tags. With this, the error is surfaced correctly (although somewhat verbose):

HTTP Error 500: <html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=ISO-8859-1"/>
<title>Error 500 </title>
</head>
<body>
<h2>HTTP ERROR: 500</h2>
<p>Problem accessing /druid/v2/. Reason:
<pre>    javax.servlet.ServletException: java.lang.OutOfMemoryError: GC overhead limit exceeded</pre></p>
<hr /><a href="http://eclipse.org/jetty">Powered by Jetty:// 9.3.19.v20170502</a><hr/>
</body>
</html>
 Druid Error: javax.servlet.ServletException: java.lang.OutOfMemoryError: GC overhead limit exceeded
 Query is: {
    "queryType": "topN",
    "dataSource": "testdatasource",
    "granularity": "all",
    "intervals": "2015-12-29/pt1h",
    "aggregations": [
        {
            "type": "doubleSum",
            "fieldName": "count",
            "name": "count"
        }
    ],
    "dimension": "user_name",
    "metric": "count",
    "filter": {
        "type": "selector",
        "dimension": "user_lang",
        "value": "en"
    },
    "threshold": 1,
    "context": {
        "timeout": 1000
    }
}

@mistercrunch
Copy link
Member

@gianm seems like that should be fixed upstream, but PyDruid should probably handle it as well for the sake of supporting previous versions.

@mistercrunch
Copy link
Member

@betodealmeida the build failed :(

@betodealmeida
Copy link
Contributor Author

Looks like JSONDecodeError was introduced in Python 3.5 only, I added a workaround.

mistercrunch
mistercrunch reviewed Feb 19, 2018
View reviewed changes
pydruid/client.py Outdated

from six.moves import urllib

from pydruid.query import QueryBuilder
from base64 import b64encode

try:
# available only in Python >= 3.5
from json.decoder import JSONDecodeError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like JSONDecodeError is derived from ValueError, so catching ValueError should just work and remove the need for this extra complexity.

mistercrunch
mistercrunch reviewed Feb 19, 2018
View reviewed changes
tests/test_client.py Outdated
@patch('pydruid.client.urllib.request.urlopen')
def test_druid_returns_html_error(self, mock_urlopen):
# given
message = """<html>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: there's this trick indent long strings in code but not in the variable itself:

    s = textwrap.dedent"""\
    this
        wont
    be indented
    """

@mistercrunch mistercrunch merged commit 125a2d7 into druid-io:master Feb 21, 2018
@gianm gianm mentioned this pull request Feb 23, 2018
Open
@EmadMokhtar EmadMokhtar mentioned this pull request Feb 12, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mistercrunch mistercrunch mistercrunch left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@betodealmeida @mistercrunch