-
Notifications
You must be signed in to change notification settings - Fork 570
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
Add a default timeout for filelock #2391
Conversation
Hi @edevil, thanks for raising the question. I'm not a fan of a 5min timeout since 1. it's already quite long for a user to debug 2. it's still too short when downloading huge files.
This way the new behavior will be similar to the current one, while still having a correct log message for the end user. What do you think? |
Hey @Wauplin, addressed your comments. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the quick changes @edevil! That's a good place where to put that indeed :) I've left some minor comments. Could you also run the linters? You need to install the dev dependencies (pip install -e ".[dev]"
) and run make style
. To be sure the changes are correct, you can run make quality
. Thanks!
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
I did run those commands and it passed, maybe my editor changed something again on save. The failures don't seem related to this change, though. I've addressed your comments. |
File lock operations did not have an associated timeout, which could present problems when trying to debug an issue since nothing would be logged if we could not acquire the lock and would wait forever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! PR looks good to me now :)
Failing tests are unrelated to your PR so I'm merging it now. Thanks @edevil! |
File lock operations did not have an associated timeout, which could present problems when trying to debug an issue since nothing would be logged if we could not acquire the lock and would wait forever.