(dev/core#217) PrevNext - More conservative transition (5.7=>5.8=>5.9) #13164
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ad of SQL JOIN
The general context of this code is roughly as follows:
* We've already filled up the prevnext cache with a bunch of contact-IDs.
* The user wants to view a page of 50 contacts.
* We want to lookup full information about 50 specific contacts for this page.
It does makes sense to use `CRM_Contact_BAO_Query` for looking up the "full information"
about contacts. However, the function `Query::getCachedContacts()` is hard-coded to
read from the SQL-based prevnext cache.
Before
------
* In `getCachedContacts()`, it grabbed the full SQL for `CRM_Contact_BAO_Query`
and munged the query to:
* Add an extra JOIN on `civicrm_prevnext_cache` (with a constraint on `cacheKey`)
* Respect pagination (LIMIT/OFFSET)
* Order results based on their position in the prevnext cache
After
-----
* In `CRM_Core_PrevNextCache_Interface`, the `fetch()` function provides one page-worth
of contact IDs (in order). The `fetch()` function is tested by `E2E_Core_PrevNextTest`.
* In `getCachedContacts()`, it doesn't know anything about `civicrm_prevnext_cache`
or `cacheKey` or pagination. Instead, it just accepts CIDs for one page-worth of
contacts. It returns contacts in the same order that was given.
The new name is prettier and matches the names in `CRM_Core_PrevNextCache_{Interface,Redis}`.
The auto-detection is a good default policy. However, this is new functionality. If some bug gets through the review/RC cycles, then this option provides an escape path.
This revision sets up more conservative transition process for adopting PrevNext drivers. Before ------ * 5.7.x - Hard-coded to SQL driver. * 5.8.x - Allow setting to pick driver. If left as `default`, then choose best-available (based on configured services). After ------ * 5.7.x - Hard-coded to SQL driver. * 5.8.x - Allow setting to pick driver. If left as `default`, then use SQL driver. * 5.9.x - Allow setting to pick driver. If left as `default`, then choose best-available (based on configured services). This essentially mitigates the risk that bugs in the new Redis driver cause regreessions for sites already running Redis.
|
(Standard links)
|
|
NOTE: Relevant comment from @eileenmcnaughton on 12665:
We can close or merge this PR whenever that feedback comes. |
|
Ah well no harm in being more conservative I guess. I can't see a downside in merging this so let's do it |
|
I'm gonna try 'rebase and merge' just to see - I suspect I'll lose the merge commit but we'll see |
|
OK - I can confirm using that lost me some history - it makes it look like you pushed a commit directly. If anyone accuses you of that you can point the finger at me. I will avoid using that in future |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This revision sets up more conservative transition process for adopting new PrevNext drivers.
Before
default, then auto-detect best-available (based on configured services).After
default, then use SQL driver.default, then auto-detect best-available (based on configured services).This essentially mitigates the risk that bugs in the new Redis driver cause regreessions for sites already running Redis.
Comments
This extends/depends on #12665. The new change here is only one commit - the last one, 318583d.
An alternative way to achieve the same goal (more conservative transition) would be to default to SQL under all circumstances -- and (if Redis is available, then...) show a status message suggesting that the admin change the settings. OTOH, 6 months from now, we'll probably be happier to just have it auto-detect the best driver. I'm OK to close and do a different PR if folks really want that process instead, but either approach seems fair enough IMHO.