moomindani commented on code in PR #70130:
URL: https://github.com/apache/airflow/pull/70130#discussion_r3654048500
##########
providers/databricks/src/airflow/providers/databricks/operators/databricks.py:
##########
@@ -1167,6 +1167,8 @@ class DatabricksRunNowOperator(ResumableJobMixin,
BaseOperator):
before polling begins so that a worker crash and retry reconnects to
the existing run
instead of triggering a duplicate run of the same job. Set to
``False`` to always trigger a
fresh run on retry. Requires Airflow 3.3+; on earlier versions it is
silently ignored.
+ :param forward_dag_params: Whether to forward Dag-level params as
``job_parameters``
Review Comment:
The `.. note::` a few lines below still states forwarding happens
unconditionally, so the two now contradict each other. Worth adding the opt-out
to the note, e.g. append: "Set ``forward_dag_params=False`` to disable this."
Also worth noting there that `job_parameters` cannot be combined with
`notebook_params` / `python_params` / `jar_params` / `spark_submit_params` /
`python_named_params` / `dbt_commands` — the API rejects such a run, which is
the main reason a user would reach for this flag.
##########
providers/databricks/src/airflow/providers/databricks/operators/databricks.py:
##########
@@ -1315,7 +1319,7 @@ def _build_run_now_payload(self) -> dict[str, Any]:
json["job_id"] = job_id
del json["job_name"]
- if not json.get("job_parameters") and self.params:
+ if self.forward_dag_params and not json.get("job_parameters") and
self.params:
Review Comment:
Consider also skipping injection when a conflicting slot is already present.
Because `forward_dag_params` defaults to `True`, every Dag that is currently
broken by #66613 stays broken until its author discovers this new flag — and
the failure mode gives no hint that a new operator argument is the fix. The
conflict is deterministic and fully knowable from the payload at this point, so
it can be repaired automatically:
```python
_RUN_NOW_PARAM_SLOTS_CONFLICTING_WITH_JOB_PARAMETERS = (
"notebook_params",
"python_params",
"jar_params",
"spark_submit_params",
"python_named_params",
"dbt_commands",
)
if (
self.forward_dag_params
and not json.get("job_parameters")
and self.params
and not any(k in json for k in
_RUN_NOW_PARAM_SLOTS_CONFLICTING_WITH_JOB_PARAMETERS)
):
json["job_parameters"] = dict(self.params)
```
That fixes the regression for existing users without requiring them to
change anything, and `forward_dag_params=False` then covers the remaining case
the issue describes — a job whose entry point rejects extra params even when
there is no slot conflict.
I'm happy to be argued out of this if maintainers prefer to keep the change
minimal and purely opt-in; the silent-failure-by-default aspect is what makes
me raise it.
##########
providers/databricks/tests/unit/databricks/operators/test_databricks.py:
##########
@@ -2925,6 +2925,28 @@ def
test_run_now_does_not_override_existing_job_parameters(self, db_mock_class):
actual = db_mock.run_now.call_args.args[0]
assert actual["job_parameters"] == {"explicit": "value"}
+
@mock.patch("airflow.providers.databricks.operators.databricks.DatabricksHook")
+ def
test_run_now_does_not_inject_airflow_params_when_forward_dag_params_is_false(self,
db_mock_class):
Review Comment:
The test correctly covers the flag's mechanics (I verified it fails without
the guard). What isn't covered is the scenario that actually motivates the
change: Dag-level `params` combined with one of the legacy param slots, which
is what the API rejects. Nothing in the existing suite exercises that
combination.
Worth adding a case asserting that with `notebook_params` set and
`forward_dag_params=False`, the payload contains `notebook_params` and no
`job_parameters` — that pins the regression scenario rather than just the flag.
If you take the auto-skip suggestion on `_build_run_now_payload`, the same case
parametrized over the six conflicting slots would cover it directly.
--
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]
