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/16 19:01:45 UTC
[GitHub] [airflow] KevYuen opened a new pull request #13115: Decode Remote Google Logs
KevYuen opened a new pull request #13115:
URL: https://github.com/apache/airflow/pull/13115
The `Blob.download_as_string` function returns a byte which cause
the log result to be displayed in a single line like:
b"line1\nline2"
instead of
line1
line2
added an isinstance check to make sure it doesn't break if it
returns string in some case and not others
all the logs is in a single line:
<img width="959" alt="image" src="https://user-images.githubusercontent.com/4744103/102393049-daec3900-3fa5-11eb-8b37-361adac86ba4.png">
Version: 2.0.0rc3
TODO:
---
- [ ] Update test to include case with byte return value instead of string
----------------------------------------------------------------
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] KevYuen commented on a change in pull request #13115: Decode Remote Google Logs
Posted by GitBox <gi...@apache.org>.
KevYuen commented on a change in pull request #13115:
URL: https://github.com/apache/airflow/pull/13115#discussion_r545108821
##########
File path: airflow/providers/google/cloud/log/gcs_task_handler.py
##########
@@ -153,6 +153,10 @@ def _read(self, ti, try_number, metadata=None):
try:
blob = storage.Blob.from_string(remote_loc, self.client)
remote_log = blob.download_as_string()
+
+ if isinstance(remote_log, bytes):
+ remote_log = remote_log.decode()
Review comment:
You're right, looks like the method is deprecated and it is now an alias for `download_as_byte`
https://googleapis.dev/python/storage/latest/blobs.html#google.cloud.storage.blob.Blob.download_as_string
Will update the PR
----------------------------------------------------------------
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] potiuk commented on pull request #13115: Decode Remote Google Logs
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13115:
URL: https://github.com/apache/airflow/pull/13115#issuecomment-750881710
Thanks @KevYuen !
----------------------------------------------------------------
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 #13115: Decode Remote Google Logs
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #13115:
URL: https://github.com/apache/airflow/pull/13115#discussion_r544696994
##########
File path: airflow/providers/google/cloud/log/gcs_task_handler.py
##########
@@ -153,6 +153,10 @@ def _read(self, ti, try_number, metadata=None):
try:
blob = storage.Blob.from_string(remote_loc, self.client)
remote_log = blob.download_as_string()
+
+ if isinstance(remote_log, bytes):
+ remote_log = remote_log.decode()
Review comment:
I would say that this should be part of `download_as_string` method. The name suggest that the output should be a `str` not `bytes` so fixing it there may solve some issues in future.
----------------------------------------------------------------
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] KevYuen commented on pull request #13115: Decode Remote Google Logs
Posted by GitBox <gi...@apache.org>.
KevYuen commented on pull request #13115:
URL: https://github.com/apache/airflow/pull/13115#issuecomment-750304757
@turbaszek I missed that! 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
[GitHub] [airflow] turbaszek commented on pull request #13115: Decode Remote Google Logs
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #13115:
URL: https://github.com/apache/airflow/pull/13115#issuecomment-749160553
@KevYuen pleas take a look at the pylint issues on CI
----------------------------------------------------------------
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] potiuk commented on pull request #13115: Decode Remote Google Logs
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13115:
URL: https://github.com/apache/airflow/pull/13115#issuecomment-788979127
Those changes are available even now in already released 2.0.0 version of Google providers - see the changelog: https://airflow.apache.org/docs/apache-airflow-providers-google/2.0.0/index.html
You can upgrade even now to the new google providers without waiting for the new Airflow version (which will include it by default).
----------------------------------------------------------------
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 #13115: Decode Remote Google Logs
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13115:
URL: https://github.com/apache/airflow/pull/13115#issuecomment-747489702
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
[GitHub] [airflow] brenocosta0901 commented on pull request #13115: Decode Remote Google Logs
Posted by GitBox <gi...@apache.org>.
brenocosta0901 commented on pull request #13115:
URL: https://github.com/apache/airflow/pull/13115#issuecomment-788912438
@KevYuen @potiuk I would like to know if these changes will enter on the next Airflow version. I'm facing this problem when migrating to version 2.0.1.
----------------------------------------------------------------
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] potiuk merged pull request #13115: Decode Remote Google Logs
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #13115:
URL: https://github.com/apache/airflow/pull/13115
----------------------------------------------------------------
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] potiuk edited a comment on pull request #13115: Decode Remote Google Logs
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #13115:
URL: https://github.com/apache/airflow/pull/13115#issuecomment-788979127
Those changes are available even now in already released 2.0.0 version of Google providers - see the changelog: https://airflow.apache.org/docs/apache-airflow-providers-google/2.0.0/index.html #changelog
You can upgrade even now to the new google providers without waiting for the new Airflow version (which will include it by default).
----------------------------------------------------------------
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] KevYuen commented on pull request #13115: Decode Remote Google Logs
Posted by GitBox <gi...@apache.org>.
KevYuen commented on pull request #13115:
URL: https://github.com/apache/airflow/pull/13115#issuecomment-748996090
@turbaszek Is there anything I should do to get this PR merged? Apologies, I am not too familiar with the process.
----------------------------------------------------------------
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