Refactoring layers and cache #3175

Macroz · 2023-08-08T16:57:13Z

Goals:

Refactor all code to follow already agreed layered architecture (refactoring has been a slow ongoing process)
Fully populate service layer and do not bypass it in business code
Rename functions for clarity (e.g. get-full-internal-application instead of always just get-application)
Introduce rems.service.cache to cache "joined things". E.g. fully populated application with users, workflow, form etc. joined to the map.
Introduce rems.cache for basic caching things and use it throughout.
Cache metadata (catalogue items, workflow, resources, ...) completely (likely to be small)
Cache most recent applications only
Cache common complex queries as projections:
- Which roles a user has? (also from applications)
- Which applications a user is participating in?

NB:

Removes catalogue item localization cache (should not be needed)
Tests may use service/DB layer as they wish
Removes the event namespace (as it is really a part of the application)
- User service should handle attributes, settings etc.
- Application service should handle also events

Open issues:

Many TODO
Commented code
Management of application event projections (including new ones such as user roles and user applications)
Performance testing
Introduce back just enough caching (and cache invalidation)
ADR
Follow-up tasks
Should mounted state be in rems.system.xxx?
...

Related issues:

Checklist for author

Remove items that aren't applicable, check items that are done.

Reviewability

Link to issue

Backwards compatibility

API is backwards compatible or completely new
Events are backwards compatible or have migrations
Config is backwards compatible
Feature appears correctly in PDF print

Documentation

Update changelog if necessary
Update docs/ (if applicable)
ADR for major architectural decisions or experiments (write a caching startegy ADR)
New config options in config-defaults.edn

Testing

Complex logic is unit tested
Valuable features are integration / browser / acceptance tested automatically

Follow-up

New tasks are created for pending or remaining tasks

There could be less rows to project after filtering.

Macroz · 2023-11-02T13:29:56Z

src/clj/rems/api/applications.clj

+               (mapv project-application-overview)
               (filter-with-search query))))))


We can change the order of these operations like in the next function (slightly improve perf)

Macroz · 2023-11-09T12:10:31Z

src/clj/rems/application/process_managers.clj

-        (applications/delete-application! (:application/id event))
-        (applications/delete-application-and-reload-cache! (:application/id event))))))
+      (application/delete-application! (:application/id event))
+      #_(if (= "expirer-bot" (:event/actor event)) ; will reload in the end


This can be removed. Cache invalidation (and reload) happens automatically.

Macroz · 2023-11-09T12:25:55Z

src/clj/rems/application/rejecter_bot.clj

-  (->> (mapcat applications/get-my-applications user-ids)
+  (->> (mapcat application/get-full-personalized-applications-by-user user-ids)
       (mapv :application/id)
       distinct
-       (mapv applications/get-application)
+       (mapv application/get-full-internal-application)


We don't have to get the full internal twice (the previous get-my-applications returned overview?).

(->> (mapcat application/get-full-personalized-applications-by-user user-ids) (distinct-by :application/id))

Does it need to be a full internal application?

Consider implementing get-full-internal-applications-by-user

Macroz · 2023-11-09T12:45:48Z

src/clj/rems/application/search.clj

+            #_(log/info "Start indexing" (count app-ids) "applications...")
+            (doseq [app-id app-ids
+                    :let [application (get-full-internal-application app-id)]
+                    :when application]
+              (index-application! writer application))
+            #_(log/info "Finished indexing" (count app-ids) "applications")))


Remove commented code

Macroz · 2023-11-09T13:08:38Z

src/clj/rems/application/search.clj

+  (locking index-lock
+    (when-some [requests (seq @indexing-requests)]
+      (let [events (mapcat first requests)
+            get-full-internal-application (second (first requests))] ; NB: doesn't matter which get-full-internal-application is actually used
+        (index-events! events
+                       get-full-internal-application))  ; TODO avoid by moving to system ns
+      (reset! indexing-requests nil))))


Locking not necessary with reset-vals!

get-full-internal-application could be static reference if we move the system ns

Macroz · 2023-11-09T13:09:06Z

src/clj/rems/application/search.clj

+  (locking index-lock
+    (swap! indexing-requests conj [events get-full-internal-application])
+    nil)) ; process manager expects sequence of new events)


Locking not necessary (see above)

sequence of new commands?

Macroz · 2023-11-09T13:12:47Z

src/clj/rems/cache.clj

@@ -0,0 +1,76 @@
+(ns rems.cache


TODO: Implement (use from service.cache)

Macroz · 2023-11-09T13:16:07Z

src/clj/rems/cache.clj

+;;     - Schema in use
+;;     - response codes
+;;     - user info from headers
+;;     - access rights


Who is what role? Coarse-grained access control, i.e. who can call what API?

Macroz · 2023-11-09T13:16:26Z

src/clj/rems/cache.clj

+;;   - transforms between db and internal representation
+;;     - plain Clojure data
+;;     - Schema supported
+;; - rems.service


Fine-grained role-based access control

Macroz · 2023-11-09T13:16:53Z

src/clj/rems/cache.clj

+;;     - access rights
+;; - rems.cache (NEW)
+;;   - collection of caching functionality
+;;   - centralized access to cache status, invalidation and statistics


Macroz · 2023-11-09T13:20:21Z

src/clj/rems/cache.clj

+;; rems.api -> rems.custom -> rems.cache -> rems.service -> rems.db -> pg
+;; - cache complete joined representation
+;; - items are customized later in code so everything cached can be shared
+;;


OPTION 4 - rems.api -> rems.service -> rems.service.cache (rems.cache) -> rems.db (rems.cache) -> pg - rems.service.cache joins stuff together and caches full representation - rems.db can use low level rems.cache to cache db data - rems.watcher to notify when something has changed, listeners will invalidate and/or recompute - rems.service.cache may cache customized / personalized content and collections if necessary - additional projections for user-roles, user-applications etc. performance critical data

Macroz · 2023-11-09T13:31:34Z

src/clj/rems/db/applications.clj

+;;; Cache
+
+(defn reload-cache! []
+  ;; TODO implement


Macroz · 2023-11-10T09:48:37Z

src/clj/rems/db/applications.clj

+       :application.event/voted)
+      user-applications ; no user added
+
+      :application.event/copied-from


Should this actually be copied-to if we want to add the new application id to the members of the (old) application?

{:event/type :application.event/copied-from :application/id new-app-id :application/copied-from (select-keys application [:application/id :application/external-id])}

So the app-id is the new application id. The members come from application which is from the command, i.e. the old application before this event (and with members).

Macroz · 2023-11-10T09:49:18Z

src/clj/rems/db/applications.clj

+      (reduce (fn [user-applications userid]
+                (update user-applications userid app-id))
+              user-applications
+              (:application/members application))


; NB: applicant gets the copied application already from `created` event.

Macroz · 2023-11-10T10:01:54Z

src/clj/rems/db/applications.clj

+      :application.event/reviewer-joined
+      (update user-applications actor conj-set app-id))))
+
+(defn- user-roles-projection [user-roles event application]


Could this name be user-application-roles-projection or so because these are only the roles that come from applications. Not workflow handlers, roles table etc.

Consider also just listening to application and updating based on its :application/user-roles data that is enriched.

Macroz · 2023-11-10T10:07:15Z

src/clj/rems/db/applications.clj

+      :application.event/applicant-changed
+      (-> user-roles
+          (update (:application/applicant application) disj-set :applicant)
+          (update (:application/applicant event) (conj-set :applicant)))


Should make the old applicant a member?

Macroz · 2023-11-10T10:11:21Z

src/clj/rems/service/cache.clj

+(defn get-users-with-role [role]
+  (set/union
+   (users/get-users-with-role role)
+   (applications/get-users-with-role role)))


This should consider workflow handlers. Currently applications/get-users-with-role does not do it but could.

There isn't a test that requires it at the moment. This is only user to enrich super users into applications which could use the "roles" table directly. We can remove this function.

Macroz · 2023-11-10T10:15:56Z

src/clj/rems/middleware.clj

                                 (set/union (roles/get-roles (getx-user-id))
                                            (organizations/get-all-organization-roles (getx-user-id))
                                            (workflow/get-all-workflow-roles (getx-user-id))
-                                            (applications/get-all-application-roles (getx-user-id))))
+                                            (application/get-all-application-roles (getx-user-id))))


Let's refactor this to use rems.service.user/get-roles which would use rems.service.cache/get-roles and do this whole thing. User roles would be cached as a projection but the whole result should be cached too (it joins many tables).

Macroz · 2023-11-10T10:31:04Z

src/clj/rems/db/applications.clj

+    (when (empty? (:errors result)) ; nothing can't change when there's an error
+      (dependencies/notify-watchers! {:application/id (mapv :application/id (:events result))}))


Is this double notification? Wouldn't the lower level functions be enough (add-event etc.)?

Macroz · 2023-11-10T10:35:30Z

src/clj/rems/db/applications.clj

+(defn get-applications-with-user [userid]
+  (throw (RuntimeException. "get-applications-with-user not implemented")))


Is this needed?

Macroz and others added 10 commits June 8, 2023 20:06

wip: cache and trace

5c1eeb8

wip: renamed functions and proper layers

dd51f04

feat: cache get-full-internal-catalogue-item

e2abffd

feat: delay search indexing to a poller

24ee534

fix: reset indexing requests

2b001b8

Merge remote-tracking branch 'origin/master' into cachee

c7b10b5

fix: formatting

eeeaae1

refactor: contains? is not needed as it's a set

2dd65ac

refactor: filter first to reduce work

e5a219d

There could be less rows to project after filtering.

Merge remote-tracking branch 'origin/master' into cachee

ea8dbc9

Macroz commented Nov 2, 2023

View reviewed changes

Macroz commented Nov 9, 2023

View reviewed changes

src/clj/rems/cache.clj

@@ -0,0 +1,76 @@

(ns rems.cache

Copy link

Collaborator Author

Macroz Nov 9, 2023

Choose a reason for hiding this comment

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

TODO: Implement (use from service.cache)

Macroz commented Nov 9, 2023

View reviewed changes

src/clj/rems/db/applications.clj

;;; Cache

(defn reload-cache! []

;; TODO implement

Copy link

Collaborator Author

Macroz Nov 9, 2023

Choose a reason for hiding this comment

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

Remove?

Macroz commented Nov 10, 2023

View reviewed changes

Macroz mentioned this pull request Nov 22, 2023

Faster events #3227

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring layers and cache #3175

Refactoring layers and cache #3175

Macroz commented Aug 8, 2023 •

edited

Macroz Nov 2, 2023

Macroz Nov 9, 2023

Macroz Nov 9, 2023

Macroz Nov 9, 2023

Macroz Nov 9, 2023

Macroz Nov 9, 2023

Macroz Nov 9, 2023

Macroz Nov 9, 2023

Macroz Nov 9, 2023

Macroz Nov 9, 2023

Macroz Nov 9, 2023

Macroz Nov 9, 2023

Macroz Nov 9, 2023 •

edited

Macroz Nov 9, 2023

Macroz Nov 10, 2023

Macroz Nov 10, 2023 •

edited

Macroz Nov 10, 2023

Macroz Nov 10, 2023

Macroz Nov 10, 2023

Macroz Nov 10, 2023

Macroz Nov 10, 2023

Macroz Nov 10, 2023 •

edited

Macroz Nov 10, 2023 •

edited

Macroz Nov 10, 2023

Macroz Nov 10, 2023

		(mapv project-application-overview)
		(filter-with-search query))))))

		(when (empty? (:errors result)) ; nothing can't change when there's an error
		(dependencies/notify-watchers! {:application/id (mapv :application/id (:events result))}))

		(defn get-applications-with-user [userid]
		(throw (RuntimeException. "get-applications-with-user not implemented")))

Refactoring layers and cache #3175

Are you sure you want to change the base?

Refactoring layers and cache #3175

Conversation

Macroz commented Aug 8, 2023 • edited

Checklist for author

Reviewability

Backwards compatibility

Documentation

Testing

Follow-up

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

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

Macroz Nov 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Macroz Nov 10, 2023 • edited

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

Macroz Nov 10, 2023 • edited

Choose a reason for hiding this comment

Macroz Nov 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Macroz commented Aug 8, 2023 •

edited

Macroz Nov 9, 2023 •

edited

Macroz Nov 10, 2023 •

edited

Macroz Nov 10, 2023 •

edited

Macroz Nov 10, 2023 •

edited