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/12/02 20:07:31 UTC
[GitHub] [airflow] XD-DENG opened a new pull request #12763: Avoid log spam when pull image in DockerOperator
XD-DENG opened a new pull request #12763:
URL: https://github.com/apache/airflow/pull/12763
Closes https://github.com/apache/airflow/issues/12576
- Fix the log spam issue reported in https://github.com/apache/airflow/issues/12576 . It's easy to validate the issue
```python
import docker
cli = docker.APIClient()
for x in cli.pull(self.image, stream=True, decode=True):
print(x)
```
- The change in this PR actually also makes the log more meaningful. When we pull an image, normally there are a few layers, simply printing the `status` is not very meaningful. It's much better to print the `status` by `id`.
#### Sample output when we pull image using `docker.APIClient.pull(stream=True)`
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/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 commented on a change in pull request #12763: Avoid log spam & have more meaningful log when pull image in DockerOperator
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12763:
URL: https://github.com/apache/airflow/pull/12763#discussion_r534522587
##########
File path: airflow/providers/docker/operators/docker.py
##########
@@ -282,13 +282,20 @@ def execute(self, context) -> Optional[str]:
raise Exception("The 'cli' should be initialized before!")
# Pull the docker image if `force_pull` is set or image does not exist locally
+ # pylint: disable=too-many-nested-blocks
if self.force_pull or not self.cli.images(name=self.image):
self.log.info('Pulling docker image %s', self.image)
+ latest_status = {}
for output in self.cli.pull(self.image, stream=True, decode=True):
if isinstance(output, str):
self.log.info("%s", output)
if isinstance(output, dict) and 'status' in output:
- self.log.info("%s", output['status'])
+ if 'id' in output:
+ if latest_status.get(output['id']) != output['status']:
+ self.log.info("%s: %s", output['id'], output['status'])
+ latest_status[output['id']] = output['status']
Review comment:
There's a chance that `latest_status.get(output.get("id")) != output["status"]` will also work
----------------------------------------------------------------
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 #12763: Avoid log spam & have more meaningful log when pull image in DockerOperator
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12763:
URL: https://github.com/apache/airflow/pull/12763#discussion_r534567095
##########
File path: airflow/providers/docker/operators/docker.py
##########
@@ -282,13 +282,20 @@ def execute(self, context) -> Optional[str]:
raise Exception("The 'cli' should be initialized before!")
# Pull the docker image if `force_pull` is set or image does not exist locally
+ # pylint: disable=too-many-nested-blocks
if self.force_pull or not self.cli.images(name=self.image):
self.log.info('Pulling docker image %s', self.image)
+ latest_status = {}
for output in self.cli.pull(self.image, stream=True, decode=True):
if isinstance(output, str):
self.log.info("%s", output)
if isinstance(output, dict) and 'status' in output:
- self.log.info("%s", output['status'])
+ if 'id' in output:
+ if latest_status.get(output['id']) != output['status']:
+ self.log.info("%s: %s", output['id'], output['status'])
+ latest_status[output['id']] = output['status']
Review comment:
The maybe we should revert the ifs:
```py
if 'id' not in output:
self.log.info("%s", output['status'])
continue
output_id, output_status = output["id"], output["status"]
if latest_status.get(output_id) != output_status:
self.log.info("%s: %s", output_id, output_status)
latest_status[output_id] = output_status
```
I'm not a fan of too many nested ifs as pylint likes to complain about them 😉
----------------------------------------------------------------
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 #12763: Avoid log spam & have more meaningful log when pull image in DockerOperator
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12763:
URL: https://github.com/apache/airflow/pull/12763#discussion_r534520589
##########
File path: airflow/providers/docker/operators/docker.py
##########
@@ -282,13 +282,20 @@ def execute(self, context) -> Optional[str]:
raise Exception("The 'cli' should be initialized before!")
# Pull the docker image if `force_pull` is set or image does not exist locally
+ # pylint: disable=too-many-nested-blocks
if self.force_pull or not self.cli.images(name=self.image):
self.log.info('Pulling docker image %s', self.image)
+ latest_status = {}
for output in self.cli.pull(self.image, stream=True, decode=True):
if isinstance(output, str):
self.log.info("%s", output)
Review comment:
```suggestion
self.log.info("%s", output)
continue
```
To avoid additional checks
----------------------------------------------------------------
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 #12763: Avoid log spam & have more meaningful log when pull image in DockerOperator
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12763:
URL: https://github.com/apache/airflow/pull/12763#discussion_r534567095
##########
File path: airflow/providers/docker/operators/docker.py
##########
@@ -282,13 +282,20 @@ def execute(self, context) -> Optional[str]:
raise Exception("The 'cli' should be initialized before!")
# Pull the docker image if `force_pull` is set or image does not exist locally
+ # pylint: disable=too-many-nested-blocks
if self.force_pull or not self.cli.images(name=self.image):
self.log.info('Pulling docker image %s', self.image)
+ latest_status = {}
for output in self.cli.pull(self.image, stream=True, decode=True):
if isinstance(output, str):
self.log.info("%s", output)
if isinstance(output, dict) and 'status' in output:
- self.log.info("%s", output['status'])
+ if 'id' in output:
+ if latest_status.get(output['id']) != output['status']:
+ self.log.info("%s: %s", output['id'], output['status'])
+ latest_status[output['id']] = output['status']
Review comment:
The maye we should revert the ifs:
```py
if 'id' not in output:
self.log.info("%s", output['status'])
continue
output_id, output_status = output["id"], output["status"]
if latest_status.get(output_id) != output_status:
self.log.info("%s: %s", output_id, output_status)
latest_status[output_id] = output_status
```
I'm not a fan of too many nested ifs as pylint likes to complain about them 😉
----------------------------------------------------------------
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 #12763: Avoid log spam & have more meaningful log when pull image in DockerOperator
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12763:
URL: https://github.com/apache/airflow/pull/12763#discussion_r534567095
##########
File path: airflow/providers/docker/operators/docker.py
##########
@@ -282,13 +282,20 @@ def execute(self, context) -> Optional[str]:
raise Exception("The 'cli' should be initialized before!")
# Pull the docker image if `force_pull` is set or image does not exist locally
+ # pylint: disable=too-many-nested-blocks
if self.force_pull or not self.cli.images(name=self.image):
self.log.info('Pulling docker image %s', self.image)
+ latest_status = {}
for output in self.cli.pull(self.image, stream=True, decode=True):
if isinstance(output, str):
self.log.info("%s", output)
if isinstance(output, dict) and 'status' in output:
- self.log.info("%s", output['status'])
+ if 'id' in output:
+ if latest_status.get(output['id']) != output['status']:
+ self.log.info("%s: %s", output['id'], output['status'])
+ latest_status[output['id']] = output['status']
Review comment:
The maye we should revert the ifs:
```py
if 'id' not in output:
self.log.info("%s", output['status'])
continue
output_id = output.get("id")
output_status = output.get("status")
if latest_status.get(output_id) != output_status:
self.log.info("%s: %s", output_id, output_status)
latest_status[output_id] = output_status
```
I'm not a fan of too many nested ifs as pylint likes to complain about them 😉
----------------------------------------------------------------
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] XD-DENG merged pull request #12763: Avoid log spam & have more meaningful log when pull image in DockerOperator
Posted by GitBox <gi...@apache.org>.
XD-DENG merged pull request #12763:
URL: https://github.com/apache/airflow/pull/12763
----------------------------------------------------------------
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 #12763: Avoid log spam & have more meaningful log when pull image in DockerOperator
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12763:
URL: https://github.com/apache/airflow/pull/12763#discussion_r534520589
##########
File path: airflow/providers/docker/operators/docker.py
##########
@@ -282,13 +282,20 @@ def execute(self, context) -> Optional[str]:
raise Exception("The 'cli' should be initialized before!")
# Pull the docker image if `force_pull` is set or image does not exist locally
+ # pylint: disable=too-many-nested-blocks
if self.force_pull or not self.cli.images(name=self.image):
self.log.info('Pulling docker image %s', self.image)
+ latest_status = {}
for output in self.cli.pull(self.image, stream=True, decode=True):
if isinstance(output, str):
self.log.info("%s", output)
Review comment:
```suggestion
self.log.info("%s", output)
continue
```
----------------------------------------------------------------
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] XD-DENG commented on a change in pull request #12763: Avoid log spam & have more meaningful log when pull image in DockerOperator
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #12763:
URL: https://github.com/apache/airflow/pull/12763#discussion_r534525961
##########
File path: airflow/providers/docker/operators/docker.py
##########
@@ -282,13 +282,20 @@ def execute(self, context) -> Optional[str]:
raise Exception("The 'cli' should be initialized before!")
# Pull the docker image if `force_pull` is set or image does not exist locally
+ # pylint: disable=too-many-nested-blocks
if self.force_pull or not self.cli.images(name=self.image):
self.log.info('Pulling docker image %s', self.image)
+ latest_status = {}
for output in self.cli.pull(self.image, stream=True, decode=True):
if isinstance(output, str):
self.log.info("%s", output)
if isinstance(output, dict) and 'status' in output:
- self.log.info("%s", output['status'])
+ if 'id' in output:
+ if latest_status.get(output['id']) != output['status']:
+ self.log.info("%s: %s", output['id'], output['status'])
+ latest_status[output['id']] = output['status']
Review comment:
This change you proposed may not work, because if this `if` is not matched, it goes to `else` and print the `status`. Then it's no difference from the earlier "spamming" status.
This is why I have the nested `if`s here. may you let me know if this clarification makes sense to you?
----------------------------------------------------------------
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] XD-DENG commented on pull request #12763: Avoid log spam & have more meaningful log when pull image in DockerOperator
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #12763:
URL: https://github.com/apache/airflow/pull/12763#issuecomment-737505780
Hi @turbaszek , I added https://github.com/apache/airflow/pull/12763/commits/b54303640b2e30ad336211d1d9684e47ffc9886a to address cases in the screenshot below
<img width="1199" alt="Default__Python_" src="https://user-images.githubusercontent.com/11539188/100933280-64533400-34ed-11eb-8684-6d34154f68c6.png">
Mind taking another 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 #12763: Avoid log spam & have more meaningful log when pull image in DockerOperator
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12763:
URL: https://github.com/apache/airflow/pull/12763#discussion_r534522050
##########
File path: airflow/providers/docker/operators/docker.py
##########
@@ -282,13 +282,20 @@ def execute(self, context) -> Optional[str]:
raise Exception("The 'cli' should be initialized before!")
# Pull the docker image if `force_pull` is set or image does not exist locally
+ # pylint: disable=too-many-nested-blocks
if self.force_pull or not self.cli.images(name=self.image):
self.log.info('Pulling docker image %s', self.image)
+ latest_status = {}
for output in self.cli.pull(self.image, stream=True, decode=True):
if isinstance(output, str):
self.log.info("%s", output)
if isinstance(output, dict) and 'status' in output:
- self.log.info("%s", output['status'])
+ if 'id' in output:
+ if latest_status.get(output['id']) != output['status']:
+ self.log.info("%s: %s", output['id'], output['status'])
+ latest_status[output['id']] = output['status']
Review comment:
```suggestion
if 'id' in output and latest_status.get(output['id']) != output['status']:
self.log.info("%s: %s", output['id'], output['status'])
latest_status[output['id']] = output['status']
```
----------------------------------------------------------------
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] XD-DENG commented on a change in pull request #12763: Avoid log spam & have more meaningful log when pull image in DockerOperator
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #12763:
URL: https://github.com/apache/airflow/pull/12763#discussion_r535470671
##########
File path: airflow/providers/docker/operators/docker.py
##########
@@ -282,13 +282,20 @@ def execute(self, context) -> Optional[str]:
raise Exception("The 'cli' should be initialized before!")
# Pull the docker image if `force_pull` is set or image does not exist locally
+ # pylint: disable=too-many-nested-blocks
if self.force_pull or not self.cli.images(name=self.image):
self.log.info('Pulling docker image %s', self.image)
+ latest_status = {}
for output in self.cli.pull(self.image, stream=True, decode=True):
if isinstance(output, str):
self.log.info("%s", output)
if isinstance(output, dict) and 'status' in output:
- self.log.info("%s", output['status'])
+ if 'id' in output:
+ if latest_status.get(output['id']) != output['status']:
+ self.log.info("%s: %s", output['id'], output['status'])
+ latest_status[output['id']] = output['status']
Review comment:
Actually we already cannot avoid having to apply `disable=too-many-nested-blocks`, but your suggestion above is definitely fair.
Have addressed it in https://github.com/apache/airflow/pull/12763/commits/23e81de0b7170d6132b183319bcd73729e508a52 (with extremely minor change)
----------------------------------------------------------------
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] github-actions[bot] commented on pull request #12763: Avoid log spam & have more meaningful log when pull image in DockerOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12763:
URL: https://github.com/apache/airflow/pull/12763#issuecomment-737475736
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
----------------------------------------------------------------
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