[GitHub] potiuk commented on a change in pull request #4792: [AIRFLOW-3659] Create Google Cloud Transfer Service Operators

[GitBox <gi...@apache.org>]

potiuk commented on a change in pull request #4792:  [AIRFLOW-3659] Create Google Cloud Transfer Service Operators 
URL: https://github.com/apache/airflow/pull/4792#discussion_r261526269
 
 

 ##########
 File path: airflow/contrib/hooks/gcp_api_base_hook.py
 ##########
 @@ -188,6 +190,21 @@ def inner_wrapper(self, *args, **kwargs):
 
     fallback_to_default_project_id = staticmethod(fallback_to_default_project_id)
 
+    @staticmethod
+    def catch_http_exception(func):
 
 Review comment:
   It's been several months ago when we introduced it and it's mostly for debugging clarity. From what we experienced - exceptions like HttpError were wrapped in AirflowException anyway in some executors (I believe at least Sequential Executor we used for testing). And you loose the full stack trace of the original exception as it is thrown higher in the call hierarchy - retaining only the error message, but not original stack trace.
   
   If you throw an AirflowException on the other hand - it will not be wrapped and you can actually see where it was thrown in your code including full stack trace.
   
   I have not checked it recently - that behaviour might have changed since, we might want to test and revisit it, but in general I think it's a good idea to only throw AirflowException from execute() methods and having decorator makes it rather straightforward.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services