Skip to content
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

mypy: Add type hints to pglookout [BF-1560] #106

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
0d4ea2c
mypy: `pglookout/logutil.py` [BF-1560]
Mar 16, 2023
238eb76
mypy: `pglookout/pgutil.py` [BF-1560]
Mar 16, 2023
3db5684
mypy: `pglookout/current-master.py` [BF-1560]
Mar 17, 2023
2ce13ab
mypy: `pglookout/statsd.py` [BF-1560]
Mar 17, 2023
25617d1
mypy: `pglookout/common.py` [BF-1560]
Mar 16, 2023
016d14d
mypy: `pglookout/config.py` [BF-1560]
Mar 17, 2023
bf067b3
mypy: `pglookout/common_types.py` [BF-1560]
Mar 23, 2023
a462bf9
mypy: `pglookout/webserver.py` [BF-1560]
Mar 17, 2023
9b4e13f
mypy: `pglookout/cluster_monitor.py` [BF-1560]
Mar 17, 2023
45d413b
mypy: `pglookout/pglookout.py` [BF-1560]
Mar 23, 2023
6ce993e
mypy: Refactor `check_cluster_state` [BF-1560]
Mar 24, 2023
0d438e9
mypy: `test/test_pglookout.py` [BF-1560]
Mar 27, 2023
1f47155
mypy: New method to normalize config [BF-1560]
Mar 27, 2023
5d4ac6e
mypy: `version.py` [BF-1560]
Mar 27, 2023
51bb85e
mypy: `setup.py` [BF-1560]
Mar 27, 2023
e9834d4
mypy: Remove python2 dependency [BF-1560]
Mar 27, 2023
a3ad6f8
mypy: `conftest.py` [BF-1560]
Mar 27, 2023
7434873
Bump `Makefile` version to `2.1.0` [BF-1560]
Mar 27, 2023
30bcd0d
mypy: Compatible with Python 3.9 logging [BF-1560]
Mar 27, 2023
e4e771a
pylint: Enable more messages [BF-1560]
Mar 27, 2023
56c01a8
pylint: Fix broad exception raised [BF-1560]
Mar 28, 2023
09a15b1
pylint: Dev requirements are now pinned [BF-1560]
Mar 28, 2023
9a0abae
make: Dev reqs are now in isolation [BF-1560]
Apr 4, 2023
dac0aad
Merge remote-tracking branch 'origin/main' into sgiffard/BF-1560_add_…
Apr 5, 2023
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
53 changes: 53 additions & 0 deletions pglookout/common_types.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
# Copyright (c) 2023 Aiven, Helsinki, Finland. https://aiven.io/
from __future__ import annotations

from datetime import datetime
from typing import Any, Dict, TypedDict


class ReplicationSlotAsDict(TypedDict, total=True):
slot_name: str
plugin: str
slot_type: str
database: str
catalog_xmin: str
restart_lsn: str
confirmed_flush_lsn: str
state_data: str


class MemberState(TypedDict, total=False):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the naming is not clear enough, I had to jump to a different commit and file just to read the docstring in order to know what was going on in 9b4e13f

Suggested change
class MemberState(TypedDict, total=False):
class ClusterMemberState(TypeDict, total=False):

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm... even with the docstring? I personally think that your name suggestion is also good and I think it makes sense. I'm just wondering.

"""Represents the state of a member of the cluster.

Note:
This is a very loose type as no key is mandatory. This is because
it is too dangerous to impose a stricter type until we have a
better test coverage, as it would change some behaviour in the
code (some unconventional behaviour was detected, and it may be a
bug or a feature).
"""

# Connection Status
connection: bool
fetch_time: str
# Queried Status
db_time: str | datetime
pg_is_in_recovery: bool
pg_last_xact_replay_timestamp: datetime | str | None
pg_last_xlog_receive_location: str | None
pg_last_xlog_replay_location: str | None
# Replication info
replication_slots: list[ReplicationSlotAsDict]
replication_time_lag: float | None
min_replication_time_lag: float
replication_start_time: float | None


# Note for future improvements:
# If we want ObserverState to accept arbitrary keys, we have three choices:
# - Use a different type (pydantic, dataclasses, etc.)
# - Use a TypedDict for static keys (connection, fetch_time) and a sub-dict for
# dynamic keys (received from state.json).
# - Wait for something like `allow_extra` to be implemented into TypedDict (unlikely)
# https://github.com/python/mypy/issues/4617
ObserverState = Dict[str, Any]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually confusing... ClusterMonitor.observer_state is actually dict[str, ObserverState], while ClusterMonitor._fetch_observer_state return type is Optional[ObserverState] ... so its quite misleading in my opinion. I rather see a more specific typing and later work on improvements

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I pulled a lot of hair on that one. The names "observer state" were loosely used for different things. Both for an "observed state" and for an "observer state".

For now, I'd say to ignore this, as it gets improved in a later commit :-). Ideally, I could also change the name instance variable ClusterMonitor.observer_state and such, but I didn't (I think), as I didn't want to change any of the public APIs (symbols) in this PR.