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/08/21 13:27:40 UTC
[GitHub] [airflow] turbaszek opened a new pull request #10447: Add connection caching to KubernetesHook
turbaszek opened a new pull request #10447:
URL: https://github.com/apache/airflow/pull/10447
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
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).
----------------------------------------------------------------
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
[GitHub] [airflow] turbaszek merged pull request #10447: Add connection caching to KubernetesHook
Posted by GitBox <gi...@apache.org>.
turbaszek merged pull request #10447:
URL: https://github.com/apache/airflow/pull/10447
----------------------------------------------------------------
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
[GitHub] [airflow] mik-laj commented on a change in pull request #10447: Add connection caching to KubernetesHook
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10447:
URL: https://github.com/apache/airflow/pull/10447#discussion_r475190210
##########
File path: airflow/providers/cncf/kubernetes/hooks/kubernetes.py
##########
@@ -46,11 +46,14 @@ def __init__(
):
super().__init__()
self.conn_id = conn_id
+ self._client: Optional[client.ApiClient] = None
Review comment:
I think it's worth doing, but you have to be careful because we don't always want to have a cache. It depends on each 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
[GitHub] [airflow] turbaszek commented on a change in pull request #10447: Add connection caching to KubernetesHook
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10447:
URL: https://github.com/apache/airflow/pull/10447#discussion_r482845521
##########
File path: airflow/providers/cncf/kubernetes/hooks/kubernetes.py
##########
@@ -44,6 +45,7 @@ def __init__(self, conn_id: str = "kubernetes_default"):
super().__init__()
self.conn_id = conn_id
+ @cached_property
Review comment:
You are right, I fixed it
----------------------------------------------------------------
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
[GitHub] [airflow] mik-laj commented on a change in pull request #10447: Add connection caching to KubernetesHook
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10447:
URL: https://github.com/apache/airflow/pull/10447#discussion_r474820447
##########
File path: airflow/providers/cncf/kubernetes/hooks/kubernetes.py
##########
@@ -46,11 +46,14 @@ def __init__(
):
super().__init__()
self.conn_id = conn_id
+ self._client: Optional[client.ApiClient] = None
Review comment:
cached_property?
----------------------------------------------------------------
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
[GitHub] [airflow] mik-laj commented on a change in pull request #10447: Add connection caching to KubernetesHook
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10447:
URL: https://github.com/apache/airflow/pull/10447#discussion_r475152039
##########
File path: airflow/providers/cncf/kubernetes/hooks/kubernetes.py
##########
@@ -46,11 +46,14 @@ def __init__(
):
super().__init__()
self.conn_id = conn_id
+ self._client: Optional[client.ApiClient] = None
Review comment:
I don't think it's a big change. See: https://github.com/apache/airflow/blob/master/airflow/providers/amazon/aws/hooks/base_aws.py#L353-L367
----------------------------------------------------------------
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
[GitHub] [airflow] turbaszek commented on a change in pull request #10447: Add connection caching to KubernetesHook
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10447:
URL: https://github.com/apache/airflow/pull/10447#discussion_r474886643
##########
File path: airflow/providers/cncf/kubernetes/hooks/kubernetes.py
##########
@@ -46,11 +46,14 @@ def __init__(
):
super().__init__()
self.conn_id = conn_id
+ self._client: Optional[client.ApiClient] = None
Review comment:
I can do, but this introduces fewer changes. I think we do not have a unified pattern for this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #10447: Add connection caching to KubernetesHook
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #10447:
URL: https://github.com/apache/airflow/pull/10447#issuecomment-678290679
@roitvt would mind taking a look?
----------------------------------------------------------------
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
[GitHub] [airflow] turbaszek commented on a change in pull request #10447: Add connection caching to KubernetesHook
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10447:
URL: https://github.com/apache/airflow/pull/10447#discussion_r485019189
##########
File path: airflow/providers/cncf/kubernetes/hooks/kubernetes.py
##########
@@ -141,12 +148,11 @@ def get_pod_log_stream(
:param pod_name: pod name
:type pod_name: str
:param container: container name
- :type version: str
Review comment:
Removing as there's no version param
##########
File path: airflow/providers/cncf/kubernetes/hooks/kubernetes.py
##########
@@ -167,11 +173,10 @@ def get_pod_logs(
:param pod_name: pod name
:type pod_name: str
:param container: container name
- :type version: str
Review comment:
Removing as there's no version param
----------------------------------------------------------------
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
[GitHub] [airflow] mik-laj commented on a change in pull request #10447: Add connection caching to KubernetesHook
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10447:
URL: https://github.com/apache/airflow/pull/10447#discussion_r482838268
##########
File path: airflow/providers/cncf/kubernetes/hooks/kubernetes.py
##########
@@ -44,6 +45,7 @@ def __init__(self, conn_id: str = "kubernetes_default"):
super().__init__()
self.conn_id = conn_id
+ @cached_property
Review comment:
It is incorrect. It should be method. If you want, you could create a new property e.g. client and then this method can use this property.
----------------------------------------------------------------
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
[GitHub] [airflow] turbaszek commented on a change in pull request #10447: Add connection caching to KubernetesHook
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10447:
URL: https://github.com/apache/airflow/pull/10447#discussion_r475186657
##########
File path: airflow/providers/cncf/kubernetes/hooks/kubernetes.py
##########
@@ -46,11 +46,14 @@ def __init__(
):
super().__init__()
self.conn_id = conn_id
+ self._client: Optional[client.ApiClient] = None
Review comment:
Should we try to standardize it? What do you think?
----------------------------------------------------------------
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