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

Refactor query polling logic for athena queries #21488

Merged
merged 1 commit into from
Mar 8, 2022

Conversation

victorphoenix3
Copy link
Contributor

Query status polling logic currently polls until the query is not in intermmediate state. It should instead poll until query is not in the terminal state.
Similar to #19877

Copy link
Contributor

@ferruzzi ferruzzi left a comment

Choose a reason for hiding this comment

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

You'll need to wait for a committer to approve and merge, but LGTM. 👍 Thanks for adding this one.

@potiuk
Copy link
Member

potiuk commented Feb 15, 2022

Some tests are failing :( (related)

@eladkal
Copy link
Contributor

eladkal commented Mar 7, 2022

@victorphoenix3 can you please fix the failing tests?
https://github.com/apache/airflow/runs/5156246799?check_suite_focus=true#step:7:4789

@victorphoenix3
Copy link
Contributor Author

@eladkal I have updated the PR. Please review

@@ -72,7 +72,7 @@ def test_init(self):

assert self.athena.hook.sleep_time == 0

@mock.patch.object(AthenaHook, 'check_query_status', side_effect=("SUCCESS",))
Copy link
Contributor Author

@victorphoenix3 victorphoenix3 Mar 7, 2022

Choose a reason for hiding this comment

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

'SUCCESS' is not a state for athena queries, "SUCCEEDED" is. The tests were failing because of this discrepancy

@eladkal eladkal mentioned this pull request Mar 7, 2022
@eladkal
Copy link
Contributor

eladkal commented Mar 7, 2022

@victorphoenix3 can you please rebase? tests are broken due to issue with moto (was already fixed in #22046 )

@victorphoenix3
Copy link
Contributor Author

@eladkal PR is ready

@potiuk potiuk merged commit 184a46f into apache:main Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:amazon AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants