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 poll_query_status in EMRContainerHook #19877

Closed
1 task done
eladkal opened this issue Nov 29, 2021 · 10 comments · Fixed by #21423
Closed
1 task done

Refactor poll_query_status in EMRContainerHook #19877

eladkal opened this issue Nov 29, 2021 · 10 comments · Fixed by #21423
Assignees
Labels
area:providers good first issue kind:task A task that needs to be completed as part of a larger issue provider:amazon AWS/Amazon - related issues

Comments

@eladkal
Copy link
Contributor

eladkal commented Nov 29, 2021

Body

The goal is to refactor the code so that we can remove this TODO

# TODO: Make this logic a little bit more robust.
# Currently this polls until the state is *not* one of the INTERMEDIATE_STATES
# While that should work in most cases...it might not. :)

More information about the concerns can be found on #16766 (comment)

Committer

  • I acknowledge that I am a maintainer/committer of the Apache Airflow project.
@eladkal eladkal added provider:amazon AWS/Amazon - related issues area:providers good first issue kind:task A task that needs to be completed as part of a larger issue labels Nov 29, 2021
@patrikhv
Copy link

Hi. I would like to work on this issue. Can you assign it to me?

@uranusjr
Copy link
Member

Go ahead!

@dacort
Copy link
Contributor

dacort commented Nov 29, 2021

Thanks @patrikhv - let me know if I can help in any way! (original PR contributor and EMR team member)

@patrikhv
Copy link

patrikhv commented Dec 7, 2021

Hi, I am working on that, but I do not have any idea how to refactor it. I already just did a more explicit call, if the query is in a completed stage. Can you give me any tips?

@ferruzzi
Copy link
Contributor

ferruzzi commented Jan 6, 2022

Had a chat with dacort. They mentioned that the change they had in mind was that we don't actually confirm that the state is at a terminal state, only that it is not at an intermediate state. I can't think of any other changes I'd make to it.

@ferruzzi
Copy link
Contributor

ferruzzi commented Jan 6, 2022

@patrikhv - It looks to me like adding in the explicit "is it a terminal state?" check is all that is needed here. You want to put in the PR?

@patrikhv patrikhv removed their assignment Jan 7, 2022
@eladkal
Copy link
Contributor Author

eladkal commented Jan 31, 2022

@ferruzzi since patrikhv is not working on it. Do you want to put a PR to solve this?

@ferruzzi
Copy link
Contributor

@eladkal I have a few other things I'm working on. I can have a look when I get time, but it'll likely be a couple weeks unless somebody gets to it before then.

@victorphoenix3
Copy link
Contributor

Hello! I am new to the community and would like to take this on as my first issue. May I submit a PR for this? @eladkal

@potiuk
Copy link
Member

potiuk commented Feb 7, 2022

Feel fre to attempt it :). Maybe @ferruzzi will be able to help with reviewing it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers good first issue kind:task A task that needs to be completed as part of a larger issue provider:amazon AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants