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/18 18:04:04 UTC
[GitHub] [airflow] jmcarp opened a new pull request #13156: Add timeout option to gcs hook methods.
jmcarp opened a new pull request #13156:
URL: https://github.com/apache/airflow/pull/13156
The google-cloud-storage client sets a timeout of 60s for all requests and defaults to uploading and downloading files in a single request. When working with large files or slow connections, users might need to either increase the timeout or set a chunk size so that the sdk uses chunked uploads and downloads. This patch adds optional chunk_size and timeout arguments to the upload and download methods of the gcs hook and pins google-cloud-storage to a version that supports timeouts for all applicable methods.
----------------------------------------------------------------
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] xinbinhuang commented on a change in pull request #13156: Add timeout option to gcs hook methods.
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #13156:
URL: https://github.com/apache/airflow/pull/13156#discussion_r546084232
##########
File path: tests/providers/google/cloud/hooks/test_gcs.py
##########
@@ -672,7 +672,7 @@ def test_download_to_file(self, mock_service):
)
self.assertEqual(response, test_file)
- download_filename_method.assert_called_once_with(test_file)
+ download_filename_method.assert_called_once_with(test_file, timeout=60)
Review comment:
How about swapping the `timeout=60` with a variable as well? So that we don't need to hard code them in all test cases
----------------------------------------------------------------
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 #13156: Add timeout option to gcs hook methods.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13156:
URL: https://github.com/apache/airflow/pull/13156#issuecomment-748312923
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, 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] potiuk commented on a change in pull request #13156: Add timeout option to gcs hook methods.
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #13156:
URL: https://github.com/apache/airflow/pull/13156#discussion_r546856753
##########
File path: tests/providers/google/cloud/hooks/test_gcs.py
##########
@@ -672,7 +672,7 @@ def test_download_to_file(self, mock_service):
)
self.assertEqual(response, test_file)
- download_filename_method.assert_called_once_with(test_file)
+ download_filename_method.assert_called_once_with(test_file, timeout=60)
Review comment:
I think it's good to have them hard-coded :).
the tests should be DAMP rather than DRY https://stackoverflow.com/questions/6453235/what-does-damp-not-dry-mean-when-talking-about-unit-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
[GitHub] [airflow] potiuk merged pull request #13156: Add timeout option to gcs hook methods.
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #13156:
URL: https://github.com/apache/airflow/pull/13156
----------------------------------------------------------------
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