Suppress any Exception in wasb task handler#27495
Conversation
It's important that problems with the log handler don't cause the task itself to fail. We discovered that sometimes the blob client will throw an error from azure.core.AzureError (currently we catch from azure.common). Rather than just add azure.core.AzureError, it seems sensible to just catch anything.
boring-cyborg
bot
added
area:logging
area:providers
provider:microsoft-azure
|
|
||
| return WasbHook(remote_conn_id) | ||
| except AzureHttpError: | ||
| except Exception: |
There was a problem hiding this comment.
If we are trying to catch everything here, the log msg below should be updated too as it was tailored for AzureHttpError. For example, we can included the exception detail itself into the log msg.
There was a problem hiding this comment.
I don't see what's AzureHttpError specific about the error message? Maybe propose what you'd like to see instead.
There was a problem hiding this comment.
Hi @jedcunningham , for example, the msg says "Please make sure that apache-airflow[azure] is installed". This would hint users that the error is due to missing apache-airflow[azure] while it's not necessarily the root cause.
I would suggest to make the error msg just general, plus adding the representation of the Exception itself.
There was a problem hiding this comment.
OK so... maybe everybody can be happy with this
I have phrased it less conclusively. Making it more clear that these are just suggestions to look into. It was never the case that, were we to find ourselves in this except block, that provider wasn't installed (how is this code running in that case? maybe installed from sources???) or connection not there (that wouldn't even make it to this block before because it is AirflowNotFoundException). Now it actually could be missing connection (since we have the broader except). So the suggestion is actually better as a result of this change. Meanwhile, the provider not installed theory is ... at least... no less bad..... In any case they are just questions and not statements of fact, so I feel ok with this now.
Thanks for your help in reviewing, and let me know if any further concerns.
|
|
||
| return WasbHook(remote_conn_id) | ||
| except AzureHttpError: | ||
| except Exception: |
There was a problem hiding this comment.
I don't see what's AzureHttpError specific about the error message? Maybe propose what you'd like to see instead.
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
3 participants
It's important that problems with the log handler don't cause the task itself to fail. We discovered that sometimes the blob client will throw an error from azure.core.AzureError (currently we catch from azure.common). Rather than just add azure.core.AzureError, it seems sensible to just catch anything.