refactor: Use native Lock timeout instead of reimplementing #676
Conversation
a-ungurianu
force-pushed
the
refactor/thread_timeout
branch
from
3bfe021
to
5524a37
Compare
a-ungurianu
force-pushed
the
refactor/thread_timeout
branch
3 times, most recently
from
6d9422f
to
e4e2362
Compare
Codecov ReportBase: 94.38% // Head: 94.40% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #676 +/- ##
==========================================
+ Coverage 94.38% 94.40% +0.02%
==========================================
Files 57 57
Lines 8399 8389 -10
==========================================
- Hits 7927 7920 -7
+ Misses 472 469 -3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| locked = self._lock.acquire(False) | ||
| if not locked and not blocking: | ||
| thread_locked = self._thread_lock.acquire( | ||
| blocking=blocking, timeout=timeout if timeout is not None else -1 |
There was a problem hiding this comment.
I'm not a fan that python's threading chose to require timeout to be a number. Thankfully, it seems the other handler implementation's underlying Lock replacement also follows the python API, so this should be safe.
I could just replace the default parameter to -1 instead. Don't have a strong opinion either way
There was a problem hiding this comment.
IMO the way you implemented it is better: it does not change the method signature, so no chance to break someone's current code.
|
The test that's failing seems to suggest it's already flaky to begin with. Not sure how my change could affect that specific test. It seems that the client is failing to connect to all zookeeper instances, not just the ones we stopped. |
|
Hey, thank you for this. Yes the tests around the "read only" feature is kind of flaky, mostly because one testcase is stopping the cluster. I have a hard time reproducing it locally so yeah... |
| locked = self._lock.acquire(False) | ||
| if not locked and not blocking: | ||
| thread_locked = self._thread_lock.acquire( | ||
| blocking=blocking, timeout=timeout if timeout is not None else -1 |
There was a problem hiding this comment.
IMO the way you implemented it is better: it does not change the method signature, so no chance to break someone's current code.
kazoo/recipe/lock.py
Outdated
| @@ -134,7 +134,7 @@ def __init__(self, client, path, identifier=None, extra_lock_patterns=()): | |||
| self._retry = KazooRetry( | |||
| max_tries=None, sleep_func=client.handler.sleep_func | |||
| ) | |||
| self._lock = client.handler.lock_object() | |||
| self._thread_lock = client.handler.lock_object() | |||
There was a problem hiding this comment.
You asked for a suggestion: how about _handler_lock?
Just to recognize that other handlers are not thread based
There was a problem hiding this comment.
Actually, how about _acquire_method_lock which describes what it does instead of what it is.
|
LGTM |
|
My pleasure. I'm trying to learn more about Zookeeper and the Kazoo recipes. Happy give back :) |
a-ungurianu
force-pushed
the
refactor/thread_timeout
branch
from
e4e2362
to
e978bd7
Compare
|
Sure. Sounds good to me 👍
…On Fri, Oct 28, 2022, 13:48 Alex Ungurianu ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In kazoo/recipe/lock.py
<#676 (comment)>:
> @@ -134,7 +134,7 @@ def __init__(self, client, path, identifier=None, extra_lock_patterns=()):
self._retry = KazooRetry(
max_tries=None, sleep_func=client.handler.sleep_func
)
- self._lock = client.handler.lock_object()
+ self._thread_lock = client.handler.lock_object()
Actually, how about "_acquire_method_lock" which describes what it does
instead of what it is.
—
Reply to this email directly, view it on GitHub
<#676 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIFTHWLOCZH73SPQBTQY7DWFQGWNANCNFSM6AAAAAARMOOKSY>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Fixes #605
Why is this needed?
Not needed per se, but removes unnecessary complexity from the lock acquiring code
Proposed Changes
self._locktoself._thread_lock(suggestions encouraged), as having a lock inside a lock made this hard to parseDoes this PR introduce any breaking change?
Nope