[airflow][presto] Gracefully handle 503 errors and avoid eval()

[shahafabileah]

TO: @mistercrunch

What? Why?

I was running an airflow job that executes a number of presto queries. The first bunch succeeded, and then the job suddenly died with this error:

    records = presto.get_records(my_hql)
  File "/usr/local/lib/python2.7/dist-packages/airflow/hooks/presto_hook.py", line 50, in get_records
    obj = eval(str(e))
  File "<string>", line 1
    Unexpected status code 503
                    ^
SyntaxError: invalid syntax

What happened here is that presto was temporarily unavailable (a "hiccup") so it returned a 503 error. Our error-handling code expected the response to be eval()'able, which is true in most cases but not in the 503 case. With 503's, the message is just "Unexpected status code 503". You can't eval() that.

We discussed this offline. We both agree that eval() is just a bad thing to do. You suggested using json.loads instead. I tried that and found that str(e) is not valid json. Instead, it's python-hash formatted. For example, it has u prefixes for all the strings.

So I chose to take a different approach. I simply check for the values I expect to find (a la duck-typing). If they exist, I extract them and use them in the PrestoError. If not, I just str() the whole error.

How was it tested?

I executed an invalid query ("blah blah select...") and verified that we handle that error. Here's what I saw:

PrestoException: SYNTAX_ERROR: line 1:1: no viable alternative at input 'blah'

For the other case, rather than taking down presto, I simulated this by manually setting the error object to the string "Unexpected status code 503". Here's what I saw:

PrestoException: Unexpected status code 503

I'm brand new to airflow development so let me know if there are other/better ways to verify my change.

[shahafabileah]

Looks like I need to add a test to raise code coverage. I'll investigate...

[mistercrunch]

No need to raise coverage, we can't test Presto in travis (yet).

But we like lines under 80char to comply to PEP8.

[shahafabileah]

My bad -- I have my editor set to 100 chars. I'll revise...

[shahafabileah]

@mistercrunch I revised the code to be under 80 chars per line. PTAL.

[aoen]

LGTM