Re: [PR] Add retry on error 502 and 504 [airflow]

["jscheffl (via GitHub)" <gi...@apache.org>]

jscheffl commented on code in PR #42994:
URL: https://github.com/apache/airflow/pull/42994#discussion_r1800074335


##########
airflow/api_internal/internal_api_call.py:
##########
@@ -103,12 +110,21 @@ def internal_api_call(func: Callable[PS, RT]) -> Callable[PS, RT]:
     See [AIP-44](https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-44+Airflow+Internal+API)
     for more information .
     """
+
     from requests.exceptions import ConnectionError
 
+    def is_retryable_exception(exception: BaseException) -> bool:

Review Comment:
   This is a piurely internal method, can you add some explanaition "why" and mark it as private?
   ```suggestion
       def _is_retryable_exception(exception: BaseException) -> bool:
       """
       Evaluates which exception types should be re-tried.
       
       This is especially demanded for cases where (a) an application gateway or Kubernetes ingress can
       not find a running instance of a webserver hostring the API (HTTP 502+504) or when the
       HTTP request fails in general on network level.
       
       Note that we want to fail on other general errors on the webserver not to send bad requests in an endless loop.
       """
   ```



##########
airflow/api_internal/internal_api_call.py:
##########
@@ -39,6 +40,12 @@
 
 logger = logging.getLogger(__name__)
 
+class AirflowHttpException(AirflowException):
+    """Raise when there is a problem during an http request."""

Review Comment:
   Maybe some re-phrasing to make it clear (so that the docstring has a benefit over the pure class name)
   ```suggestion
       """Raise when there is a problem during an http request on the internal API decorator."""
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org