Skip to content

chore(typing): clean up some of the workrounds from the initial PR #801

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

Draft
ThosRTanner wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

chore(typing): clean up some of the workrounds from the initial PR #801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
173 changes: 56 additions & 117 deletions kazoo/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,6 @@ def __init__(
) -> None:
...

# FIXME This should be deprecated then killed
@overload
@deprecated(
"Passing retry configuration parameters directly to the client"
Expand Down Expand Up @@ -348,16 +347,8 @@ def __init__(
self.auth_data = set(auth_data if auth_data else [])
self.default_acl = default_acl
self.randomize_hosts = randomize_hosts
# FIXME Note: hosts and chroot are set by set_hosts, which also checks
# for chroot changes at runtime, so we initialize them to None here to
# avoid confusion with the empty string that set_hosts would set them
# to. This is massively hacky as set_hosts is only called from here
# anyway, but I want to make this change minimally invasive.
# we should really do self.hosts, self.chroot = self.set_hosts(hosts)
# and have set_hosts return the hosts and chroot
self.hosts: list[tuple[str, int]] = None # type: ignore[assignment]
self.chroot: str = None # type: ignore[assignment]
self.set_hosts(hosts)

self.hosts, self.chroot = self._collect_hosts(hosts)

self.use_ssl = use_ssl
self.verify_certs = verify_certs
Expand Down Expand Up @@ -397,74 +388,54 @@ def __init__(
self._stopped.set()
self._writer_stopped.set()

# FIXME This is kind of gross but we need to set these to something so
# that the type checker will understand that they are set by the time
# they are used and that they have the right type.
# We would do better to use a few variables/functions instead of
# overloading self.retry but this is a bit less invasive to the code
# and the type checker can understand it with a few hacks
self.retry: KazooRetry = None # type: ignore[assignment]
self._conn_retry: KazooRetry = None # type: ignore[assignment]

if type(connection_retry) is dict:
self._conn_retry = KazooRetry(**connection_retry)
elif type(connection_retry) is KazooRetry:
self._conn_retry = connection_retry

if type(command_retry) is dict:
self.retry = KazooRetry(**command_retry)
elif type(command_retry) is KazooRetry:
self.retry = command_retry

if type(self._conn_retry) is KazooRetry:
old_retry_keys = dict(_RETRY_COMPAT_DEFAULTS)
for key in old_retry_keys:
try:
old_retry_keys[key] = cast(
"dict[str, float | None]", kwargs
).pop(key)
warnings.warn(
"Passing retry configuration param %s to the "
"client directly is deprecated, please pass a "
"configured retry object (using param %s)"
% (key, _RETRY_COMPAT_MAPPING[key]),
DeprecationWarning,
stacklevel=2,
)
except KeyError:
pass

retry_keys: dict[str, Any] = {}
for oldname, value in old_retry_keys.items():
retry_keys[_RETRY_COMPAT_MAPPING[oldname]] = value
retry_keys["sleep_func"] = self.handler.sleep_func

def make_retry(
retry: KazooRetry | KazooRetryParams | None,
) -> KazooRetry:
if isinstance(retry, dict):
return KazooRetry(**retry)
if isinstance(retry, KazooRetry):
return retry
return KazooRetry(**retry_keys)

self._conn_retry = make_retry(connection_retry)
self._retry = make_retry(command_retry)

if self._conn_retry is not None:
if self.handler.sleep_func != self._conn_retry.sleep_func:
raise ConfigurationError(
"Retry handler and event handler "
" must use the same sleep func"
)

if type(self.retry) is KazooRetry:
if self.handler.sleep_func != self.retry.sleep_func:
if self._retry is not None:
if self.handler.sleep_func != self._retry.sleep_func:
raise ConfigurationError(
"Command retry handler and event handler "
"must use the same sleep func"
)

if self.retry is None or self._conn_retry is None:
# Note: because of the hacks at line 280, mypy thinks this is
# unreachable
old_retry_keys = dict( # type: ignore[unreachable]
_RETRY_COMPAT_DEFAULTS
)
for key in old_retry_keys:
try:
old_retry_keys[key] = kwargs.pop(key)
warnings.warn(
"Passing retry configuration param %s to the "
"client directly is deprecated, please pass a "
"configured retry object (using param %s)"
% (key, _RETRY_COMPAT_MAPPING[key]),
DeprecationWarning,
stacklevel=2,
)
except KeyError:
pass

retry_keys = {}
for oldname, value in old_retry_keys.items():
retry_keys[_RETRY_COMPAT_MAPPING[oldname]] = value

if self._conn_retry is None:
self._conn_retry = KazooRetry(
sleep_func=self.handler.sleep_func,
**retry_keys,
)
if self.retry is None:
self.retry = KazooRetry(
sleep_func=self.handler.sleep_func,
**retry_keys,
)

# Managing legacy SASL options
for scheme, auth in self.auth_data:
if scheme != "sasl":
Expand Down Expand Up @@ -508,7 +479,6 @@ def __init__(

# Every retry call should have its own copy of the retry helper
# to avoid shared retry counts
self._retry = self.retry

def _retry(
func: Callable[GenericArgs, KazooRetry.RETRY_RETURN],
Expand All @@ -517,14 +487,7 @@ def _retry(
) -> KazooRetry.RETRY_RETURN:
return self._retry.copy()(func, *args, **kwargs)

# FIXME
# (expression has type "Callable[[VarArg(Any), KwArg(Any)], Any]",
# variable has type "KazooRetry") so basically self.retry needs to be
# set to that and then the type checker will understand that
# self.retry.copy() is a valid call. This is just a mess and needs the
# code rearranging to be more mypy friendly but this is the least
# invasive way to do it for now
self.retry = _retry # type: ignore[assignment]
self.retry = _retry

self.Barrier = partial(Barrier, self)
self.Counter = partial(Counter, self)
Expand Down Expand Up @@ -609,6 +572,12 @@ def connected(self) -> bool:
established."""
return self._live.is_set()

def _collect_hosts(
self, hosts: str | list[str]
) -> tuple[list[tuple[str, int]], str]:
new_hosts, chroot = collect_hosts(hosts)
return new_hosts, normpath(chroot)

def set_hosts(
self,
hosts: str | list[str],
Expand Down Expand Up @@ -637,25 +606,18 @@ def set_hosts(
zookeeper server cluster has undefined behavior.

"""

# Change the client setting for randomization if specified
# Randomizing the list will be done at connect time
if randomize_hosts is not None:
self.randomize_hosts = randomize_hosts

# Randomizing the list will be done at connect time
self.hosts, chroot = collect_hosts(hosts)

if chroot:
new_chroot = normpath(chroot)
else:
new_chroot = ""
self.hosts, chroot = self._collect_hosts(hosts)

if self.chroot is not None and new_chroot != self.chroot:
if chroot != self.chroot:
raise ConfigurationError(
"Changing chroot at runtime is not " "currently supported"
"Changing chroot at runtime is not currently supported"
)

self.chroot = new_chroot
self.chroot = chroot

def add_listener(self, listener: ListenerFunc) -> None:
"""Add a function to be called for connection state changes.
Expand Down Expand Up @@ -993,31 +955,12 @@ def _try_fetch() -> tuple[int, ...] | None:
except ValueError:
return None

def _is_valid(version: tuple[int, ...] | None) -> bool:
# All zookeeper versions should have at least major.minor
# version numbers; if we get one that doesn't it is likely not
# correct and was truncated...
if version and len(version) > 1:
return True
return False

# FIXME A better way of doing this would be to put the initial
# _try_fetch in the loop and inline _is_valid but I want to minimise
# code changes

# Try 1 + retries amount of times to get a version that we know
# will likely be acceptable...
version = _try_fetch()
if _is_valid(version):
# mypy doesn't recognise that _is_valid guarantees this
# and the next 2 suppress should include return-value
# but hound is broken
return version # type: ignore
for _i in range(0, retries):
for _i in range(0, retries + 1):
version = _try_fetch()
if _is_valid(version):
# mypy doesn't recognise that _is_valid guarantees this
return version # type: ignore
if version is not None and len(version) > 1:
return version
raise KazooException(
"Unable to fetch useable server"
" version after trying %s times" % (1 + max(0, retries))
Expand Down Expand Up @@ -1596,12 +1539,8 @@ def get_children_async(
raise TypeError("Invalid type for 'include_data' (bool expected)")

async_result = self.handler.async_result()
# FIXME? Do this as req = getc2 if include_data else getc
req: GetChildren | GetChildren2
if include_data:
req = GetChildren2(_prefix_root(self.chroot, path), watch)
else:
req = GetChildren(_prefix_root(self.chroot, path), watch)
func = GetChildren2 if include_data else GetChildren
req = func(_prefix_root(self.chroot, path), watch)
self._call(req, async_result)
return async_result

Expand Down
29 changes: 1 addition & 28 deletions kazoo/handlers/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,33 +384,6 @@ def captured_function(
return capture


def fileobj_to_fd(fileobj: FdLike) -> int:
"""Return a file descriptor from a file object.

Parameters:
fileobj -- file object or file descriptor

Returns:
corresponding file descriptor

Raises:
TypeError if the object is invalid
"""
if isinstance(fileobj, int):
fd = fileobj
else:
# FIXME given the protocol I don't think the try/catch/int are
# required.
try:
fd = int(fileobj.fileno())
except (AttributeError, TypeError, ValueError):
raise TypeError("Invalid file object: " "{!r}".format(fileobj))
# FIXME Questionable, just let select deal with it.
if fd < 0:
raise TypeError("Invalid file descriptor: {}".format(fd))
return fd


def selector_select(
rlist: Iterable[FdLike],
wlist: Iterable[FdLike],
Expand All @@ -436,7 +409,7 @@ def selector_select(

for event, fileobjs in events_mapping.items():
for fileobj in fileobjs:
fd = fileobj_to_fd(fileobj)
fd = fileobj if isinstance(fileobj, int) else fileobj.fileno()
fd_events[fd] |= event
fd_fileobjs[fd].append(fileobj)

Expand Down
9 changes: 5 additions & 4 deletions kazoo/hosts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

def collect_hosts(
hosts: str | list[str],
) -> tuple[list[tuple[str, int]], str | None]:
) -> tuple[list[tuple[str, int]], str]:
"""
Collect a set of hosts and an optional chroot from
a string or a list of strings.
Expand All @@ -14,11 +14,12 @@ def collect_hosts(
if hosts[-1].strip().startswith("/"):
host_ports, chroot = hosts[:-1], hosts[-1]
else:
host_ports, chroot = hosts, None
host_ports, chroot = hosts, ""
else:
host_ports_1, chroot = hosts.partition("/")[::2]
host_ports = host_ports_1.split(",")
chroot = "/" + chroot if chroot else None
if chroot != "":
chroot = "/" + chroot

result = []
for host_port in host_ports:
Expand All @@ -28,7 +29,7 @@ def collect_hosts(
host = res.hostname
if host is None:
raise ValueError("bad hostname")
port = int(res.port) if res.port else 2181
port = 2181 if res.port is None else res.port
result.append((host.strip(), port))

return result, chroot
1 change: 0 additions & 1 deletion kazoo/interfaces.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

from __future__ import annotations


from typing import (
Any,
Callable,
Expand Down
Loading
Loading