moomindani commented on code in PR #68017: URL: https://github.com/apache/airflow/pull/68017#discussion_r3510011110
########## providers/databricks/docs/operators/run_now.rst: ########## @@ -80,3 +80,41 @@ DatabricksRunNowDeferrableOperator Deferrable version of the :class:`~airflow.providers.databricks.operators.DatabricksRunNowOperator` operator. It allows to utilize Airflow workers more effectively using `new functionality introduced in Airflow 2.2.0 <https://airflow.apache.org/docs/apache-airflow/2.2.0/concepts/deferring.html#triggering-deferral>`_ + +.. _howto/operator:DatabricksRunNowDeferrableOperator:retry-args: + +Retry args in deferrable mode +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +When ``deferrable=True``, the ``databricks_retry_args`` dictionary is serialized across the +trigger boundary and must contain only Airflow-serializable values (plain Python primitives +such as ``int``, ``float``, ``str``, ``bool``, ``None``, ``dict``, and ``list``). Review Comment: Two precision issues here: 1. The boundary is Airflow serde, not plain primitives. #64960 deliberately switched the validator from `json.dumps` back to `airflow.sdk.serde.serialize` precisely because serde accepts more than primitives — the merged tests cover a `datetime` value as supported. Documenting "plain Python primitives" re-narrows what #64960 resolved to keep broad; "Airflow-serde-serializable values" (with primitives as the common case) would be accurate. 2. Serializability is necessary but not sufficient: the validator checks values only, not whether the keys are valid `tenacity.Retrying` kwargs. For example `{"stop": 3}` passes validation, then fails at runtime with `TypeError: 'int' object is not callable` when tenacity invokes `stop(retry_state)`. Worth stating that passing validation does not mean the retry config is meaningful. --- Drafted-by: Claude Code (Fable 5); reviewed by @moomindani before posting ########## providers/databricks/docs/operators/run_now.rst: ########## @@ -80,3 +80,41 @@ DatabricksRunNowDeferrableOperator Deferrable version of the :class:`~airflow.providers.databricks.operators.DatabricksRunNowOperator` operator. It allows to utilize Airflow workers more effectively using `new functionality introduced in Airflow 2.2.0 <https://airflow.apache.org/docs/apache-airflow/2.2.0/concepts/deferring.html#triggering-deferral>`_ + +.. _howto/operator:DatabricksRunNowDeferrableOperator:retry-args: + +Retry args in deferrable mode +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +When ``deferrable=True``, the ``databricks_retry_args`` dictionary is serialized across the +trigger boundary and must contain only Airflow-serializable values (plain Python primitives +such as ``int``, ``float``, ``str``, ``bool``, ``None``, ``dict``, and ``list``). + +**Supported** (serialization-safe and runtime-valid): + +.. code-block:: python + + # Only plain-primitive Retrying kwarg: reraise + databricks_retry_args = {"reraise": True} Review Comment: This example is serialization-safe but not runtime-valid as documented. When `retry_args` is provided, `BaseDatabricksHook.__init__` replaces the defaults rather than merging (`hooks/databricks_base.py:151-161` — only `retry` and `after` are re-injected), so `stop` and `wait` are gone and tenacity falls back to `stop_never` + `wait_none()`. On a persistently retryable API error this retries forever with zero backoff. It also contradicts the paragraph below: once `databricks_retry_args` is set, `retry_limit`/`retry_delay` are ignored entirely (they are only consulted in the `else` branch), so the two cannot be combined. Given deferrable mode rejects the tenacity objects that would restore `stop`/`wait`, I would replace the "Supported" example with explicit guidance: in deferrable mode, leave `databricks_retry_args` unset and use `retry_limit`/`retry_delay`. --- Drafted-by: Claude Code (Fable 5); reviewed by @moomindani before posting ########## generated/provider_dependencies.json.sha256sum: ########## @@ -1 +1 @@ -2d6f34bb40832f84cb6c121237b1c5b0a05181dccface9fd171558f4df1747dc +2ccde55d75b93c7fc2c5723fc7f74bf8995244606190c98acf005ea1f39f04ca Review Comment: This file was intentionally deleted and gitignored on `main` by #68801 (it is auto-regenerated by breeze when needed), so this PR re-introduces a file that no longer exists upstream — and the hash edit itself is byte-for-byte identical to the already-merged #68011. It looks like the branch was cut in the window between #67080 (which mistakenly re-added the file) and #68011. Could you rebase onto current `main` and drop this file from the diff? This is the same stale-base issue flagged during the #64960 review round. --- Drafted-by: Claude Code (Fable 5); reviewed by @moomindani before posting ########## providers/databricks/docs/operators/submit_run.rst: ########## @@ -166,3 +166,41 @@ DatabricksSubmitRunDeferrableOperator Deferrable version of the :class:`~airflow.providers.databricks.operators.DatabricksSubmitRunOperator` operator. It allows to utilize Airflow workers more effectively using `new functionality introduced in Airflow 2.2.0 <https://airflow.apache.org/docs/apache-airflow/2.2.0/concepts/deferring.html#triggering-deferral>`_ + +.. _howto/operator:DatabricksSubmitRunDeferrableOperator:retry-args: + +Retry args in deferrable mode +----------------------------- + +When ``deferrable=True``, the ``databricks_retry_args`` dictionary is serialized across the Review Comment: Same comments as the equivalent section in `run_now.rst` apply to this copy: the `{"reraise": True}` example, the "plain primitives" framing, the "at task submission" timing, and the `:class:` path. --- Drafted-by: Claude Code (Fable 5); reviewed by @moomindani before posting ########## providers/databricks/docs/operators/run_now.rst: ########## @@ -80,3 +80,41 @@ DatabricksRunNowDeferrableOperator Deferrable version of the :class:`~airflow.providers.databricks.operators.DatabricksRunNowOperator` operator. It allows to utilize Airflow workers more effectively using `new functionality introduced in Airflow 2.2.0 <https://airflow.apache.org/docs/apache-airflow/2.2.0/concepts/deferring.html#triggering-deferral>`_ + +.. _howto/operator:DatabricksRunNowDeferrableOperator:retry-args: + +Retry args in deferrable mode +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +When ``deferrable=True``, the ``databricks_retry_args`` dictionary is serialized across the +trigger boundary and must contain only Airflow-serializable values (plain Python primitives +such as ``int``, ``float``, ``str``, ``bool``, ``None``, ``dict``, and ``list``). + +**Supported** (serialization-safe and runtime-valid): + +.. code-block:: python + + # Only plain-primitive Retrying kwarg: reraise + databricks_retry_args = {"reraise": True} + +For controlling attempt count and delay, prefer the dedicated operator +parameters ``retry_limit`` and ``retry_delay`` rather than +``databricks_retry_args``. Custom tenacity strategy objects (``stop``, +``wait``, ``retry``, ``before``, ``after``, etc.) require tenacity +callable objects, which are not serialization-safe in deferrable mode. + +**Not supported** in deferrable mode (will raise ``ValueError`` at task submission): Review Comment: "At task submission" overstates the timing. The validation added by #64960 lives only in the trigger constructors (`triggers/databricks.py:61,160`), and the operator defers after submitting: `submit_run`/`run_now` runs first, then `_handle_deferrable_databricks_operator_execution` builds the trigger (`operators/databricks.py:834-836`, `1183-1185`). So with invalid `retry_args` the Databricks run is already launched, and the task fails at defer time, leaving that run in flight. (And if the run is already terminal on the first poll, the trigger is never built and no error is raised at all.) Suggest: "will raise ValueError when the task defers — note the Databricks run has already been submitted at that point". --- Drafted-by: Claude Code (Fable 5); reviewed by @moomindani before posting ########## providers/databricks/docs/operators/run_now.rst: ########## @@ -80,3 +80,41 @@ DatabricksRunNowDeferrableOperator Deferrable version of the :class:`~airflow.providers.databricks.operators.DatabricksRunNowOperator` operator. It allows to utilize Airflow workers more effectively using `new functionality introduced in Airflow 2.2.0 <https://airflow.apache.org/docs/apache-airflow/2.2.0/concepts/deferring.html#triggering-deferral>`_ + +.. _howto/operator:DatabricksRunNowDeferrableOperator:retry-args: + +Retry args in deferrable mode +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +When ``deferrable=True``, the ``databricks_retry_args`` dictionary is serialized across the +trigger boundary and must contain only Airflow-serializable values (plain Python primitives +such as ``int``, ``float``, ``str``, ``bool``, ``None``, ``dict``, and ``list``). + +**Supported** (serialization-safe and runtime-valid): + +.. code-block:: python + + # Only plain-primitive Retrying kwarg: reraise + databricks_retry_args = {"reraise": True} + +For controlling attempt count and delay, prefer the dedicated operator +parameters ``retry_limit`` and ``retry_delay`` rather than +``databricks_retry_args``. Custom tenacity strategy objects (``stop``, +``wait``, ``retry``, ``before``, ``after``, etc.) require tenacity +callable objects, which are not serialization-safe in deferrable mode. + +**Not supported** in deferrable mode (will raise ``ValueError`` at task submission): + +.. code-block:: python + + from tenacity import stop_after_attempt, wait_incrementing + + # Tenacity strategy objects — NOT serializable + databricks_retry_args = {"stop": stop_after_attempt(3)} + databricks_retry_args = {"wait": wait_incrementing(start=30, increment=30)} + + # Arbitrary callables — NOT serializable + databricks_retry_args = {"retry": my_custom_retry_callable} + +If you need a custom callable retry strategy, use the non-deferrable +:class:`~airflow.providers.databricks.operators.DatabricksRunNowOperator` (``deferrable=False``). Review Comment: The cross-reference path is missing the module segment: the class lives at `airflow.providers.databricks.operators.databricks.DatabricksRunNowOperator`, and `operators/__init__.py` has no re-exports, so this xref will not resolve. Sibling pages (`sql_statements.rst`, `notebook.rst`, `task.rst`) use the full path. The same short form already exists in the pre-existing intro lines of these two pages — since this PR touches both files, it would be good to fix those occurrences too. --- Drafted-by: Claude Code (Fable 5); reviewed by @moomindani before posting ########## providers/databricks/docs/changelog.rst: ########## @@ -33,6 +33,7 @@ Features ~~~~~~~~ * ``Fail fast for non-serializable retry_args in deferrable operators and triggers (#64960)`` +* ``Document supported retry_args shapes for deferrable Databricks operators`` Review Comment: Please drop this entry. Per the NOTE TO CONTRIBUTORS at the top of this file, the changelog is maintained semi-automatically by the release manager, and contributor edits are only for breaking-change guidance. A hand-added line (without the `(#NNNNN)` suffix, and under "Features" for a doc-only change) will conflict with the generated entries at release time — the commit message alone is sufficient. --- Drafted-by: Claude Code (Fable 5); reviewed by @moomindani before posting -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
