-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
reduce timeout when acquring lock #4481
Conversation
Previously, it'd sleep for 5s. Now, it should retry 6 times before timing out. Also reduced the timeout to 3s.
retries = 6 | ||
delay = DEFAULT_TIMEOUT / retries | ||
lock_retry = retry(retries, LockError, timeout=delay)(self._do_lock) |
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.
delay
is clearly incorrect terminology π Let's just
retries = 6 | |
delay = DEFAULT_TIMEOUT / retries | |
lock_retry = retry(retries, LockError, timeout=delay)(self._do_lock) | |
lock_retry = retry(RETRIES, LockError, timeout=TIMEOUT)(self._do_lock) |
with, e.g.
TIMEOUT = 0.1
RETRIES = 50
and then just patch for tests as needed.
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.
Why do you think so?
From funcy's documentation:
There will be a delay in
timeout
seconds between tries
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.
@skshetry Precisely because it is called timeout
and not delay
. Dividing stuff is not obvious, better to keep explicit TIMEOUT and RETRIES (and not hardcode it).
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.
Looks like timeout has two different meanings? I was only familiar with one.
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.
@skshetry They are pretty much the same, but you are thinking about the total lock timeout, while I'm talking about this particular piece of code, where timeout is about the time between tries. In the future there might be a total-timeout CLI option, but right now I'm just concerned about this piece of code.
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.
@efiop, HardlinkLock
uses DEFAULT_TIMEOUT
as well. I don't want to have two different timeouts, so I'd have to do something like following:
retries = 6 | |
delay = DEFAULT_TIMEOUT / retries | |
lock_retry = retry(retries, LockError, timeout=delay)(self._do_lock) | |
delay = DEFAULT_TIMEOUT / RETRIES | |
lock_retry = retry(RETRIES, LockError, timeout=delay)(self._do_lock) |
which is no different from this PR. Also, I want to mock as less as possible, and just mocking total timeout is enough for the tests.
Previously, it'd sleep for 5s. Now, it should retry 6 times
before timing out.
Also reduced the timeout to 3s.
β I have followed the Contributing to DVC checklist.
π If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Thank you for the contribution - we'll try to review it as soon as possible. π