dabla commented on code in PR #74600:
URL: https://github.com/apache/airflow/pull/74600#discussion_r4315612111
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/hooks/powerbi.py:
##########
@@ -187,6 +196,47 @@ async def get_refresh_details_by_refresh_id(
refresh_details = refresh_histories[refresh_ids.index(refresh_id)]
+ if refresh_details.get(PowerBIDatasetRefreshFields.REFRESH_TYPE.value)
== self.VIA_ENHANCED_API:
Review Comment:
**[warning]** After the rebase on the durable operator (#74675), the retry
decision and the trigger read the status of an enhanced refresh from different
sources.
The line that needs to change,
`PowerBIDatasetRefreshOperator.get_job_status` in `operators/powerbi.py`, is
not part of this diff, so I'm commenting here, where the execution-details
lookup starts.
With `durable=True`, a retry starts with `get_job_status`, and the
`ResumableJobMixin` uses the status it returns to choose between three things:
reconnect to an active refresh, skip a refresh that already succeeded, or
submit a new one when the old one failed. `get_job_status` still scans
`get_refresh_history` itself, so for an enhanced refresh it never sees the
execution details this PR adds. The trigger does see them, through
`get_refresh_details_by_refresh_id`.
Take the case this PR is built around: history stays `Unknown` while the
execution details say `Failed`. A probe at this head (operator and hook against
a mocked `run`) gives:
```
resume decision sees: 'In Progress' -> is_job_active=True
trigger sees: 'Failed'
```
So every durable retry reconnects to the failed refresh, the trigger reports
`Failed` straight away, and the task fails again without ever submitting a new
refresh. The mixin's "failed job → resubmit fresh" path is never reached, and
all retries are used up on the same failed refresh. Before this PR the trigger
also read only the history, so the same case waited until the timeout. The PR
improves the trigger side but leaves the retry decision on the old source.
Reading the status from the same lookup as the trigger fixes it, and keeps
the "not in history" case for refreshes Power BI has dropped:
```python
def get_job_status(self, external_id: JsonValue, context: Context) -> str:
refresh_histories = self.hook.run_sync(
self.hook.get_refresh_history(dataset_id=self.dataset_id,
group_id=self.group_id)
)
if not any(
details[PowerBIDatasetRefreshFields.REQUEST_ID.value] == external_id
for details in refresh_histories
):
return _REFRESH_NOT_IN_HISTORY
refresh_details = self.hook.run_sync(
self.hook.get_refresh_details_by_refresh_id(
dataset_id=self.dataset_id, group_id=self.group_id,
refresh_id=str(external_id)
)
)
return refresh_details[PowerBIDatasetRefreshFields.STATUS.value]
```
The history is then fetched twice, but only once per retry, not on every
poll. A failed execution-details request still raises, so the retry fails
instead of submitting a second refresh while the first one may still be
running. A durable test next to the existing `TaskStateStore` ones in
`operators/test_powerbi.py` (stored ID, enhanced history `Unknown`, execution
`Failed`, expect a new submit) would cover it.
This only matters if history really does stay `Unknown` after the refresh
has ended, which the fixture README says wasn't reproduced. But that is the
premise of the whole PR, so the retry decision should follow it too.
--
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]
