You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "vincbeck (via GitHub)" <gi...@apache.org> on 2023/05/11 15:01:52 UTC
[GitHub] [airflow] vincbeck opened a new pull request, #31215: Increase the number of max attemps to 10 for EmrAddStepsOperator
vincbeck opened a new pull request, #31215:
URL: https://github.com/apache/airflow/pull/31215
We received multiple complaints from users that when using the operator `EmrAddStepsOperator`, they get throttled a lot. Boto3 retries already on `ThrottleException` by default, we can increase the number of attempts in order to try to mitigate that.
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
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] vincbeck commented on a diff in pull request #31215: Increase the number of max attemps to 10 for EmrAddStepsOperator
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #31215:
URL: https://github.com/apache/airflow/pull/31215#discussion_r1191435925
##########
airflow/providers/amazon/aws/operators/emr.py:
##########
@@ -102,10 +104,22 @@ def __init__(
self.waiter_max_attempts = waiter_max_attempts
self.execution_role_arn = execution_role_arn
- def execute(self, context: Context) -> list[str]:
- emr_hook = EmrHook(aws_conn_id=self.aws_conn_id)
+ @cached_property
+ def hook(self) -> EmrHook:
+ """Create and return an EmrHook."""
+ # Increase the number of max attempts (default 5) because the AddJobFlowSteps API gets throttled,
+ # replenishment is quite slow.
+ # See https://docs.aws.amazon.com/general/latest/gr/emr.html#limits_emr
+ config = Config(
+ retries={
+ "max_attempts": 10,
Review Comment:
Actually, you can specify that through the connection: https://airflow.apache.org/docs/apache-airflow-providers-amazon/stable/connections/aws.html#avoid-throttling-exceptions. That makes me think, these changes might override any configuration users might have set. e.g. Let's say a user specified `max_attempts` to 20 in the connection configuration, this change is going to override this
--
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] pankajkoti commented on a diff in pull request #31215: Increase the number of max attemps to 10 for EmrAddStepsOperator
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #31215:
URL: https://github.com/apache/airflow/pull/31215#discussion_r1191333952
##########
airflow/providers/amazon/aws/operators/emr.py:
##########
@@ -102,10 +104,22 @@ def __init__(
self.waiter_max_attempts = waiter_max_attempts
self.execution_role_arn = execution_role_arn
- def execute(self, context: Context) -> list[str]:
- emr_hook = EmrHook(aws_conn_id=self.aws_conn_id)
+ @cached_property
+ def hook(self) -> EmrHook:
+ """Create and return an EmrHook."""
+ # Increase the number of max attempts (default 5) because the AddJobFlowSteps API gets throttled,
+ # replenishment is quite slow.
+ # See https://docs.aws.amazon.com/general/latest/gr/emr.html#limits_emr
+ config = Config(
+ retries={
+ "max_attempts": 10,
Review Comment:
Should we expose this and accept as a parameter so users could control it? If we observe that 10 is not enough too, the only way to control this would be via a code change.
Or do we see any downside in exposing this?
--
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] vincbeck commented on a diff in pull request #31215: Increase the number of max attemps to 10 for EmrAddStepsOperator
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #31215:
URL: https://github.com/apache/airflow/pull/31215#discussion_r1191431350
##########
airflow/providers/amazon/aws/operators/emr.py:
##########
@@ -102,10 +104,22 @@ def __init__(
self.waiter_max_attempts = waiter_max_attempts
self.execution_role_arn = execution_role_arn
- def execute(self, context: Context) -> list[str]:
- emr_hook = EmrHook(aws_conn_id=self.aws_conn_id)
+ @cached_property
+ def hook(self) -> EmrHook:
+ """Create and return an EmrHook."""
+ # Increase the number of max attempts (default 5) because the AddJobFlowSteps API gets throttled,
+ # replenishment is quite slow.
+ # See https://docs.aws.amazon.com/general/latest/gr/emr.html#limits_emr
+ config = Config(
+ retries={
+ "max_attempts": 10,
Review Comment:
That's a good point. Let me add that as parameter
--
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] vincbeck closed pull request #31215: Increase the number of max attemps to 10 for EmrAddStepsOperator
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck closed pull request #31215: Increase the number of max attemps to 10 for EmrAddStepsOperator
URL: https://github.com/apache/airflow/pull/31215
--
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] vincbeck commented on a diff in pull request #31215: Increase the number of max attemps to 10 for EmrAddStepsOperator
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #31215:
URL: https://github.com/apache/airflow/pull/31215#discussion_r1191436879
##########
airflow/providers/amazon/aws/operators/emr.py:
##########
@@ -102,10 +104,22 @@ def __init__(
self.waiter_max_attempts = waiter_max_attempts
self.execution_role_arn = execution_role_arn
- def execute(self, context: Context) -> list[str]:
- emr_hook = EmrHook(aws_conn_id=self.aws_conn_id)
+ @cached_property
+ def hook(self) -> EmrHook:
+ """Create and return an EmrHook."""
+ # Increase the number of max attempts (default 5) because the AddJobFlowSteps API gets throttled,
+ # replenishment is quite slow.
+ # See https://docs.aws.amazon.com/general/latest/gr/emr.html#limits_emr
+ config = Config(
+ retries={
+ "max_attempts": 10,
Review Comment:
Setting the PR as draft to not merge it accidentally. Meanwhile, I am going to think a bit more on that specific issue
--
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] pankajkoti commented on a diff in pull request #31215: Increase the number of max attemps to 10 for EmrAddStepsOperator
Posted by "pankajkoti (via GitHub)" <gi...@apache.org>.
pankajkoti commented on code in PR #31215:
URL: https://github.com/apache/airflow/pull/31215#discussion_r1191333952
##########
airflow/providers/amazon/aws/operators/emr.py:
##########
@@ -102,10 +104,22 @@ def __init__(
self.waiter_max_attempts = waiter_max_attempts
self.execution_role_arn = execution_role_arn
- def execute(self, context: Context) -> list[str]:
- emr_hook = EmrHook(aws_conn_id=self.aws_conn_id)
+ @cached_property
+ def hook(self) -> EmrHook:
+ """Create and return an EmrHook."""
+ # Increase the number of max attempts (default 5) because the AddJobFlowSteps API gets throttled,
+ # replenishment is quite slow.
+ # See https://docs.aws.amazon.com/general/latest/gr/emr.html#limits_emr
+ config = Config(
+ retries={
+ "max_attempts": 10,
Review Comment:
Should we expose this and accept as a parameter? If we observe that 10 is not enough too, the only way to control this would be via a code change.
Or do we see any downside in exposing this?
--
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] vincbeck commented on pull request #31215: Increase the number of max attemps to 10 for EmrAddStepsOperator
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #31215:
URL: https://github.com/apache/airflow/pull/31215#issuecomment-1544369851
I am closing this PR because or the reasons mentioned in [this comment](https://github.com/apache/airflow/pull/31215#discussion_r1191435925). Users can specify a custom `max_attempts` through the connection configuration. Though, I will link the documentation on how to do that in EMR docs
--
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