[AIRFLOW-3528] handle socket exception with SFTPOperator

[eladkal]

Jira

• My PR addresses the following Airflow Jira

Description

add try/exception to catch socket exceptions

[codecov-io]

Codecov Report

Merging #4325 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #4325   +/-   ##
======================================
  Coverage    78.1%   78.1%           
======================================
  Files         201     201           
  Lines       16471   16471           
======================================
  Hits        12864   12864           
  Misses       3607    3607

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1023e8c...c83a4e5. Read the comment docs.

[feluelle]

Does it need to be a Custom (Airflow) Exception?
Why can't the original exception just be raised?

[eladkal]

Currently it raises AirflowException as it's being handled at this line:
https://github.com/apache/incubator-airflow/blob/c83a4e5b952baa8d390e94d27fd12df888d3113e/airflow/contrib/operators/sftp_operator.py#L158

The propose here is just not to show the "Error while transferring (file_msg)" as this is confusing and file_msg is None.

[ashb]

This fix I don't think will do what we want, as I think now this will end up ultimately logging this as a message:

Error while transferring None, error: Error while connecting client, error: socket timed out

(I took a guess at what might be in the OSError).

Which means we probably need to add some tests, via mocking, that makes open_sftp fail with an exception. And then once we have tests we can be sure the error is reported right in this case.

[eladkal]

This fix I don't think will do what we want, as I think now this will end up ultimately logging this as a message:

Error while transferring None, error: Error while connecting client, error: socket timed out

(I took a guess at what might be in the OSError).

Which means we probably need to add some tests, via mocking, that makes open_sftp fail with an exception. And then once we have tests we can be sure the error is reported right in this case.

I'm not sure what you mean... Since Python 3.3 socket.error is deprecated. It raised OSError instead.
https://docs.python.org/3/library/socket.html#socket.error

This PR doesn't attempt to solve the problem regarding why the connection fails. It only fix the message in order to avoid confusion. This problem was reported on Slack. @XD-DENG said that he will try to investigate about why the connection fails.

[ashb]

I guessed at what error it might report. The main thing is you have try-inside-try, so this won't do what you expect.

Additionally the error that was reporting was 'NoneType' object has no attribute 'open_sftp' which means no exception is being thrown, instead self.ssh_hook.get_conn() as ssh_client is returning None.

So the except you added (capturing OSError) won't ever get triggered.

Tests :)

[XD-DENG]

Agree with @ashb on the exception type to catch. Based on the exception/log shared in the Slack thread, it's an AttributeError instead of OSError. This PR can not (completely) handle the issue shared in the Slack thread.

[eladkal]

OK. Got it.
In this case I'll leave this to @XD-DENG or whoever wants to pick this up. From what was reported regardless of the error handling the operator doesn't seem to work. This exceed from the scope of what I can help with at this point.

[ashb]

Okay, thanks for trying though!