Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed retry of PowerBIDatasetRefreshOperator when dataset refresh wasn't directly available #45513

Merged
merged 12 commits into from
Jan 26, 2025

Conversation

Ohashiro
Copy link
Contributor

@Ohashiro Ohashiro commented Jan 9, 2025

Fixes: #44618

Changes

This PR adds 2 changes:

  • a retry mechanism on the API request to get the refresh status, to fix the bug when the API is not yet up-to-date with the triggered refreshId
  • a new separation between the refresh trigger and the status polling

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@Ohashiro Ohashiro force-pushed the powerbi_operator/retry-get-refresh-status branch 2 times, most recently from 3da84b7 to 6e3ed2c Compare January 9, 2025 17:08
@dabla
Copy link
Contributor

dabla commented Jan 10, 2025

@Ohashiro really nice work here well done!

@Ohashiro Ohashiro force-pushed the powerbi_operator/retry-get-refresh-status branch from 6e3ed2c to c06cac9 Compare January 10, 2025 15:15
@Ohashiro Ohashiro marked this pull request as ready for review January 13, 2025 08:48
@dabla
Copy link
Contributor

dabla commented Jan 14, 2025

@Ohashiro If you have time to apply changes from comments, then I think you could remove the draft en we merge this PR.

Copy link
Contributor

@dabla dabla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM good job @Ohashiro thx a lot!

@Ohashiro
Copy link
Contributor Author

Thanks for all your feedbacks and your review!
Before merging, I just wanted to test the behavior of the xcom_push for the key powerbi_dataset_refresh_status. I think that we push this message only when the refresh is successful but I think that we should also push when the refresh fails

@dabla
Copy link
Contributor

dabla commented Jan 14, 2025

Thanks for all your feedbacks and your review! Before merging, I just wanted to test the behavior of the xcom_push for the key powerbi_dataset_refresh_status. I think that we push this message only when the refresh is successful but I think that we should also push when the refresh fails

Then it's more safe to put it back to draft, and set it back to ready once you've done the testing.

@Ohashiro Ohashiro marked this pull request as draft January 14, 2025 10:38
@Ohashiro
Copy link
Contributor Author

Hi @dabla I made a quick change to push the powerbi_dataset_refresh_status even when the refresh fails. Should I let you review this commit before setting the PR as "Ready for review"?

@Ohashiro Ohashiro marked this pull request as ready for review January 15, 2025 10:12
@dabla
Copy link
Contributor

dabla commented Jan 15, 2025

@Ohashiro do not forget to remove the draft option

@Ohashiro
Copy link
Contributor Author

Sorry, where is the "draft option"? I thought it was "ready" as is

@dabla
Copy link
Contributor

dabla commented Jan 15, 2025

Sorry, where is the "draft option"? I thought it was "ready" as is

It should be somewhere at the bottom, I cannot see it unfortunately. It's indeed the "Ready for review" button at bottom, now your PR is still marked as "Draft".

@Ohashiro Ohashiro force-pushed the powerbi_operator/retry-get-refresh-status branch from fe57655 to bfe7fa2 Compare January 15, 2025 19:19
@Ohashiro
Copy link
Contributor Author

I don't know, there might be a bug somewhere, I clicked on the "Ready for review" button a few hours ago and the PR appears "Open" to me.

I rebased main to see if it refreshes something and solves the display issue

@dabla
Copy link
Contributor

dabla commented Jan 15, 2025

I don't know, there might be a bug somewhere, I clicked on the "Ready for review" button a few hours ago and the PR appears "Open" to me.

I rebased main to see if it refreshes something and solves the display issue

Now it's fine.

@potiuk or @eladkal I've just reviewed this PR, if approved by you guys could it be merged when you have time as this fixes an issue with the PowerBIDatasetRefreshOperator.

@dabla
Copy link
Contributor

dabla commented Jan 15, 2025

@Ohashiro Could you rename the title of the PR into something like:

"Fixed retry of PowerBIDatasetRefreshOperator when dataset refresh wasn't directly available"

@Ohashiro Ohashiro changed the title PowerBi operator: retry the request to get the refresh status Fixed retry of PowerBIDatasetRefreshOperator when dataset refresh wasn't directly available Jan 15, 2025
@Ohashiro
Copy link
Contributor Author

Hi @dabla ! Do you know if there's something missing on this PR?

@dabla
Copy link
Contributor

dabla commented Jan 24, 2025

Hi @dabla ! Do you know if there's something missing on this PR?

No I just think they are being busy working on Airflow 3. Could @potiuk or @eladkal look to merge this please, it’s a bugfix which I already reviewed.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I just think they are being busy working on Airflow 3

Yes. THAT

@potiuk potiuk merged commit 6b87d1c into apache:main Jan 26, 2025
61 checks passed
@Ohashiro
Copy link
Contributor Author

OK I understand, thank you very much!

@dabla
Copy link
Contributor

dabla commented Jan 26, 2025

OK I understand, thank you very much!

Thank you @Ohashiro for fixing this issue.

@utkarsharma2 utkarsharma2 added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Jan 27, 2025
got686-yandex pushed a commit to got686-yandex/airflow that referenced this pull request Jan 30, 2025
niklasr22 pushed a commit to niklasr22/airflow that referenced this pull request Feb 8, 2025
ambika-garg pushed a commit to ambika-garg/airflow that referenced this pull request Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:microsoft-azure Azure-related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PowerBIDatasetRefreshOperator task fails whereas dataset refresh succeeds
4 participants