You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/12/19 14:29:20 UTC
[GitHub] [airflow] ShelRoman opened a new pull request, #28464: Add waiter config params to emr.add_job_flow_steps
ShelRoman opened a new pull request, #28464:
URL: https://github.com/apache/airflow/pull/28464
A small tweak related to https://github.com/apache/airflow/pull/27286
```
WaiterConfig={
"Delay": 5,
"MaxAttempts": 100,
}
```
The hardcoded config isn't usable with jobs that are running longer than ~500 seconds.
Moved those values into default arguments of the `add_job_flow_steps()` function which won't change default behaviour but will add some flexibility for use cases with longer emr steps.
Also considered option to pass whole config dict instead of `waiter_delay` and `waiter_max_attempts`, but these changes are smaller.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ShelRoman commented on pull request #28464: Add waiter config params to emr.add_job_flow_steps
Posted by GitBox <gi...@apache.org>.
ShelRoman commented on PR #28464:
URL: https://github.com/apache/airflow/pull/28464#issuecomment-1372885098
@potiuk thanks for the clarification and the link. :)
I didn't expect an immediate reaction, which might be commercial deformation kicks in, since it's my first PR in open-source.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis merged pull request #28464: Add waiter config params to emr.add_job_flow_steps
Posted by GitBox <gi...@apache.org>.
Taragolis merged PR #28464:
URL: https://github.com/apache/airflow/pull/28464
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ShelRoman commented on a diff in pull request #28464: Add waiter config params to emr.add_job_flow_steps
Posted by GitBox <gi...@apache.org>.
ShelRoman commented on code in PR #28464:
URL: https://github.com/apache/airflow/pull/28464#discussion_r1052448782
##########
airflow/providers/amazon/aws/hooks/emr.py:
##########
@@ -126,14 +126,21 @@ def create_job_flow(self, job_flow_overrides: dict[str, Any]) -> dict[str, Any]:
return response
def add_job_flow_steps(
- self, job_flow_id: str, steps: list[dict] | str | None = None, wait_for_completion: bool = False
+ self,
+ job_flow_id: str,
+ steps: list[dict] | str | None = None,
+ wait_for_completion: bool = False,
+ waiter_delay: int = 5,
+ waiter_max_attempts: int = 100,
Review Comment:
yes, I considered options, but chose to keep it as closer to author's decision.
Maybe even better option is to pass config as a parameter
```
def add_job_flow_steps(
self,
job_flow_id: str,
steps: list[dict] | str | None = None,
wait_for_completion: bool = False,
waiter_config: dict | None = None,
)
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ShelRoman commented on a diff in pull request #28464: Add waiter config params to emr.add_job_flow_steps
Posted by GitBox <gi...@apache.org>.
ShelRoman commented on code in PR #28464:
URL: https://github.com/apache/airflow/pull/28464#discussion_r1052448782
##########
airflow/providers/amazon/aws/hooks/emr.py:
##########
@@ -126,14 +126,21 @@ def create_job_flow(self, job_flow_overrides: dict[str, Any]) -> dict[str, Any]:
return response
def add_job_flow_steps(
- self, job_flow_id: str, steps: list[dict] | str | None = None, wait_for_completion: bool = False
+ self,
+ job_flow_id: str,
+ steps: list[dict] | str | None = None,
+ wait_for_completion: bool = False,
+ waiter_delay: int = 5,
+ waiter_max_attempts: int = 100,
Review Comment:
yes, I considered options, but chose to keep it as closer to author's decision.
Maybe even better option is to pass config as a parameter
```
def add_job_flow_steps(
self, job_flow_id: str, steps: list[dict] | str | None = None, wait_for_completion: bool = False
self,
job_flow_id: str,
steps: list[dict] | str | None = None,
wait_for_completion: bool = False,
waiter_config: dict | None = None,
```
##########
airflow/providers/amazon/aws/hooks/emr.py:
##########
@@ -126,14 +126,21 @@ def create_job_flow(self, job_flow_overrides: dict[str, Any]) -> dict[str, Any]:
return response
def add_job_flow_steps(
- self, job_flow_id: str, steps: list[dict] | str | None = None, wait_for_completion: bool = False
+ self,
+ job_flow_id: str,
+ steps: list[dict] | str | None = None,
+ wait_for_completion: bool = False,
+ waiter_delay: int = 5,
+ waiter_max_attempts: int = 100,
Review Comment:
yes, I considered options, but chose to keep it as closer to author's decision.
Maybe even better option is to pass config as a parameter
```
def add_job_flow_steps(
self, job_flow_id: str, steps: list[dict] | str | None = None, wait_for_completion: bool = False
self,
job_flow_id: str,
steps: list[dict] | str | None = None,
wait_for_completion: bool = False,
waiter_config: dict | None = None,
)
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ShelRoman commented on pull request #28464: Add waiter config params to emr.add_job_flow_steps
Posted by GitBox <gi...@apache.org>.
ShelRoman commented on PR #28464:
URL: https://github.com/apache/airflow/pull/28464#issuecomment-1372530223
@eladkal could you please take a look also? I didn't intend to remove you from the reviewers but it happened
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #28464: Add waiter config params to emr.add_job_flow_steps
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28464:
URL: https://github.com/apache/airflow/pull/28464#discussion_r1052434511
##########
airflow/providers/amazon/aws/hooks/emr.py:
##########
@@ -126,14 +126,21 @@ def create_job_flow(self, job_flow_overrides: dict[str, Any]) -> dict[str, Any]:
return response
def add_job_flow_steps(
- self, job_flow_id: str, steps: list[dict] | str | None = None, wait_for_completion: bool = False
+ self,
+ job_flow_id: str,
+ steps: list[dict] | str | None = None,
+ wait_for_completion: bool = False,
+ waiter_delay: int = 5,
+ waiter_max_attempts: int = 100,
Review Comment:
Hmm.. what do you think if by default `waiter_delay` and `waiter_max_attempts` set to `None` and use in waiter config only if this values not None?
In this case default values from `botocore` waiters definition file will use: delay=30, max_attempts=60.
Something like that
```python
waiter.wait(
ClusterId=job_flow_id,
StepId=step_id,
WaiterConfig=trim_none_values(
{
"Delay": waiter_delay,
"MaxAttempts": waiter_max_attempts,
}
),
)
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28464: Add waiter config params to emr.add_job_flow_steps
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28464:
URL: https://github.com/apache/airflow/pull/28464#issuecomment-1372857385
> @Taragolis could you please merge this one? I don't have `write access` to do that.
Just to make sure to set your own expectations. Please do not ping people like that. It's quite selfish.
Be patient. There are 100s of PR being possibly looked at in parallel and merged in due time. You are looking at 1 PR while people who merge them at many @ShelRoman. be mindful for people time and accept the fact that people here work in their free time - and do not expect "immediate" reactions.
Watch my talk https://www.youtube.com/watch?v=G6VjYvKr2wQ&list=PLGudixcDaxY2LxjeHpZRtzq7miykjjFOn&index=55 about empathy to understand why.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ShelRoman commented on pull request #28464: Add waiter config params to emr.add_job_flow_steps
Posted by GitBox <gi...@apache.org>.
ShelRoman commented on PR #28464:
URL: https://github.com/apache/airflow/pull/28464#issuecomment-1372707240
@Taragolis could you please merge this one?
I don't have `write access` to do that.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #28464: Add waiter config params to emr.add_job_flow_steps
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #28464:
URL: https://github.com/apache/airflow/pull/28464#issuecomment-1372899766
Awesome work, congrats on your first merged pull request!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #28464: Add waiter config params to emr.add_job_flow_steps
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #28464:
URL: https://github.com/apache/airflow/pull/28464#issuecomment-1357756362
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Taragolis commented on a diff in pull request #28464: Add waiter config params to emr.add_job_flow_steps
Posted by GitBox <gi...@apache.org>.
Taragolis commented on code in PR #28464:
URL: https://github.com/apache/airflow/pull/28464#discussion_r1052506327
##########
airflow/providers/amazon/aws/hooks/emr.py:
##########
@@ -126,14 +126,21 @@ def create_job_flow(self, job_flow_overrides: dict[str, Any]) -> dict[str, Any]:
return response
def add_job_flow_steps(
- self, job_flow_id: str, steps: list[dict] | str | None = None, wait_for_completion: bool = False
+ self,
+ job_flow_id: str,
+ steps: list[dict] | str | None = None,
+ wait_for_completion: bool = False,
+ waiter_delay: int = 5,
+ waiter_max_attempts: int = 100,
Review Comment:
Yep, in this case we reduce number of parameters.
But we need to be sure that we send as a dictionary (otherwise it raise an error) and only `Delay` and `MaxAttempts` are acceptable parameters.
Unfortunetly waiters not well documented or better say `botocore` sources are only the documentation of waiters internals:
https://github.com/boto/botocore/blob/6af5c29d9c71deb325eca99f24698303fc9e3dea/botocore/waiter.py#L336-L338
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org