Remote patron activity sync celery task (PP-1164) #1894

jonathangreen · 2024-06-07T14:30:20Z

Description

This PR updates our Patron API that sync with remote providers to do the sync as an async celery task, rather then in the main web thread worker. To coordinate these patron activity sync tasks, I wired up redis as a service that we can use to store data, not just as a celery broker.

Redis is used to store the state of the patron activity sync and as a distributed lock, so that only one task is doing a sync at a time.

~~🚨 This needs a PR to update our deployments, since we have new environment variables. I'm going to leave it in draft until its ready to be merged.~~ PR up for this.

Motivation and Context

This should improve our speed and responsiveness when syncing the loans feed and will help with the issues we have been seeing in CA where a provider going down can cause the whole CM to become unresponsive because we are waiting for so long for the remote requests.

JIRA:

Both those tasks in JIRA ended up being worked on together in this one, since it wasn't easy to pull them apart.

How Has This Been Tested?

Tested locally
Unit tests

Checklist

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

codecov · 2024-06-07T15:43:54Z

Codecov Report

Attention: Patch coverage is 99.26650% with 3 lines in your changes missing coverage. Please review.

Project coverage is 90.28%. Comparing base (f372958) to head (aea8db2).

Files	Patch %	Lines
src/palace/manager/api/circulation.py	96.05%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1894      +/-   ##
==========================================
+ Coverage   90.16%   90.28%   +0.11%     
==========================================
  Files         325      332       +7     
  Lines       39701    39865     +164     
  Branches     8645     8647       +2     
==========================================
+ Hits        35798    35991     +193     
+ Misses       2597     2581      -16     
+ Partials     1306     1293      -13

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

src/palace/manager/api/circulation.py

src/palace/manager/api/controller/loan.py

src/palace/manager/api/opds_for_distributors.py

src/palace/manager/util/datetime_helpers.py

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

jonathangreen · 2024-06-11T18:55:16Z

I'm doing some local testing on this, and getting the PR ready for our hosting environment, but I think while that is happening this is ready for a code review.

jonathangreen · 2024-06-12T15:57:40Z

~~Once #1899 goes in, there will be a couple minor conflicts that need to be resolved with this one.~~ Upstream has been merged in.

tdilauro

This looks great! 🥇

A couple of comments, but nothing that would stop this going in as-is.

tdilauro · 2024-06-13T12:37:25Z

src/palace/manager/service/redis/redis.py

+# We do this right now because we are using types-redis, which defines Redis as a generic type,
+# even though it is not actually generic. Redis 5 now support type hints natively, so we we
+# should be able to drop types-redis in the future.
+# Currently, the build in type hints are incomplete at best, so types-redis does a better job.


Minor typo: "build" -> "built"?

tdilauro · 2024-06-13T12:39:47Z

src/palace/manager/service/redis/redis.py

+
+class RedisPrefixCheckMixin(ABC):
+    """
+    A mixin that with functions to check that the keys used in a redis command have the expected


Minor: "that with" -> "with"?

tdilauro · 2024-06-13T12:45:36Z

src/palace/manager/service/redis/redis.py

+    def _check_prefix(self, *args: Any) -> None:
+        arg_list = list(args)
+        command = arg_list.pop(0)
+        cmd_args = self._PREFIXED_COMMANDS.get(command)


Is it possible for command NOT to be all uppercase here?

It might be, if someone called execute_command directly. I'll call upper() just in case.

tdilauro · 2024-06-13T13:26:50Z

tests/manager/api/test_circulationapi.py

@@ -256,6 +252,7 @@ def test_attempt_renew_with_local_loan_and_no_available_copies(
        # Contrast with the way NoAvailableCopies is handled in
        # test_loan_becomes_hold_if_no_available_copies.
        circulation_api.remote.queue_checkout(NoAvailableCopies())
+        circulation_api.remote.queue_checkout(NoAvailableCopies())


Why do we need another checkout here? Might be good to add a comment about this.

Good catch, we don't actually need a second checkout here. Must have been an accidental copy / paste on my part.

tdilauro · 2024-06-13T13:42:14Z

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

+            status = patron_activity_status.status()
+            state = status.state.name if status is not None else "UNKNOWN"
+            task.log.info(
+                f"Patron activity sync not needed (state: {state}) for "


It's not clear to me what "not needed" means here. I seems like it could mean a variety of things (a sync is already in progress, a sync failed recently, etc.).

Perhaps "A new patron activity sync will not be queued..." would be more clear. Or better yet, something that makes the underlying reason more clear.

Is this more clear?

f"Patron activity sync task could not acquire lock. Task will not " f"perform sync. Lock state ({state}) for patron (id: {patron_id}) and " f"collection (id: {collection_id})."

Definitely more clear!

tdilauro · 2024-06-13T16:02:45Z

src/palace/manager/service/redis/redis.py

+        if cmd_args is not None:
+            for key in cmd_args.key_args(arg_list):
+                if not key.startswith(self._prefix):
+                    raise RedisValueError(
+                        f"Key {key} does not start with prefix {self._prefix}. Command {command} args: {arg_list}"
+                    )
+        else:
+            raise RedisValueError(
+                f"Command {command} is not checked for prefix. Args: {arg_list}"
+            )


Minor: Swapping the if and else blocks would let us guard and simplify here:

if cmd_args is None: raise RedisValueError( f"Command {command} is not checked for prefix. Args: {arg_list}" ) for key in cmd_args.key_args(arg_list): ...

jonathangreen commented Jun 11, 2024

View reviewed changes

jonathangreen requested a review from a team June 11, 2024 18:54

jonathangreen added cleanup migration PR that will need a cleanup migration once its been fully deployed feature New feature incompatible changes Changes that require a new major version labels Jun 11, 2024

jonathangreen force-pushed the feature/patron-activity-task branch from 0d61b94 to 3fd7c8b Compare June 12, 2024 00:56

jonathangreen mentioned this pull request Jun 12, 2024

Remove patron.last_loan_activity_sync #1897

Merged

2 tasks

jonathangreen marked this pull request as ready for review June 12, 2024 12:58

jonathangreen force-pushed the feature/patron-activity-task branch from 3fd7c8b to c17e068 Compare June 12, 2024 13:00

jonathangreen force-pushed the feature/patron-activity-task branch 2 times, most recently from 4317047 to aa3d646 Compare June 12, 2024 17:23

tdilauro approved these changes Jun 13, 2024

View reviewed changes

jonathangreen added 5 commits June 13, 2024 14:20

Add a redis service

73a08ac

Patron activity task

0e39144

Code review feedback

c0c437c

Add missed space

207b667

Update log text in tests as well.

87caa2f

jonathangreen force-pushed the feature/patron-activity-task branch from a83fc20 to 87caa2f Compare June 13, 2024 17:20

Roll back logging change. Break that out to seperate PR.

aea8db2

jonathangreen merged commit 1b2ce5f into main Jun 13, 2024
20 checks passed

jonathangreen deleted the feature/patron-activity-task branch June 13, 2024 18:35

This was referenced Jun 13, 2024

Celery logging config #1904

Merged

Overdrive API patron_activity returning None #1912

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remote patron activity sync celery task (PP-1164) #1894

Remote patron activity sync celery task (PP-1164) #1894

jonathangreen commented Jun 7, 2024 •

edited

Loading

codecov bot commented Jun 7, 2024 •

edited

Loading

jonathangreen commented Jun 11, 2024

jonathangreen commented Jun 12, 2024 •

edited

Loading

tdilauro left a comment

tdilauro Jun 13, 2024

tdilauro Jun 13, 2024

tdilauro Jun 13, 2024

jonathangreen Jun 13, 2024

tdilauro Jun 13, 2024

jonathangreen Jun 13, 2024

tdilauro Jun 13, 2024

jonathangreen Jun 13, 2024 •

edited

Loading

tdilauro Jun 13, 2024

tdilauro Jun 13, 2024

Remote patron activity sync celery task (PP-1164) #1894

Remote patron activity sync celery task (PP-1164) #1894

Conversation

jonathangreen commented Jun 7, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist

codecov bot commented Jun 7, 2024 • edited Loading

Codecov Report

jonathangreen commented Jun 11, 2024

jonathangreen commented Jun 12, 2024 • edited Loading

tdilauro left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathangreen Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathangreen commented Jun 7, 2024 •

edited

Loading

codecov bot commented Jun 7, 2024 •

edited

Loading

jonathangreen commented Jun 12, 2024 •

edited

Loading

jonathangreen Jun 13, 2024 •

edited

Loading