Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MED-100 Make read_timeout default to None, only use it if set #813

Merged
merged 3 commits into from
Oct 31, 2024

Conversation

rzvoncek
Copy link
Contributor

@rzvoncek rzvoncek commented Oct 9, 2024

Fixes #814

@rzvoncek rzvoncek force-pushed the radovan/fix-default-timeouts branch 7 times, most recently from 681413b to d02e684 Compare October 9, 2024 15:00
@rzvoncek rzvoncek marked this pull request as ready for review October 10, 2024 08:40
@rzvoncek rzvoncek changed the title Make read_timeout default to None, only use it if set MED-100 Make read_timeout default to None, only use it if set Oct 10, 2024
@@ -158,7 +158,7 @@ async def _download_blob(self, src: str, dest: str):
stream = await self.gcs_storage.download_stream(
bucket=self.bucket_name,
object_name=object_key,
timeout=self.read_timeout if self.read_timeout is not None else -1,
timeout=self.read_timeout,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Disabling the timeout seemed to be done by setting the value to -1 previously and now it's set to None. Does that work as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually do not know.
Since it's really tedious to find this out empirically, let's just default to -1 for GCS. The commit 3dd0a55 does that.

@rzvoncek rzvoncek force-pushed the radovan/fix-default-timeouts branch 10 times, most recently from 34f295c to 1008c55 Compare October 29, 2024 14:27
@rzvoncek rzvoncek force-pushed the radovan/fix-default-timeouts branch 7 times, most recently from 798c38c to d6a037f Compare October 30, 2024 10:28
@rzvoncek rzvoncek force-pushed the radovan/fix-default-timeouts branch 9 times, most recently from cbaec83 to 73442f2 Compare October 30, 2024 13:19
Copy link

sonarcloud bot commented Oct 30, 2024

Copy link
Contributor

@adejanovski adejanovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the efforts on fixing the tests!

@adejanovski adejanovski merged commit 1394391 into master Oct 31, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The read_timeout should default to nothing instead of 60 seconds
2 participants