Closed a-ungurianu closed 1 year ago
Base: 94.38% // Head: 94.40% // Increases project coverage by +0.02%
:tada:
Coverage data is based on head (
e978bd7
) compared to base (a43ef2b
). Patch coverage: 100.00% of modified lines in pull request are covered.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
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...
LGTM Thanks a lot for this.
My pleasure. I'm trying to learn more about Zookeeper and the Kazoo recipes. Happy give back :)
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 https://github.com/python-zk/kazoo/pull/676#discussion_r1008309882:
@@ -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 https://github.com/python-zk/kazoo/pull/676#discussion_r1008309882, 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._lock
toself._thread_lock
(suggestions encouraged), as having a lock inside a lock made this hard to parseDoes this PR introduce any breaking change?
Nope