Convert database_reaper to celery (PP-1470) #2324

jonathangreen · 2025-03-06T21:24:19Z

Description

Convert all of the ReaperMonitor scripts to be Celery Tasks:

CredentialReaper
PatronRecordReaper
WorkReaper
CollectionReaper
MeasurementReaper
LoanReaper
HoldReaper
IdlingAnnotationReaper

Updates some of our database foreign key relationships so its easier to use a database level delete cascade to clean up old records in the database, rather then relying on the ORM to do the cascade.

~~This PR is based on these PR (they should get merged first):~~

Motivation and Context

Working through the epic to convert all of our scripts into tasks.

JIRA: PP-1470

How Has This Been Tested?

Running tests in CI

Checklist

I have updated the documentation accordingly.
All new and existing tests passed.

codecov · 2025-03-06T21:35:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.28%. Comparing base (bb90e20) to head (602a726).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2324      +/-   ##
==========================================
- Coverage   91.29%   91.28%   -0.02%     
==========================================
  Files         364      364              
  Lines       41402    41317      -85     
  Branches     8862     8870       +8     
==========================================
- Hits        37800    37715      -85     
  Misses       2340     2340              
  Partials     1262     1262

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jonathangreen · 2025-03-07T01:22:18Z

src/palace/manager/celery/tasks/reaper.py

+        select(Hold)
+        .where(Hold.start < cutoff, or_(Hold.end == None, Hold.end < utc_now()))
+        .order_by(Hold.id)
+        .limit(batch_size)


This query has been simplified in two ways:

We aren't filtering out unlimited access and open access license pools, because you can't place holds on these items anyway.

We drop the concept of "source of truth" integrations.

The only source of truth integration we had was OPDS for distributors, which only supports unlimited access items, so you couldn't place a hold anyway.

So this query should be effectively equivalent, even though its dropped some of the conditions.

jonathangreen · 2025-03-07T01:25:23Z

src/palace/manager/celery/tasks/reaper.py

+        or_(
+            Loan.end < now,
+            and_(Loan.start < now - timedelta(days=90), Loan.end == None),
+        ),


This query drops the concept of "source of truth" integrations.

Previously the only source of truth integration we had was OPDS for distributors, which only supports unlimited access items, so these items are filtered out by the LicensePool.unlimited_access == False check, so no need to complicate the query by filtering on the collection protocol.

So this query should be effectively equivalent to what we had before.

jonathangreen · 2025-03-07T01:29:11Z

src/palace/manager/celery/tasks/reaper.py

+    deletion_query = delete(Credential).where(Credential.expires < cutoff)
+    with task.transaction() as session:
+        rows_removed = _execute_delete(session, deletion_query)
+    task.log.info(f"Deleted {pluralize(rows_removed, 'expired credential')}.")


For as many of the reapers as I could, i updated them to use a direct SQL delete statement, rather then loading each record into the ORM, to just delete it. Should allow us to perform the deletes much faster.

This required setting ondelete="CASCADE" on the patron_id foreign keys, so that we are able to use delete statements to clean up patron records in the reaper, without needing the ORM to clean up the related records.

That is why this needs a DB migration.

jonathangreen · 2025-03-07T01:34:50Z

src/palace/manager/core/monitor.py

-ReaperMonitor.REGISTRY.append(MeasurementReaper)
-
-
-class ScrubberMonitor(ReaperMonitor):


I didn't bother migrating either of the srubber monitors, since we are not using the patron location feature. Its pretty NYPL specific. I'm going to instead just drop whats left of that code in a follow up PR to this one.

Here is the follow up PR: #2326 (still in progress)

jonathangreen · 2025-03-07T01:35:51Z

src/palace/manager/service/celery/celery.py

+                minute="40",
+                hour="2",
+            ),  # Once a day at 2:40 AM
+        },


These all used to run at 2am every day. Since they are all separate task now, I decided to space them out a little bit.

jonathangreen · 2025-03-07T01:38:11Z

src/palace/manager/sqlalchemy/model/patron.py

+        back_populates="patron",
+        cascade="delete",
+        uselist=True,
+        passive_deletes=True,


See SQLAlchemy docs: https://docs.sqlalchemy.org/en/20/orm/cascades.html#passive-deletes

I updated these relationships to set the passive_deletes parameter since I added a DB level foreign key cascade.

jonathangreen · 2025-03-07T01:39:20Z

tests/manager/api/test_monitor.py

-from tests.fixtures.services import ServicesFixture
-
-
-class TestLoanlikeReaperMonitor:


I moved the bulk of these tests into test_reaper.py

jonathangreen · 2025-03-07T01:40:34Z

tests/manager/core/test_monitor.py

@@ -1031,298 +1018,3 @@ def test_set_item(self, db: DatabaseTransactionFixture):
        monitor = CustomListEntryWorkUpdateMonitor(db.session)
        monitor.process_item(entry)
        assert old_work == entry.work
-
-
-class MockReaperMonitor(ReaperMonitor):


I moved the bulk of these tests into test_reaper.py

tdilauro

LGTM! 🎸🤘🏽

tdilauro · 2025-03-07T15:10:27Z

tests/manager/celery/tasks/test_reaper.py

I'm assuming that most of these just moved over, so I did't really give this much review. If there are specific things you want me to look at, just let me know.

jonathangreen commented Mar 7, 2025

View reviewed changes

jonathangreen requested a review from a team March 7, 2025 01:51

jonathangreen added feature New feature DB migration This PR contains a DB migration labels Mar 7, 2025

tdilauro approved these changes Mar 7, 2025

View reviewed changes

jonathangreen force-pushed the chore/refactor-analytics branch from 8c582d1 to 20a6c64 Compare March 7, 2025 17:33

jonathangreen force-pushed the feature/reapers-celery branch from defe008 to c7ba85b Compare March 7, 2025 17:34

Base automatically changed from chore/refactor-analytics to main March 10, 2025 19:38

jonathangreen force-pushed the feature/reapers-celery branch from c7ba85b to 628de50 Compare March 10, 2025 19:40

jonathangreen added 11 commits March 10, 2025 16:48

Convert reapers to celery

d67c1ba

Reapers added and tested

5299364

Add our reaper configuration

0b40892

Remove old reaper code

149d892

Fix mypy issue

87aa456

Add database migration

5bedb92

Update test

24851d5

Add one more test

cba2517

Move function, add test, add doc string

a288bb0

Update alembic migration

94a9aa3

Clean up formatting a bit

602a726

jonathangreen force-pushed the feature/reapers-celery branch from 628de50 to 602a726 Compare March 10, 2025 19:49

jonathangreen merged commit a389d5a into main Mar 10, 2025
19 checks passed

jonathangreen deleted the feature/reapers-celery branch March 10, 2025 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert database_reaper to celery (PP-1470) #2324

Convert database_reaper to celery (PP-1470) #2324

jonathangreen commented Mar 6, 2025 •

edited

Loading

codecov bot commented Mar 6, 2025 •

edited

Loading

jonathangreen Mar 7, 2025

jonathangreen Mar 7, 2025

jonathangreen Mar 7, 2025 •

edited

Loading

jonathangreen Mar 7, 2025

jonathangreen Mar 7, 2025

jonathangreen Mar 7, 2025

jonathangreen Mar 7, 2025

jonathangreen Mar 7, 2025

jonathangreen Mar 7, 2025

tdilauro left a comment

tdilauro Mar 7, 2025

		ReaperMonitor.REGISTRY.append(MeasurementReaper)


		class ScrubberMonitor(ReaperMonitor):

		from tests.fixtures.services import ServicesFixture


		class TestLoanlikeReaperMonitor:

Convert database_reaper to celery (PP-1470) #2324

Convert database_reaper to celery (PP-1470) #2324

Conversation

jonathangreen commented Mar 6, 2025 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist

codecov bot commented Mar 6, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathangreen Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdilauro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathangreen commented Mar 6, 2025 •

edited

Loading

codecov bot commented Mar 6, 2025 •

edited

Loading

jonathangreen Mar 7, 2025 •

edited

Loading