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 2020/02/26 05:32:50 UTC
[GitHub] [airflow] baolsen opened a new pull request #7541: [AIRFLOW-6822]
AWS hooks should cache boto3 client
baolsen opened a new pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541
---
Issue link: WILL BE INSERTED BY [boring-cyborg](https://github.com/kaxil/boring-cyborg)
Make sure to mark the boxes below before creating PR: [x]
- [x] Description above provides context of the change
- [x] Commit message/PR title starts with `[AIRFLOW-NNNN]`. AIRFLOW-NNNN = JIRA ID<sup>*</sup>
- [x] Unit tests coverage for changes (not needed for documentation changes)
- [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [x] Relevant documentation is updated including usage instructions.
- [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
<sup>*</sup> For document-only changes commit message can start with `[AIRFLOW-XXXX]`.
---
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385542306
##########
File path: tests/providers/amazon/aws/operators/test_ecs.py
##########
@@ -171,7 +166,7 @@ def test_execute_with_failures(self):
}
)
- def test_wait_end_tasks(self):
+ def test_wait_end_tasks(self, aws_hook_mock):
Review comment:
Found a way to fix the issue with `setUp`, so this arg will be removed :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385542156
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -47,11 +48,29 @@ class AwsBaseHook(BaseHook):
:param verify: Whether or not to verify SSL certificates.
https://boto3.amazonaws.com/v1/documentation/api/latest/reference/core/session.html
:type verify: str or bool
+ :param str region_name: AWS Region name to use. If this is None then the default boto3
+ behaviour is used.
+ :param str client_type: boto3 client_type used when creating boto3.client(). For
+ example, 's3', 'emr', etc. Provided by specific hooks for these clients which
+ subclass AwsBaseHook.
+ :param str resource_type: boto3 resource_type used when creating boto3.resource(). For
+ example, 's3'. Provided by specific hooks for these resources which
+ subclass AwsBaseHook.
Review comment:
Resolved, thanks.
I preferred the conciseness of `:param <type> <var>: xyz`
But have changed back to `:param` + `:type`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385142816
##########
File path: tests/providers/amazon/aws/operators/test_ecs.py
##########
@@ -48,12 +48,10 @@
]
}
-
+# pylint: disable=unused-argument
+@mock.patch('airflow.providers.amazon.aws.operators.ecs.AwsBaseHook')
Review comment:
That's weird. It should work.
1. `@mock.patch('airflow.providers.amazon.aws.operators.ecs.AwsBaseHook')`
2. `self.aws_hook_mock = aws_hook_mock`
3. Access the mocked hook `self.aws_hook_mock` in the tests
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] kaxil commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385004614
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -49,9 +50,19 @@ class AwsBaseHook(BaseHook):
:type verify: str or bool
"""
- def __init__(self, aws_conn_id="aws_default", verify=None):
+ def __init__(
+ self,
+ aws_conn_id="aws_default",
+ verify=None,
+ region_name=None,
+ client_type=None,
+ resource_type=None
Review comment:
Good catch @feluelle π
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#issuecomment-591909170
# [Codecov](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=h1) Report
> Merging [#7541](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/aaa3d3286ee8298204a5fe1408ddddbf58759422&el=desc) will **decrease** coverage by `0.56%`.
> The diff coverage is `97.16%`.
[](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #7541 +/- ##
==========================================
- Coverage 88.28% 87.71% -0.57%
==========================================
Files 935 935
Lines 45196 45170 -26
==========================================
- Hits 39903 39623 -280
- Misses 5293 5547 +254
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [airflow/contrib/operators/awsbatch\_operator.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy9hd3NiYXRjaF9vcGVyYXRvci5weQ==) | `100.00% <ΓΈ> (ΓΈ)` | |
| [airflow/providers/amazon/aws/hooks/datasync.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9kYXRhc3luYy5weQ==) | `16.66% <50.00%> (-0.12%)` | :arrow_down: |
| [airflow/providers/amazon/aws/hooks/base\_aws.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9iYXNlX2F3cy5weQ==) | `61.70% <84.21%> (+2.05%)` | :arrow_up: |
| [airflow/providers/amazon/aws/hooks/athena.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9hdGhlbmEucHk=) | `64.40% <100.00%> (-2.79%)` | :arrow_down: |
| [airflow/providers/amazon/aws/hooks/aws\_dynamodb.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9hd3NfZHluYW1vZGIucHk=) | `88.23% <100.00%> (-3.07%)` | :arrow_down: |
| [airflow/providers/amazon/aws/hooks/batch\_client.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9iYXRjaF9jbGllbnQucHk=) | `96.61% <100.00%> (-0.32%)` | :arrow_down: |
| [...irflow/providers/amazon/aws/hooks/batch\_waiters.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9iYXRjaF93YWl0ZXJzLnB5) | `100.00% <100.00%> (ΓΈ)` | |
| [...flow/providers/amazon/aws/hooks/cloud\_formation.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9jbG91ZF9mb3JtYXRpb24ucHk=) | `95.83% <100.00%> (-0.95%)` | :arrow_down: |
| [airflow/providers/amazon/aws/hooks/emr.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9lbXIucHk=) | `92.00% <100.00%> (-1.75%)` | :arrow_down: |
| [airflow/providers/amazon/aws/hooks/glue\_catalog.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9nbHVlX2NhdGFsb2cucHk=) | `100.00% <100.00%> (ΓΈ)` | |
| ... and [42 more](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Ξ = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=footer). Last update [aaa3d32...c43b8cd](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385080404
##########
File path: airflow/providers/amazon/aws/sensors/athena.py
##########
@@ -57,13 +57,11 @@ def __init__(self,
super().__init__(*args, **kwargs)
self.aws_conn_id = aws_conn_id
self.query_execution_id = query_execution_id
- self.hook = None
self.sleep_time = sleep_time
self.max_retires = max_retires
+ self.hook = AWSAthenaHook(self.aws_conn_id, self.sleep_time)
Review comment:
Please don't initiate a Hook in an Operators `__init__`, because 1. the `__init__` will be called more often / every time the scheduler reads the dag file. 2. The Hook will also be initiated even if the Operator doesn't execute.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385087185
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -47,11 +48,29 @@ class AwsBaseHook(BaseHook):
:param verify: Whether or not to verify SSL certificates.
https://boto3.amazonaws.com/v1/documentation/api/latest/reference/core/session.html
:type verify: str or bool
+ :param str region_name: AWS Region name to use. If this is None then the default boto3
+ behaviour is used.
+ :param str client_type: boto3 client_type used when creating boto3.client(). For
+ example, 's3', 'emr', etc. Provided by specific hooks for these clients which
+ subclass AwsBaseHook.
+ :param str resource_type: boto3 resource_type used when creating boto3.resource(). For
+ example, 's3'. Provided by specific hooks for these resources which
+ subclass AwsBaseHook.
Review comment:
Can you use `:param` + `:type` ?
Also the type is `Optional[str]` not `str` because it allows a string to be None.
You can also define the types directly next to the argument like this: `region_name: Optional[str] = None`. You only need to import `typing`. It is an official PEP https://www.python.org/dev/peps/pep-0484/ in Python >=3.5
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385061503
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -49,9 +50,19 @@ class AwsBaseHook(BaseHook):
:type verify: str or bool
"""
- def __init__(self, aws_conn_id="aws_default", verify=None):
+ def __init__(
+ self,
+ aws_conn_id="aws_default",
+ verify=None,
+ region_name=None,
+ client_type=None,
+ resource_type=None
Review comment:
@feluelle Thanks for the feedback, please take another look. I've updated the docstrings on all the hooks accordingly. There's only a "how to" document for the DataSync operator, no other separate docs for AWS - at least that I could see. Let me know if there's anywhere else you can think of :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on issue #7541: [AIRFLOW-6822] AWS
hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on issue #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#issuecomment-599490382
Thanks @ashb , I've tried chipping away at this one when I've had time. Good luck in the meantime :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385606451
##########
File path: airflow/providers/amazon/aws/hooks/datasync.py
##########
@@ -29,10 +29,13 @@ class AWSDataSyncHook(AwsBaseHook):
"""
Interact with AWS DataSync.
+ Additional arguments (such as ``aws_conn_id``) may be specified and
+ are passed down to the underlying AwsBaseHook.
Review comment:
Same here.
You need to indent in cases where you are using rst "functions" like `.. seealso` or lists, but not plain text.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#issuecomment-591909170
# [Codecov](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=h1) Report
> Merging [#7541](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/aaa3d3286ee8298204a5fe1408ddddbf58759422&el=desc) will **decrease** coverage by `0.56%`.
> The diff coverage is `97.16%`.
[](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #7541 +/- ##
==========================================
- Coverage 88.28% 87.71% -0.57%
==========================================
Files 935 935
Lines 45196 45170 -26
==========================================
- Hits 39903 39623 -280
- Misses 5293 5547 +254
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [airflow/contrib/operators/awsbatch\_operator.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy9hd3NiYXRjaF9vcGVyYXRvci5weQ==) | `100.00% <ΓΈ> (ΓΈ)` | |
| [airflow/providers/amazon/aws/hooks/datasync.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9kYXRhc3luYy5weQ==) | `16.66% <50.00%> (-0.12%)` | :arrow_down: |
| [airflow/providers/amazon/aws/hooks/base\_aws.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9iYXNlX2F3cy5weQ==) | `61.70% <84.21%> (+2.05%)` | :arrow_up: |
| [airflow/providers/amazon/aws/hooks/athena.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9hdGhlbmEucHk=) | `64.40% <100.00%> (-2.79%)` | :arrow_down: |
| [airflow/providers/amazon/aws/hooks/aws\_dynamodb.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9hd3NfZHluYW1vZGIucHk=) | `88.23% <100.00%> (-3.07%)` | :arrow_down: |
| [airflow/providers/amazon/aws/hooks/batch\_client.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9iYXRjaF9jbGllbnQucHk=) | `96.61% <100.00%> (-0.32%)` | :arrow_down: |
| [...irflow/providers/amazon/aws/hooks/batch\_waiters.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9iYXRjaF93YWl0ZXJzLnB5) | `100.00% <100.00%> (ΓΈ)` | |
| [...flow/providers/amazon/aws/hooks/cloud\_formation.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9jbG91ZF9mb3JtYXRpb24ucHk=) | `95.83% <100.00%> (-0.95%)` | :arrow_down: |
| [airflow/providers/amazon/aws/hooks/emr.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9lbXIucHk=) | `92.00% <100.00%> (-1.75%)` | :arrow_down: |
| [airflow/providers/amazon/aws/hooks/glue\_catalog.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9nbHVlX2NhdGFsb2cucHk=) | `100.00% <100.00%> (ΓΈ)` | |
| ... and [42 more](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Ξ = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=footer). Last update [aaa3d32...c43b8cd](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385543236
##########
File path: airflow/providers/amazon/aws/sensors/sqs.py
##########
@@ -56,6 +56,7 @@ def __init__(self,
self.aws_conn_id = aws_conn_id
self.max_messages = max_messages
self.wait_time_seconds = wait_time_seconds
+ self.hook = SQSHook(aws_conn_id=self.aws_conn_id)
Review comment:
Resolved, thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385543208
##########
File path: airflow/providers/amazon/aws/sensors/redshift.py
##########
@@ -43,9 +43,9 @@ def __init__(self,
self.cluster_identifier = cluster_identifier
self.target_status = target_status
self.aws_conn_id = aws_conn_id
+ self.hook = RedshiftHook(aws_conn_id=self.aws_conn_id)
Review comment:
Resolved, thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r384466443
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -232,6 +243,23 @@ def get_resource_type(self, resource_type, region_name=None, config=None):
resource_type, endpoint_url=endpoint_url, config=config, verify=self.verify
)
+ @cached_property
+ def conn(self):
+ "Get the underlying boto3 client (cached)"
Review comment:
Would be great if you could also add a type hint for the return value. Either in the description or the function header.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] kaxil edited a comment on issue #7541: [AIRFLOW-6822] AWS
hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on issue #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#issuecomment-596544616
Some static checks have failed:
```
airflow/providers/amazon/aws/hooks/batch_client.py:40:1: F401 'airflow.utils.log.logging_mixin.LoggingMixin' imported but unused
************* Module airflow.providers.amazon.aws.hooks.batch_client
airflow/providers/amazon/aws/hooks/batch_client.py:213:4: C0116: Missing function or method docstring (missing-function-docstring)
airflow/providers/amazon/aws/hooks/batch_client.py:40:0: W0611: Unused LoggingMixin imported from airflow.utils.log.logging_mixin (unused-import)
************* Module airflow.contrib.operators.awsbatch_operator
airflow/contrib/operators/awsbatch_operator.py:43:0: R0901: Too many ancestors (8/7) (too-many-ancestors)
************* Module tests.providers.amazon.aws.hooks.test_batch_client
tests/providers/amazon/aws/hooks/test_batch_client.py:69:25: E1101: Instance of 'AwsBatchClient' has no 'hook' member (no-member)
************* Module tests.providers.amazon.aws.operators.test_batch
tests/providers/amazon/aws/operators/test_batch.py:93:25: E1101: Instance of 'AwsBatchOperator' has no 'hook' member (no-member)
************* Module tests.providers.amazon.aws.hooks.test_batch_waiters
tests/providers/amazon/aws/hooks/test_batch_waiters.py:348:25: E1101: Instance of 'AwsBatchWaiters' has no 'hook' member (no-member)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r387620707
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -232,6 +261,38 @@ def get_resource_type(self, resource_type, region_name=None, config=None):
resource_type, endpoint_url=endpoint_url, config=config, verify=self.verify
)
+ @cached_property
+ def conn(self):
+ """Get the underlying boto3 client (cached).
+
+ The return value from this method is cached for efficiency.
+
+ :return: boto3.client or boto3.resource for the current
+ client/resource type and region
+ :rtype: boto3.client() or boto3.resource()
+ :raises AirflowException: self.client_type or self.resource_type are not
+ populated. These are usually specified to this class, by a subclass
+ __init__ method.
+ """
+ if self.client_type:
+ return self.get_client_type(self.client_type, region_name=self.region_name)
+ elif self.resource_type:
+ return self.get_resource_type(self.resource_type, region_name=self.region_name)
+ else:
+ raise AirflowException(
+ 'Either self.client_type or self.resource_type'
+ ' must be specified in the subclass')
Review comment:
Yes, in the `__init__` I do the actual validation. The only way the `client_type`/`resource_type `could change after that is if a subclass changes it directly. It would represent a bug in the subclass implementation. Based on the Python docs on Exceptions, a `NotImplementedError `sounds like the best fit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385139258
##########
File path: tests/providers/amazon/aws/operators/test_ecs.py
##########
@@ -171,7 +166,7 @@ def test_execute_with_failures(self):
}
)
- def test_wait_end_tasks(self):
+ def test_wait_end_tasks(self, aws_hook_mock):
Review comment:
Okay, but can you please then add the pylint disable rule only to these lines. So that other unused arguments won't be disabled, too.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on issue #7541: [AIRFLOW-6822] AWS
hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on issue #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#issuecomment-592492686
Hi @feluelle
Thanks for all the feedback.
I had some nasty issues with Git and am busy getting things back to the right state.
Hopefully it doesn't mess up the code review history, but I will go through the previous discussion to ensure no changes are left out. Should get there today.
Then I'll take a look at your latest comments - probably only next week :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385002855
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -49,9 +50,19 @@ class AwsBaseHook(BaseHook):
:type verify: str or bool
"""
- def __init__(self, aws_conn_id="aws_default", verify=None):
+ def __init__(
+ self,
+ aws_conn_id="aws_default",
+ verify=None,
+ region_name=None,
+ client_type=None,
+ resource_type=None
Review comment:
Thanks, I forgot to do that step :p Am busy with it now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#issuecomment-591909170
# [Codecov](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=h1) Report
> Merging [#7541](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/aaa3d3286ee8298204a5fe1408ddddbf58759422&el=desc) will **decrease** coverage by `0.66%`.
> The diff coverage is `97.16%`.
[](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #7541 +/- ##
==========================================
- Coverage 88.28% 87.62% -0.67%
==========================================
Files 935 935
Lines 45196 45170 -26
==========================================
- Hits 39903 39578 -325
- Misses 5293 5592 +299
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [airflow/contrib/operators/awsbatch\_operator.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL29wZXJhdG9ycy9hd3NiYXRjaF9vcGVyYXRvci5weQ==) | `100.00% <ΓΈ> (ΓΈ)` | |
| [airflow/providers/amazon/aws/hooks/datasync.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9kYXRhc3luYy5weQ==) | `16.66% <50.00%> (-0.12%)` | :arrow_down: |
| [airflow/providers/amazon/aws/hooks/base\_aws.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9iYXNlX2F3cy5weQ==) | `61.70% <84.21%> (+2.05%)` | :arrow_up: |
| [airflow/providers/amazon/aws/hooks/athena.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9hdGhlbmEucHk=) | `64.40% <100.00%> (-2.79%)` | :arrow_down: |
| [airflow/providers/amazon/aws/hooks/aws\_dynamodb.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9hd3NfZHluYW1vZGIucHk=) | `88.23% <100.00%> (-3.07%)` | :arrow_down: |
| [airflow/providers/amazon/aws/hooks/batch\_client.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9iYXRjaF9jbGllbnQucHk=) | `96.61% <100.00%> (-0.32%)` | :arrow_down: |
| [...irflow/providers/amazon/aws/hooks/batch\_waiters.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9iYXRjaF93YWl0ZXJzLnB5) | `100.00% <100.00%> (ΓΈ)` | |
| [...flow/providers/amazon/aws/hooks/cloud\_formation.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9jbG91ZF9mb3JtYXRpb24ucHk=) | `95.83% <100.00%> (-0.95%)` | :arrow_down: |
| [airflow/providers/amazon/aws/hooks/emr.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9lbXIucHk=) | `92.00% <100.00%> (-1.75%)` | :arrow_down: |
| [airflow/providers/amazon/aws/hooks/glue\_catalog.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9nbHVlX2NhdGFsb2cucHk=) | `100.00% <100.00%> (ΓΈ)` | |
| ... and [46 more](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Ξ = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=footer). Last update [aaa3d32...c43b8cd](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io edited a comment on issue #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#issuecomment-591909170
# [Codecov](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=h1) Report
> Merging [#7541](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/b39468d2878554ba60863656364b4a95eda30685?src=pr&el=desc) will **decrease** coverage by `54.6%`.
> The diff coverage is `23.8%`.
[](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #7541 +/- ##
===========================================
- Coverage 86.76% 32.16% -54.61%
===========================================
Files 897 896 -1
Lines 42819 42752 -67
===========================================
- Hits 37153 13750 -23403
- Misses 5666 29002 +23336
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [...w/providers/amazon/aws/sensors/sagemaker\_tuning.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9zZW5zb3JzL3NhZ2VtYWtlcl90dW5pbmcucHk=) | `60% <0%> (-40%)` | :arrow_down: |
| [airflow/providers/amazon/aws/sensors/emr\_step.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9zZW5zb3JzL2Vtcl9zdGVwLnB5) | `50% <0%> (-50%)` | :arrow_down: |
| [...rflow/providers/amazon/aws/sensors/emr\_job\_flow.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9zZW5zb3JzL2Vtcl9qb2JfZmxvdy5weQ==) | `50% <0%> (-46.3%)` | :arrow_down: |
| [airflow/providers/amazon/aws/hooks/sns.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9zbnMucHk=) | `30.43% <0%> (-66%)` | :arrow_down: |
| [...providers/amazon/aws/sensors/sagemaker\_endpoint.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9zZW5zb3JzL3NhZ2VtYWtlcl9lbmRwb2ludC5weQ==) | `60% <0%> (-40%)` | :arrow_down: |
| [airflow/providers/amazon/aws/operators/ecs.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9vcGVyYXRvcnMvZWNzLnB5) | `27.61% <0%> (-57.82%)` | :arrow_down: |
| [...roviders/amazon/aws/sensors/sagemaker\_transform.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9zZW5zb3JzL3NhZ2VtYWtlcl90cmFuc2Zvcm0ucHk=) | `60% <0%> (-40%)` | :arrow_down: |
| [...providers/amazon/aws/sensors/sagemaker\_training.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9zZW5zb3JzL3NhZ2VtYWtlcl90cmFpbmluZy5weQ==) | `31.91% <0%> (-68.09%)` | :arrow_down: |
| [airflow/providers/amazon/aws/sensors/athena.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9zZW5zb3JzL2F0aGVuYS5weQ==) | `50% <0%> (-50%)` | :arrow_down: |
| [...ow/providers/amazon/aws/sensors/cloud\_formation.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9zZW5zb3JzL2Nsb3VkX2Zvcm1hdGlvbi5weQ==) | `42.5% <12.5%> (-57.5%)` | :arrow_down: |
| ... and [798 more](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Ξ = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=footer). Last update [b39468d...be5fe8f](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385089326
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -232,6 +251,38 @@ def get_resource_type(self, resource_type, region_name=None, config=None):
resource_type, endpoint_url=endpoint_url, config=config, verify=self.verify
)
+ @cached_property
+ def conn(self):
+ """Get the underlying boto3 client (cached).
+
+ The return value from this method is cached for efficiency.
+
+ :return: boto3.client or boto3.resource for the current
+ client/resource type and region
+ :rtype: boto3.client() or boto3.resource()
+ :raises AirflowException: self.client_type or self.resource_type are not
+ populated. These are usually specified to this class, by a subclass
+ __init__ method.
+ """
+ if self.client_type:
+ return self.get_client_type(self.client_type, region_name=self.region_name)
+ elif self.resource_type:
+ return self.get_resource_type(self.resource_type, region_name=self.region_name)
+ else:
+ raise AirflowException(
+ 'Either self.client_type or self.resource_type'
+ ' must be specified in the subclass')
Review comment:
I think it would be better to do the verification in the `__init__`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385542306
##########
File path: tests/providers/amazon/aws/operators/test_ecs.py
##########
@@ -171,7 +166,7 @@ def test_execute_with_failures(self):
}
)
- def test_wait_end_tasks(self):
+ def test_wait_end_tasks(self, aws_hook_mock):
Review comment:
Found a way to fix the issue with setUp, so this arg will be removed :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385543222
##########
File path: airflow/providers/amazon/aws/sensors/s3_prefix.py
##########
@@ -69,12 +70,11 @@ def __init__(self,
self.full_url = "s3://" + bucket_name + '/' + prefix
self.aws_conn_id = aws_conn_id
self.verify = verify
+ self.hook = S3Hook(aws_conn_id=self.aws_conn_id, verify=self.verify)
Review comment:
Resolved, thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385083943
##########
File path: tests/providers/amazon/aws/sensors/test_sqs.py
##########
@@ -18,8 +18,9 @@
import unittest
-from unittest.mock import MagicMock, patch
+from unittest.mock import MagicMock
+import mock
Review comment:
Please use `unittest.mock`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385002486
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -232,6 +243,23 @@ def get_resource_type(self, resource_type, region_name=None, config=None):
resource_type, endpoint_url=endpoint_url, config=config, verify=self.verify
)
+ @cached_property
+ def conn(self):
+ "Get the underlying boto3 client (cached)"
Review comment:
Thanks, resolved
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385080961
##########
File path: airflow/providers/amazon/aws/sensors/s3_prefix.py
##########
@@ -69,12 +70,11 @@ def __init__(self,
self.full_url = "s3://" + bucket_name + '/' + prefix
self.aws_conn_id = aws_conn_id
self.verify = verify
+ self.hook = S3Hook(aws_conn_id=self.aws_conn_id, verify=self.verify)
Review comment:
And here. Please check again all files you changed :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r387620707
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -232,6 +261,38 @@ def get_resource_type(self, resource_type, region_name=None, config=None):
resource_type, endpoint_url=endpoint_url, config=config, verify=self.verify
)
+ @cached_property
+ def conn(self):
+ """Get the underlying boto3 client (cached).
+
+ The return value from this method is cached for efficiency.
+
+ :return: boto3.client or boto3.resource for the current
+ client/resource type and region
+ :rtype: boto3.client() or boto3.resource()
+ :raises AirflowException: self.client_type or self.resource_type are not
+ populated. These are usually specified to this class, by a subclass
+ __init__ method.
+ """
+ if self.client_type:
+ return self.get_client_type(self.client_type, region_name=self.region_name)
+ elif self.resource_type:
+ return self.get_resource_type(self.resource_type, region_name=self.region_name)
+ else:
+ raise AirflowException(
+ 'Either self.client_type or self.resource_type'
+ ' must be specified in the subclass')
Review comment:
Yes, in the `__init__` I do the actual validation. The only way the client_type/resource_type could change after that is if a subclass changes it directly. It would represent a bug in the subclass implementation. Based on the Python docs on Exceptions, a NotImplementedError sounds like the best fit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r387621654
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -232,6 +261,38 @@ def get_resource_type(self, resource_type, region_name=None, config=None):
resource_type, endpoint_url=endpoint_url, config=config, verify=self.verify
)
+ @cached_property
+ def conn(self):
+ """Get the underlying boto3 client (cached).
+
+ The return value from this method is cached for efficiency.
+
+ :return: boto3.client or boto3.resource for the current
+ client/resource type and region
+ :rtype: boto3.client() or boto3.resource()
+ :raises AirflowException: self.client_type or self.resource_type are not
+ populated. These are usually specified to this class, by a subclass
+ __init__ method.
+ """
+ if self.client_type:
+ return self.get_client_type(self.client_type, region_name=self.region_name)
+ elif self.resource_type:
+ return self.get_resource_type(self.resource_type, region_name=self.region_name)
+ else:
+ raise AirflowException(
+ 'Either self.client_type or self.resource_type'
+ ' must be specified in the subclass')
+
+ def get_conn(self):
+ """ Get the underlying boto3 client (cached)
Review comment:
fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385083228
##########
File path: tests/providers/amazon/aws/operators/test_ecs.py
##########
@@ -171,7 +166,7 @@ def test_execute_with_failures(self):
}
)
- def test_wait_end_tasks(self):
+ def test_wait_end_tasks(self, aws_hook_mock):
Review comment:
(Why) Is this necessary?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385091513
##########
File path: airflow/providers/amazon/aws/sensors/athena.py
##########
@@ -57,13 +57,11 @@ def __init__(self,
super().__init__(*args, **kwargs)
self.aws_conn_id = aws_conn_id
self.query_execution_id = query_execution_id
- self.hook = None
self.sleep_time = sleep_time
self.max_retires = max_retires
+ self.hook = AWSAthenaHook(self.aws_conn_id, self.sleep_time)
Review comment:
Thanks - many of the existing sensor code was doing it during __init__, so I thought it was convention. Will change to do it during execution which makes more sense.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r384466443
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -232,6 +243,23 @@ def get_resource_type(self, resource_type, region_name=None, config=None):
resource_type, endpoint_url=endpoint_url, config=config, verify=self.verify
)
+ @cached_property
+ def conn(self):
+ "Get the underlying boto3 client (cached)"
Review comment:
Would be great if you could also add a type hint. Either in the description or the function header.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385601110
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -232,6 +261,38 @@ def get_resource_type(self, resource_type, region_name=None, config=None):
resource_type, endpoint_url=endpoint_url, config=config, verify=self.verify
)
+ @cached_property
+ def conn(self):
+ """Get the underlying boto3 client (cached).
+
+ The return value from this method is cached for efficiency.
+
+ :return: boto3.client or boto3.resource for the current
+ client/resource type and region
+ :rtype: boto3.client() or boto3.resource()
+ :raises AirflowException: self.client_type or self.resource_type are not
+ populated. These are usually specified to this class, by a subclass
+ __init__ method.
+ """
+ if self.client_type:
+ return self.get_client_type(self.client_type, region_name=self.region_name)
+ elif self.resource_type:
+ return self.get_resource_type(self.resource_type, region_name=self.region_name)
+ else:
+ raise AirflowException(
+ 'Either self.client_type or self.resource_type'
+ ' must be specified in the subclass')
Review comment:
In the `__init__` you are validating and here you are only returning the correct boto client, right?
Also.. what do you think of a different exception type like `ValueError` for example?
What do you think of this?
```suggestion
if self.client_type:
return self.get_client_type(self.client_type, region_name=self.region_name)
if self.resource_type:
return self.get_resource_type(self.resource_type, region_name=self.region_name)
raise ValueError('Could not get boto3 client!')
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385110474
##########
File path: tests/providers/amazon/aws/operators/test_ecs.py
##########
@@ -171,7 +166,7 @@ def test_execute_with_failures(self):
}
)
- def test_wait_end_tasks(self):
+ def test_wait_end_tasks(self, aws_hook_mock):
Review comment:
After moving the mock from setUp to above the class, all methods need to have this added (even if unused)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] kaxil edited a comment on issue #7541: [AIRFLOW-6822] AWS
hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on issue #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#issuecomment-609081689
I have Fixed conflicts. @feluelle Do you have any other pending issues/comments. If not I will merge this once if CI passes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r387642551
##########
File path: airflow/providers/amazon/aws/hooks/datasync.py
##########
@@ -29,10 +29,13 @@ class AWSDataSyncHook(AwsBaseHook):
"""
Interact with AWS DataSync.
+ Additional arguments (such as ``aws_conn_id``) may be specified and
+ are passed down to the underlying AwsBaseHook.
Review comment:
Docs build is failing since I removed the indent:
https://travis-ci.org/apache/airflow/jobs/658184121?utm_medium=notification&utm_source=github_status
Adding it back, lets see
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385605042
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -232,6 +261,38 @@ def get_resource_type(self, resource_type, region_name=None, config=None):
resource_type, endpoint_url=endpoint_url, config=config, verify=self.verify
)
+ @cached_property
+ def conn(self):
+ """Get the underlying boto3 client (cached).
+
+ The return value from this method is cached for efficiency.
+
+ :return: boto3.client or boto3.resource for the current
+ client/resource type and region
+ :rtype: boto3.client() or boto3.resource()
+ :raises AirflowException: self.client_type or self.resource_type are not
+ populated. These are usually specified to this class, by a subclass
+ __init__ method.
+ """
+ if self.client_type:
+ return self.get_client_type(self.client_type, region_name=self.region_name)
+ elif self.resource_type:
+ return self.get_resource_type(self.resource_type, region_name=self.region_name)
+ else:
+ raise AirflowException(
+ 'Either self.client_type or self.resource_type'
+ ' must be specified in the subclass')
+
+ def get_conn(self):
+ """ Get the underlying boto3 client (cached)
Review comment:
```suggestion
"""
Get the underlying boto3 client (cached)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385542156
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -47,11 +48,29 @@ class AwsBaseHook(BaseHook):
:param verify: Whether or not to verify SSL certificates.
https://boto3.amazonaws.com/v1/documentation/api/latest/reference/core/session.html
:type verify: str or bool
+ :param str region_name: AWS Region name to use. If this is None then the default boto3
+ behaviour is used.
+ :param str client_type: boto3 client_type used when creating boto3.client(). For
+ example, 's3', 'emr', etc. Provided by specific hooks for these clients which
+ subclass AwsBaseHook.
+ :param str resource_type: boto3 resource_type used when creating boto3.resource(). For
+ example, 's3'. Provided by specific hooks for these resources which
+ subclass AwsBaseHook.
Review comment:
Resolved, thanks.
I preferred the conciseness of :param <type> <var>: xyz
But have changed back to :param + :type
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385542185
##########
File path: tests/providers/amazon/aws/sensors/test_sqs.py
##########
@@ -18,8 +18,9 @@
import unittest
-from unittest.mock import MagicMock, patch
+from unittest.mock import MagicMock
+import mock
Review comment:
Resolved, thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385002535
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -232,6 +243,23 @@ def get_resource_type(self, resource_type, region_name=None, config=None):
resource_type, endpoint_url=endpoint_url, config=config, verify=self.verify
)
+ @cached_property
+ def conn(self):
+ "Get the underlying boto3 client (cached)"
+ if self.client_type:
+ return self.get_client_type(self.client_type, region_name=self.region_name)
+ elif self.resource_type:
+ return self.get_resource_type(self.resource_type, region_name=self.region_name)
+ else:
+ raise AirflowException(
+ 'Either self.client_type or self.resource_type'
+ ' must be specified in the subclass')
+
+ def get_conn(self):
+ "Get the underlying boto3 client (cached)"
Review comment:
Thanks, resolved
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io commented on issue #7541: [AIRFLOW-6822] AWS
hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#issuecomment-591909170
# [Codecov](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=h1) Report
> Merging [#7541](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/3f293001d6cf4674edab36d9981ad8407ecc9043?src=pr&el=desc) will **decrease** coverage by `0.33%`.
> The diff coverage is `97.8%`.
[](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #7541 +/- ##
==========================================
- Coverage 86.86% 86.52% -0.34%
==========================================
Files 896 896
Lines 42649 42581 -68
==========================================
- Hits 37046 36845 -201
- Misses 5603 5736 +133
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [airflow/providers/amazon/aws/hooks/datasync.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9kYXRhc3luYy5weQ==) | `16.66% <0%> (-0.12%)` | :arrow_down: |
| [airflow/providers/amazon/aws/hooks/s3.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9zMy5weQ==) | `96.58% <100%> (-0.02%)` | :arrow_down: |
| [airflow/providers/amazon/aws/sensors/redshift.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9zZW5zb3JzL3JlZHNoaWZ0LnB5) | `100% <100%> (ΓΈ)` | :arrow_up: |
| [...flow/providers/amazon/aws/hooks/lambda\_function.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9sYW1iZGFfZnVuY3Rpb24ucHk=) | `100% <100%> (ΓΈ)` | :arrow_up: |
| [airflow/providers/amazon/aws/hooks/sagemaker.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9zYWdlbWFrZXIucHk=) | `87.55% <100%> (+0.29%)` | :arrow_up: |
| [airflow/providers/amazon/aws/hooks/redshift.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9yZWRzaGlmdC5weQ==) | `75% <100%> (-0.87%)` | :arrow_down: |
| [...w/providers/amazon/aws/sensors/sagemaker\_tuning.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9zZW5zb3JzL3NhZ2VtYWtlcl90dW5pbmcucHk=) | `100% <100%> (ΓΈ)` | :arrow_up: |
| [airflow/providers/amazon/aws/sensors/sqs.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9zZW5zb3JzL3Nxcy5weQ==) | `100% <100%> (ΓΈ)` | :arrow_up: |
| [airflow/providers/amazon/aws/hooks/glue\_catalog.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9ob29rcy9nbHVlX2NhdGFsb2cucHk=) | `100% <100%> (ΓΈ)` | :arrow_up: |
| [airflow/providers/amazon/aws/sensors/emr\_step.py](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvYW1hem9uL2F3cy9zZW5zb3JzL2Vtcl9zdGVwLnB5) | `100% <100%> (ΓΈ)` | :arrow_up: |
| ... and [31 more](https://codecov.io/gh/apache/airflow/pull/7541/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Ξ = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=footer). Last update [3f29300...f282ce5](https://codecov.io/gh/apache/airflow/pull/7541?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385081131
##########
File path: airflow/providers/amazon/aws/sensors/sqs.py
##########
@@ -56,6 +56,7 @@ def __init__(self,
self.aws_conn_id = aws_conn_id
self.max_messages = max_messages
self.wait_time_seconds = wait_time_seconds
+ self.hook = SQSHook(aws_conn_id=self.aws_conn_id)
Review comment:
Here, too.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on issue #7541: [AIRFLOW-6822] AWS hooks
should cache boto3 client
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#issuecomment-599458798
I'm taking a look at this PR and fixing the tests.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385080772
##########
File path: airflow/providers/amazon/aws/sensors/redshift.py
##########
@@ -43,9 +43,9 @@ def __init__(self,
self.cluster_identifier = cluster_identifier
self.target_status = target_status
self.aws_conn_id = aws_conn_id
+ self.hook = RedshiftHook(aws_conn_id=self.aws_conn_id)
Review comment:
Same here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385541955
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -232,6 +251,38 @@ def get_resource_type(self, resource_type, region_name=None, config=None):
resource_type, endpoint_url=endpoint_url, config=config, verify=self.verify
)
+ @cached_property
+ def conn(self):
+ """Get the underlying boto3 client (cached).
+
+ The return value from this method is cached for efficiency.
+
+ :return: boto3.client or boto3.resource for the current
+ client/resource type and region
+ :rtype: boto3.client() or boto3.resource()
+ :raises AirflowException: self.client_type or self.resource_type are not
+ populated. These are usually specified to this class, by a subclass
+ __init__ method.
+ """
+ if self.client_type:
+ return self.get_client_type(self.client_type, region_name=self.region_name)
+ elif self.resource_type:
+ return self.get_resource_type(self.resource_type, region_name=self.region_name)
+ else:
+ raise AirflowException(
+ 'Either self.client_type or self.resource_type'
+ ' must be specified in the subclass')
Review comment:
Resolved, thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385084222
##########
File path: airflow/providers/amazon/aws/hooks/athena.py
##########
@@ -28,8 +28,12 @@ class AWSAthenaHook(AwsBaseHook):
"""
Interact with AWS Athena to run, poll queries and return query results
- :param aws_conn_id: aws connection to use.
- :type aws_conn_id: str
+ Additional arguments (such as ``aws_conn_id``) may be specified and
+ are passed down to the underlying AwsBaseHook.
+
+ .. seealso::
+ :class:`~airflow.providers.amazon.aws.hooks.base_aws.AwsBaseHook`
Review comment:
I like this one π
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385605372
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -232,6 +261,38 @@ def get_resource_type(self, resource_type, region_name=None, config=None):
resource_type, endpoint_url=endpoint_url, config=config, verify=self.verify
)
+ @cached_property
+ def conn(self):
+ """Get the underlying boto3 client (cached).
+
+ The return value from this method is cached for efficiency.
+
+ :return: boto3.client or boto3.resource for the current
+ client/resource type and region
+ :rtype: boto3.client() or boto3.resource()
+ :raises AirflowException: self.client_type or self.resource_type are not
+ populated. These are usually specified to this class, by a subclass
+ __init__ method.
+ """
+ if self.client_type:
+ return self.get_client_type(self.client_type, region_name=self.region_name)
+ elif self.resource_type:
+ return self.get_resource_type(self.resource_type, region_name=self.region_name)
+ else:
+ raise AirflowException(
+ 'Either self.client_type or self.resource_type'
+ ' must be specified in the subclass')
+
+ def get_conn(self):
+ """ Get the underlying boto3 client (cached)
+
+ This method is just a simple wrapper that calls the conn method
+ so that caching works as intended. It exists for compatibility
+ with subclasses that rely on a super().get_conn() method.
Review comment:
Please do not indent here :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r384461364
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -232,6 +243,23 @@ def get_resource_type(self, resource_type, region_name=None, config=None):
resource_type, endpoint_url=endpoint_url, config=config, verify=self.verify
)
+ @cached_property
+ def conn(self):
+ "Get the underlying boto3 client (cached)"
+ if self.client_type:
+ return self.get_client_type(self.client_type, region_name=self.region_name)
+ elif self.resource_type:
+ return self.get_resource_type(self.resource_type, region_name=self.region_name)
+ else:
+ raise AirflowException(
+ 'Either self.client_type or self.resource_type'
+ ' must be specified in the subclass')
+
+ def get_conn(self):
+ "Get the underlying boto3 client (cached)"
Review comment:
```suggestion
"""Get the underlying boto3 client (cached)"""
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r384462202
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -49,9 +50,19 @@ class AwsBaseHook(BaseHook):
:type verify: str or bool
"""
- def __init__(self, aws_conn_id="aws_default", verify=None):
+ def __init__(
+ self,
+ aws_conn_id="aws_default",
+ verify=None,
+ region_name=None,
+ client_type=None,
+ resource_type=None
Review comment:
Please update the documentation accordingly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385517283
##########
File path: tests/providers/amazon/aws/operators/test_ecs.py
##########
@@ -48,12 +48,10 @@
]
}
-
+# pylint: disable=unused-argument
+@mock.patch('airflow.providers.amazon.aws.operators.ecs.AwsBaseHook')
Review comment:
Agreed, it _should_ work :) I'll try and do some more digging. There must be a good reason why it isn't working like that at the moment, and I don't want to have flaky tests.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385091780
##########
File path: tests/providers/amazon/aws/operators/test_ecs.py
##########
@@ -48,12 +48,10 @@
]
}
-
+# pylint: disable=unused-argument
+@mock.patch('airflow.providers.amazon.aws.operators.ecs.AwsBaseHook')
Review comment:
I couldn't get it to work correctly with setUp, but will try again in case
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] kaxil commented on issue #7541: [AIRFLOW-6822] AWS hooks
should cache boto3 client
Posted by GitBox <gi...@apache.org>.
kaxil commented on issue #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#issuecomment-609081689
Fixed conflicts
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r387623564
##########
File path: tests/providers/amazon/aws/sensors/test_sqs.py
##########
@@ -18,7 +18,7 @@
import unittest
-from unittest.mock import MagicMock, patch
+import unittest.mock as mock
Review comment:
I use @mock.patch so it should work. Will see what Travis says :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385139258
##########
File path: tests/providers/amazon/aws/operators/test_ecs.py
##########
@@ -171,7 +166,7 @@ def test_execute_with_failures(self):
}
)
- def test_wait_end_tasks(self):
+ def test_wait_end_tasks(self, aws_hook_mock):
Review comment:
Okay, but can you please then add the pylint disable rule only to these lines. So that other unused arguments won't be disabled.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385111177
##########
File path: tests/providers/amazon/aws/operators/test_ecs.py
##########
@@ -48,12 +48,10 @@
]
}
-
+# pylint: disable=unused-argument
+@mock.patch('airflow.providers.amazon.aws.operators.ecs.AwsBaseHook')
Review comment:
No luck. I can't mock the hook properly if I move it back to above the setUp, regardless of how I patch it. I'm not sure how it was working before...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385542845
##########
File path: tests/providers/amazon/aws/operators/test_ecs.py
##########
@@ -48,12 +48,10 @@
]
}
-
+# pylint: disable=unused-argument
+@mock.patch('airflow.providers.amazon.aws.operators.ecs.AwsBaseHook')
Review comment:
Found a way to fix this.
I had to call `get_hook()` during the `setUp `function, while the hook was still mocked, to ensure that the operator saves the mocked hook in its `self.hook`
If I call `get_hook()` for the first time _after_ the `setUp` (eg by the Operator execute) then it is no longer mocked.
I guess this makes sense but it was really unexpected :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r384466706
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -232,6 +243,23 @@ def get_resource_type(self, resource_type, region_name=None, config=None):
resource_type, endpoint_url=endpoint_url, config=config, verify=self.verify
)
+ @cached_property
+ def conn(self):
+ "Get the underlying boto3 client (cached)"
+ if self.client_type:
+ return self.get_client_type(self.client_type, region_name=self.region_name)
+ elif self.resource_type:
+ return self.get_resource_type(self.resource_type, region_name=self.region_name)
+ else:
+ raise AirflowException(
+ 'Either self.client_type or self.resource_type'
+ ' must be specified in the subclass')
+
+ def get_conn(self):
+ "Get the underlying boto3 client (cached)"
Review comment:
Would be great if you could also add a type hint for the return value. Either in the description or the function header.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385542845
##########
File path: tests/providers/amazon/aws/operators/test_ecs.py
##########
@@ -48,12 +48,10 @@
]
}
-
+# pylint: disable=unused-argument
+@mock.patch('airflow.providers.amazon.aws.operators.ecs.AwsBaseHook')
Review comment:
Found a way to fix this.
I had to call `get_hook()` during the `setUp()` function, while the hook was still mocked, to ensure that the operator saves the mocked hook in its `self.hook`
If I call `get_hook()` for the first time _after_ the `setUp` - (eg by the Operator execute) then it is no longer mocked.
I guess this makes sense but it was really unexpected :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r387621500
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -232,6 +261,38 @@ def get_resource_type(self, resource_type, region_name=None, config=None):
resource_type, endpoint_url=endpoint_url, config=config, verify=self.verify
)
+ @cached_property
+ def conn(self):
+ """Get the underlying boto3 client (cached).
Review comment:
Thanks, there were some older methods with a similar style. I'll update these to all be consistent.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on issue #7541: [AIRFLOW-6822] AWS
hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on issue #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#issuecomment-592439179
Not sure what happened but my changes were reverted while rebasing... redoing them now
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r387622963
##########
File path: airflow/providers/amazon/aws/hooks/datasync.py
##########
@@ -29,10 +29,13 @@ class AWSDataSyncHook(AwsBaseHook):
"""
Interact with AWS DataSync.
+ Additional arguments (such as ``aws_conn_id``) may be specified and
+ are passed down to the underlying AwsBaseHook.
Review comment:
Thanks, fixed up
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r384460501
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -232,6 +243,23 @@ def get_resource_type(self, resource_type, region_name=None, config=None):
resource_type, endpoint_url=endpoint_url, config=config, verify=self.verify
)
+ @cached_property
+ def conn(self):
+ "Get the underlying boto3 client (cached)"
Review comment:
```suggestion
"""Get the underlying boto3 client (cached)"""
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385604962
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -232,6 +261,38 @@ def get_resource_type(self, resource_type, region_name=None, config=None):
resource_type, endpoint_url=endpoint_url, config=config, verify=self.verify
)
+ @cached_property
+ def conn(self):
+ """Get the underlying boto3 client (cached).
Review comment:
```suggestion
"""
Get the underlying boto3 client (cached).
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385082524
##########
File path: tests/providers/amazon/aws/operators/test_ecs.py
##########
@@ -48,12 +48,10 @@
]
}
-
+# pylint: disable=unused-argument
+@mock.patch('airflow.providers.amazon.aws.operators.ecs.AwsBaseHook')
Review comment:
What is the point of moving it up to class level instead of `setUp` ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] kaxil merged pull request #7541: [AIRFLOW-6822] AWS hooks
should cache boto3 client
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r385610240
##########
File path: tests/providers/amazon/aws/sensors/test_sqs.py
##########
@@ -18,7 +18,7 @@
import unittest
-from unittest.mock import MagicMock, patch
+import unittest.mock as mock
Review comment:
I think this will break things. You removed `patch` but it is still required at line 75 and 97.
Wasn't it correct before?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] feluelle commented on a change in pull request #7541:
[AIRFLOW-6822] AWS hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#discussion_r384464572
##########
File path: airflow/providers/amazon/aws/hooks/base_aws.py
##########
@@ -49,9 +50,19 @@ class AwsBaseHook(BaseHook):
:type verify: str or bool
"""
- def __init__(self, aws_conn_id="aws_default", verify=None):
+ def __init__(
+ self,
+ aws_conn_id="aws_default",
+ verify=None,
+ region_name=None,
+ client_type=None,
+ resource_type=None
Review comment:
And also update the docs of the hooks which are based on this hook.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] kaxil commented on issue #7541: [AIRFLOW-6822] AWS hooks
should cache boto3 client
Posted by GitBox <gi...@apache.org>.
kaxil commented on issue #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#issuecomment-596544616
Some static checks have failed:
```
airflow/providers/amazon/aws/hooks/batch_client.py:40:1: F401 'airflow.utils.log.logging_mixin.LoggingMixin' imported but unused
************* Module airflow.providers.amazon.aws.hooks.batch_client
airflow/providers/amazon/aws/hooks/batch_client.py:213:4: C0116: Missing function or method docstring (missing-function-docstring)
airflow/providers/amazon/aws/hooks/batch_client.py:40:0: W0611: Unused LoggingMixin imported from airflow.utils.log.logging_mixin (unused-import)
************* Module airflow.contrib.operators.awsbatch_operator
airflow/contrib/operators/awsbatch_operator.py:43:0: R0901: Too many ancestors (8/7) (too-many-ancestors)
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [airflow] baolsen commented on issue #7541: [AIRFLOW-6822] AWS
hooks should cache boto3 client
Posted by GitBox <gi...@apache.org>.
baolsen commented on issue #7541: [AIRFLOW-6822] AWS hooks should cache boto3 client
URL: https://github.com/apache/airflow/pull/7541#issuecomment-610782287
Thanks @kaxil , @feluelle , @ashb . Really appreciate the help here :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services