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.

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

Simplify recorder RecorderRunsManager #131785

Merged
merged 2 commits into from
Dec 1, 2024
Merged

Simplify recorder RecorderRunsManager #131785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bfca1d
Simplify recorder RecorderRunsManager
emontnemery Nov 28, 2024
7c6c120
Merge branch 'dev' into recorder_simplify_recorder_runs
emontnemery Dec 1, 2024
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
73 changes: 9 additions & 64 deletions homeassistant/components/recorder/table_managers/recorder_runs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,43 +2,13 @@

from __future__ import annotations

import bisect
from dataclasses import dataclass
from datetime import datetime

from sqlalchemy.orm.session import Session

import homeassistant.util.dt as dt_util

from ..db_schema import RecorderRuns
from ..models import process_timestamp


def _find_recorder_run_for_start_time(
run_history: _RecorderRunsHistory, start: datetime
) -> RecorderRuns | None:
"""Find the recorder run for a start time in _RecorderRunsHistory."""
run_timestamps = run_history.run_timestamps
runs_by_timestamp = run_history.runs_by_timestamp

# bisect_left tells us were we would insert
# a value in the list of runs after the start timestamp.
#
# The run before that (idx-1) is when the run started
#
# If idx is 0, history never ran before the start timestamp
#
if idx := bisect.bisect_left(run_timestamps, start.timestamp()):
return runs_by_timestamp[run_timestamps[idx - 1]]
return None


@dataclass(frozen=True)
class _RecorderRunsHistory:
"""Bisectable history of RecorderRuns."""

run_timestamps: list[int]
runs_by_timestamp: dict[int, RecorderRuns]


class RecorderRunsManager:
Expand All @@ -48,7 +18,7 @@ def __init__(self) -> None:
"""Track recorder run history."""
self._recording_start = dt_util.utcnow()
self._current_run_info: RecorderRuns | None = None
self._run_history = _RecorderRunsHistory([], {})
self._first_run: RecorderRuns | None = None

@property
def recording_start(self) -> datetime:
Expand All @@ -58,9 +28,7 @@ def recording_start(self) -> datetime:
@property
def first(self) -> RecorderRuns:
"""Get the first run."""
if runs_by_timestamp := self._run_history.runs_by_timestamp:
return next(iter(runs_by_timestamp.values()))
return self.current
return self._first_run or self.current
Comment on lines 28 to +31
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first property is used for system health. Considering we're no longer relying on the recorder runs to avoid unnecessary history queries, but instead the timestamp of the oldest state, we should perhaps remove this property and include the timestamp of the oldest state in the system health information instead?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a good change to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll do that in a follow-up 👍


@property
def current(self) -> RecorderRuns:
Expand All @@ -78,15 +46,6 @@ def active(self) -> bool:
"""Return if a run is active."""
return self._current_run_info is not None

def get(self, start: datetime) -> RecorderRuns | None:
"""Return the recorder run that started before or at start.

If the first run started after the start, return None
"""
if start >= self.recording_start:
return self.current
return _find_recorder_run_for_start_time(self._run_history, start)

def start(self, session: Session) -> None:
"""Start a new run.

Expand Down Expand Up @@ -122,31 +81,17 @@ def load_from_db(self, session: Session) -> None:

Must run in the recorder thread.
"""
run_timestamps: list[int] = []
runs_by_timestamp: dict[int, RecorderRuns] = {}

for run in session.query(RecorderRuns).order_by(RecorderRuns.start.asc()).all():
if (
run := session.query(RecorderRuns)
.order_by(RecorderRuns.start.asc())
.first()
):
session.expunge(run)
if run_dt := process_timestamp(run.start):
# Not sure if this is correct or runs_by_timestamp annotation should be changed
timestamp = int(run_dt.timestamp())
run_timestamps.append(timestamp)
runs_by_timestamp[timestamp] = run

#
# self._run_history is accessed in get()
# which is allowed to be called from any thread
#
# We use a dataclass to ensure that when we update
# run_timestamps and runs_by_timestamp
# are never out of sync with each other.
#
self._run_history = _RecorderRunsHistory(run_timestamps, runs_by_timestamp)
self._first_run = run

def clear(self) -> None:
"""Clear the current run after ending it.

Must run in the recorder thread.
"""
if self._current_run_info:
self._current_run_info = None
self._current_run_info = None
32 changes: 6 additions & 26 deletions tests/components/recorder/table_managers/test_recorder_runs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ async def test_run_history(recorder_mock: Recorder, hass: HomeAssistant) -> None
two_days_ago = now - timedelta(days=2)
one_day_ago = now - timedelta(days=1)

# Test that the first run falls back to the current run
assert process_timestamp(
instance.recorder_runs_manager.first.start
) == process_timestamp(instance.recorder_runs_manager.current.start)

with instance.get_session() as session:
session.add(RecorderRuns(start=three_days_ago, created=three_days_ago))
session.add(RecorderRuns(start=two_days_ago, created=two_days_ago))
Expand All @@ -29,32 +34,7 @@ async def test_run_history(recorder_mock: Recorder, hass: HomeAssistant) -> None
instance.recorder_runs_manager.load_from_db(session)

assert (
process_timestamp(
instance.recorder_runs_manager.get(
three_days_ago + timedelta(microseconds=1)
).start
)
== three_days_ago
)
assert (
process_timestamp(
instance.recorder_runs_manager.get(
two_days_ago + timedelta(microseconds=1)
).start
)
== two_days_ago
)
assert (
process_timestamp(
instance.recorder_runs_manager.get(
one_day_ago + timedelta(microseconds=1)
).start
)
== one_day_ago
)
assert (
process_timestamp(instance.recorder_runs_manager.get(now).start)
== instance.recorder_runs_manager.recording_start
process_timestamp(instance.recorder_runs_manager.first.start) == three_days_ago
)


Expand Down