Save compaction #2966
Save compaction #2966
Conversation
Improvements to the test, so that we can implement compaction without breaking the assumptions. - Check differently that event ids are increasing, so that there doesn't need to be the same amount of events, which will not work with compaction. - Write a different field value each time, but don't do it with the redef. Previously the "app version" was the same as the number of events. With compaction that will not be true, so we'll add a separate (sequential) number to check that the last one wins. - The number of final events is going to be different after compaction. - Event ids will have gaps after compaction is enabled, so we can't enforce that.
If the event is a draft save, and the last event was a draft save too, we'll replace the last event with the new data, and new id, effectively deleting the old one. This will leave gaps in the event ids, but saves space after autosave is added.
The event ids are generated in the database after the command has processed the results. We can update the events in the result to return the latest info.
Added the historical decision and the other issue result.
Let's keep the feature as optional until the rest of the autosave is ready.
The description of `:dev` doesn't make sense as the really experimental features have their own flags. And most of the feature are not experimental.
| (= (:application/id event) | ||
| (:application/id last-event))) |
There was a problem hiding this comment.
can we omit this check, i think application id should always be the same?
(= (:application/id event)
(:application/id last-event))There was a problem hiding this comment.
Now that I look like it, yes we could. I added the condition because it didn't seem to work otherwise, but with a closer inspection it mostly should have and it was probably something else. Technically there can be n events returned from a single command, and they could affect different applications (like "copy as new" does), but here, we are considering the save which never does it.
There was a problem hiding this comment.
I think I'll leave the check here as it is cheap. So it's kind of regression protection if we choose to create more complex commands or use the function from elsewhere.
I don't want to fetch the application of each event here with get-application-internal as that is not so cheap (it fetches the events and rebuilds the view).
|
|
||
| Returns the event as it went into the db." | ||
| [event] | ||
| (let [old-event (fix-event-from-db (first (db/get-application-event {:id (:event/id event)}))) |
There was a problem hiding this comment.
the existing SQL function has a little silly return, considering the name indicates it should return single event but it returns a list of events. maybe that could be refactored at some point :)
There was a problem hiding this comment.
Yes, I think the correct naming convention that we use elsewhere is :get-application-events (which does exist too). There could be a db function named :get-application-event but it should have been declared like get-latest-application-event i.e. :1. This is a bit of leftover from my refactoring of combining together some previous functions, but choosing the wrong name / not finishing all the changes. "search" functions should accept many parameters and return 0..n and singular "get" functions should always return 0..1.
Implements consecutive save compaction for #2767
Checklist for author
Remove items that aren't applicable, check items that are done.
Reviewability
Backwards compatibility
Documentation
Testing
Follow-up