Skip to content

fix: Use appropriate timeouts #704

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jun 11, 2025
Merged

fix: Use appropriate timeouts #704

merged 5 commits into from
Jun 11, 2025

Conversation

janbuchar
Copy link
Contributor

@janbuchar janbuchar added the t-tooling Issues with this label are in the ownership of the tooling team. label Jun 9, 2025
@janbuchar janbuchar requested review from B4nan, barjin and Pijukatel June 9, 2025 15:41
@github-actions github-actions bot added this to the 116th sprint - Tooling team milestone Jun 9, 2025
@github-actions github-actions bot added the tested Temporary label used only programatically for some analytics. label Jun 9, 2025
B4nan
B4nan reviewed Jun 10, 2025
View reviewed changes
barjin
barjin requested changes Jun 10, 2025
View reviewed changes
Copy link
Contributor

@barjin barjin left a comment

Choose a reason for hiding this comment

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

+1 for extracting the consts somewhere, so we don't have those in multiple places

@janbuchar
Copy link
Contributor Author

Sorry for the confusion around secs/millis - I changed my approach when working on this and stopped when the client_timeouts test started to pass 🤦

@janbuchar janbuchar requested review from B4nan and barjin June 10, 2025 10:49
B4nan
B4nan reviewed Jun 10, 2025
View reviewed changes
@janbuchar janbuchar requested review from barjin and B4nan June 11, 2025 09:54
@janbuchar
Copy link
Contributor Author

@B4nan @barjin I switched to milliseconds almost everywhere and I think the result is in fact much cleaner. If you want to skim it, go ahead. Otherwise I'll merge it later today.

@Pijukatel
Copy link

+1 for extracting the consts somewhere, so we don't have those in multiple places

Basically same issue like:
apify/apify-sdk-python#9

barjin
barjin approved these changes Jun 11, 2025
View reviewed changes
Copy link
Contributor

@barjin barjin left a comment

Choose a reason for hiding this comment

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

lgtm, thanks! 🚀

@janbuchar janbuchar merged commit b896bf2 into master Jun 11, 2025
7 checks passed
@janbuchar janbuchar deleted the timeouts-rework branch June 11, 2025 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@B4nan B4nan B4nan approved these changes

@barjin barjin barjin approved these changes

@Pijukatel Pijukatel Awaiting requested review from Pijukatel

Assignees

@janbuchar janbuchar

Labels
t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics.
Projects
None yet
Milestone
116th sprint - Tooling team
Development

Successfully merging this pull request may close these issues.

Refactor client timeouts - parity with Python implemenation
4 participants
@janbuchar @Pijukatel @B4nan @barjin