Shushankranjan commented on PR #68539:
URL: https://github.com/apache/airflow/pull/68539#issuecomment-4703651078
---
## What Changed in `variable.py`
The change (from your previous conversation) was specifically in
`Variable.set()` and `Variable.delete()`. Here's the before/after:
### `Variable.set()` - Before
```python
@classmethod
def set(cls, key, value, description=None, serialize_json=False) -> None:
import logging
from airflow.sdk.execution_time.context import _set_variable
try:
_set_variable(key, value, description, serialize_json=serialize_json)
except AirflowRuntimeError:
logging.getLogger(__name__).warning("Failed to set variable %r", key)
# error was swallowed — task continued silently
```
### `Variable.set()` - After (current)
```python
@classmethod
def set(cls, key, value, description=None, serialize_json=False) -> None:
from airflow.sdk.execution_time.context import _set_variable
_set_variable(key, value, description, serialize_json=serialize_json)
```
The same pattern applied to `Variable.delete()`.
---
## Why the Change Was Made
### 1. Silent Failure is Dangerous
The old `try/except` **caught and suppressed** `AirflowRuntimeError` from
the Execution API. If the API server rejected a `set()` or `delete()` (e.g.,
due to permissions, network issues, or server errors), the task would:
- Log a warning
- **Continue executing as if the write succeeded** ✗
This is a correctness bug: downstream tasks could read stale variable values
or operate on data that was never actually written.
### 2. Inconsistency with `Variable.get()`
`Variable.get()` already let `AirflowRuntimeError` propagate (with one
deliberate exception: `VARIABLE_NOT_FOUND` + a provided `default`). The old
`set()`/`delete()` had *no* such intentional swallowing - it was just a side
effect of defensive coding that went too far.
### 3. The `try/except` Was an Overcorrection
There's no legitimate reason to swallow a write error. The correct behavior
is:
- Write fails → task fails → Airflow marks the task instance as failed → the
user investigates
This matches Airflow's fault model: tasks are expected to be retried or
alarmed on, not silently succeed.
---
## What the Tests Verify Now
The two new tests confirm the corrected behavior:
| Test | What it asserts |
|---|---|
| `test_var_set_raises_on_error` | `Variable.set()` re-raises
`AirflowRuntimeError` when the API rejects the write |
| `test_var_delete_raises_on_error` | `Variable.delete()` re-raises
`AirflowRuntimeError` when the API rejects the delete |
The logging import was also removed as a cleanup - it was only there to
service the now-gone `except` block.
---
**Summary:** The `try/except` + logging in `set()`/`delete()` was silently
eating API errors, letting tasks proceed as if writes succeeded when they
hadn't. Removing it makes write failures immediately visible as task failures,
consistent with how `get()` already behaves.
--
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]
