Repository navigation
fix: Count automation jobs in a fixed number of Redis round trips - #2647
BelhsanHmida wants to merge 4 commits into
Conversation
JobCache.get pinged Redis, read the entry's job IDs with SMEMBERS, and then fetched each job with its own Job.fetch. A page that reads many entries, such as the jobs of every sensor in an asset's subtree, so paid a round trip per entry and another per job, which adds up quickly once Redis is not on the same machine. JobCache.get_many reads any number of entries in a fixed number of round trips: one ping, one pipeline for the job IDs of all entries, one Job.fetch_many for all jobs (fetching a job listed under several entries once), and one pipeline to remove the IDs of jobs that expired. JobCache.get is now a single-entry call to it, so its behaviour is unchanged. The two mock-based tests of get stubbed Job.fetch, so they are replaced by tests on fakeredis with real jobs: one checks that expired jobs are left out and removed from the cache, and one pins the number of direct commands and pipelines that get_many sends. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
The Automations page shows, per automation, how many of its jobs are queued, finished or failed, and refreshes this every minute. Counting them read each job cache entry separately, and then asked Redis for every job's status again with get_status(), although the job had just been fetched with its status. With 1000 jobs that is over 2000 round trips per refresh, per open tab. The counts now read all entries with JobCache.get_many and take each job's status from the fetched job with get_status(refresh=False). On a test site with 22 sensors and 1000 jobs, this brings the automations list endpoint from 1.1 s to 0.35 s with Redis on the same machine, and from 2.0 s to 0.34 s at 0.6 ms per round trip, with identical responses. A test counts the Redis commands and pipelines sent while counting the jobs of a schedule automation spread over several sensors, and checks the counts per status. On the previous code it sees 13 direct commands instead of 1. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Documentation build overview
18 files changed ·
|
Flix6x
left a comment
Flix6x
left a comment
There was a problem hiding this comment.
Read the diff and checked the three things that could have made this subtly wrong. All three hold, and I verified rather than assumed:
Job.fetch_manyis aligned with its input. On rq 2.10.0 it returns a list withNonein the place of each missing job (rq/job.py:700-730), so zipping it againstunique_job_idscannot shift jobs onto the wrong ids. Had it dropped the missing ones instead, every id after the first expired job would have mapped to the wrong job — worth stating explicitly, because the code reads the same either way.cache_refsis hashable._job_cache_refsreturnsset[tuple[int, str, str]], sodict.fromkeys(entries)is safe. A caller passing lists would raise, which is fine for a private-ish helper, andget()builds its own tuple.get_status(refresh=False)has something to read.fetch_manycallsrestore(), which sets_status, so the status comes from the hash that was just read. It also makes a page's counts one consistent snapshot rather than a status re-read per job, which I think is a small improvement in its own right.
I ran both new tests on the branch (26 passed) and then against main's job_cache.py and automations.py: both fail, so they bind. The round-trip assertions are the right shape for a performance fix — they pin the thing that regresses, not a timing.
One thing before merge: there is no changelog entry. The diff is four files, none of them documentation/changelog.rst, and the numbers in the description are squarely user-facing — an automations page going from 1.1 s to 0.35 s locally, and from 5.7 s to 0.3 s on a 2.6 ms link, is exactly what a reader of the changelog wants to know. Happy to push one if you would rather not.
Two notes, neither blocking:
- The round-trip counts encode that
fetch_manyuses exactly one pipeline, which is rq's implementation rather than its contract. If a future rq splits or chunks that, the test fails for a reason unrelated to FlexMeasures. Fine by me — a perf regression test has to pin something — but worth knowing when it next goes red. get_many([])still pings. Harmless, though_count_automation_jobswith no cache refs now costs a round trip where it previously cost none.
On the overlap you flagged: services/sensors.py:855 and :867 still call get() per entity, and since #2619 also touches JobCache.get, whichever lands second will want a look rather than a blind conflict resolution. Worth deciding the order deliberately.
🤖 Generated with Claude Code
…tomations page The page is new in this release, so what it costs to load belongs with the entry that introduces it, rather than as a line of its own about a slowdown nobody has had to live with. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl>
|
Changelog done in bb53d54, and not as an entry of its own: the automations page is new in this release, so what it costs to load belongs with the entry that introduces the page rather than reading as a fix for a slowdown nobody has had to live with. This PR is now named alongside #2294 and #2563 there. Heads-up on an overlap that will bite whoever merges second: #2591 edits that same line, to add Copy to the list of row actions. The conflict is one line and resolves by keeping both additions — the extra PR link and the Copy wording. 🤖 Generated with Claude Code |
#2619 renamed JobCache to JobMap and gave its `get` a `Job.fetch_many`, so the round trip per job this branch removed is already gone from main. What is left is the round trip per entry: `get` reads one index with its own `smembers`, so a page covering many sensors still pays one per sensor. `get_many` moves to `JobMap`, where it reads any number of indexes in one pipeline, fetches the jobs in another, and removes expired IDs in a third. `get` delegates to it, and the automation job stats ask for every index at once. The two unit tests that mocked a connection without `pipeline` are replaced by checks against a fake Redis, which exercise the real commands: one for expired IDs being dropped from their index, one pinning the round trips. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl>
This branch has not been deployed
2 participants
Description
The Automations page refreshes its job stats every minute. Counting one automation's jobs reads the Redis index of every sensor it covers — a schedule automation has one per sensor in its asset's subtree — and
JobMap.getreads one index per call, with its ownSMEMBERS. So a page covering 22 sensors pays 22 round trips before it fetches a single job.Changes
JobMap.get_manyreads any number of indexes in a fixed number of round trips: one ping, one pipeline for the job IDs, oneJob.fetch_manyfor the jobs, and one pipeline to remove IDs whose job has expired.JobMap.getdelegates to it, so a single-entry read behaves exactly as before.get_status(refresh=False), since the job was just fetched with its status rather than needing another read per job.What #2619 already did, and what is left
This branch was written against
JobCache, before #2619 renamed it toJobMapand gave itsgetaJob.fetch_many. That delivered the larger half of this work: the round trip per job is already gone from main. What remains, and what this PR now does, is the round trip per sensor, whichget_manycollapses into one pipeline.n + 2mround trips ton + 2, and now goes fromn + 2to 3.GET /assets/<id>/automationsGET /assets/<id>/automations/<id>Worth re-measuring against current main before anyone quotes a figure from this PR.
Tests
The two unit tests that mocked a Redis connection with
spec_set=["sadd", "smembers", "srem", "ping"]could not survive a method that usespipeline, and mocking the spec would only have asserted that the mock was called. They are replaced by two checks againstRQCompatibleFakeStrictRedis, which run the real commands:test_get_drops_expired_jobs— an expired job is left out and its ID removed from the index. This passes on main too, deliberately: it is the control that says the port did not lose behaviour.test_get_many_takes_a_fixed_number_of_round_trips— three entries, a job indexed under two of them, and an expired ID; one ping and three pipelines, with the shared job returned under both entries. Fails on main.test_automation_stats_take_a_fixed_number_of_redis_round_trips— the same, through the automations service, over a schedule automation covering three sensors. Fails on main.Not in this PR
Job.get_automations_feeding_sensorrebuilds the data generator or schedule trigger message of every automation on the sensor's asset and its ancestors, on every page load. That cost 60–90 ms per sensor page on a copy of the EMS database with 14–17 automations above the sensor, measured with plugins off, so the InsideOut reporters were skipped and the real cost is higher. Possible follow-ups:Changelog
No entry of its own: the automations page is new in this release, so what it costs to load belongs with the entry that introduces it, where this PR is now named alongside #2294, #2563 and #2591.
Closes #2646