SearchKit - Refresh DB entities via table-swap #31767
Conversation
Overview --------- This updates the handling of "DB Entity" (table-mode). In this mode, you need some mechanism to update the content of the table (e.g. the "refresh" operation). This updates the mechanism. Before ------ TRUNCATE the table and re-INSERTs with regenerated data. In the period between the truncation and regenerated data, the content of the table is ill-defined. After ----- CREATE a new table with a temporary name and fill that. Once ready, swap the old table and new table (atomicly).
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
|
I agree this this is all-around better and there's no need to clutter/confuse the admin UX with one-more-freaking-setting-to-configure when there's no known downside to always doing it this way. |
colemanw
added
the
merge ready
| // Only one process should actually refresh this entity (at a given time). | ||
| $lock = \Civi::lockManager()->acquire("data.skentity." . $display['id'], 1); | ||
| if (!$lock->isAcquired()) { | ||
| throw new \Civi\Search\Exception\RefreshInProgressException(sprintf('Refresh (%s) is already in progress', $this->getEntityName())); |
There was a problem hiding this comment.
@colemanw You probably know the auto-refresh flag and existing callers better than me. Will emitting this kind of error cause random trouble?
IMHO, in a greenfield, this is good style. But it a brownfield, it might require sprinkling some try/catch expressions?
There was a problem hiding this comment.
Well, the SearchKit UI calls this function every time you hit the "Save" button. It does so asynchronously so if this action fails it doesn't prevent the rest of the save from happening...
but I'd imagine if you repeatedly hit the "Save" button while rapidly making changes to the search, you'd manage to trigger the failure, which means your latest changes to the savedsearch wouldn't be reflected in the skentity table...
until the next time you hit the Save button or it gets refreshed via cron.
That's not a disastrous outcome by any means, but a nicer behavior would be like a tail-end debounce where e.g. if you click the button 5 times in quick succession, then refreshes 1-4 get cancelled and the 5th one gets to complete.
Overview
This updates the handling of "DB Entity" (table-mode). In this mode, you need some mechanism to update the content of the table (e.g. the "refresh" operation). This revises the mechanism.
(This is an alternative to #31768. ping @colemanw)
Before
Clear the table (TRUNCATE) and re-fill with regenerated data (INSERT) .
In the period between the truncation and the last insertion, the content of the table is ill-defined.
After
Make a new table with a temporary name and fill that. The original table remains available during the build. Once ready, swap the tables atomically.
Comments
I'm kind of ambivalent about whether to do a straight policy-change or to configurable policy-change. This PR is a straight change-- which is simpler. In theory, new algo sounds better -- but it's clearly not proven yet. There may be some trade-offs (eg space-efficiency), and there may be other modes the future. shrug