More generic layout for caching. #13
Conversation
|
Do we need |
|
I believe the intent of the "fetched_at" column was for cache invalidation, but I don't think we need it at all. |
|
Actually, even if we had a cron job later, it could use the |
|
You are correct in your assessment. Truth be told, I think database caching is a silly idea for the most part. I preserved it because it was original to the tool, but for the most part it's far slower than other forms of caching. |
|
Agree, but it comes in handy as well for a quick setup. |
| require_once $dir . '/cache/BugzillaCacheI.class.php'; | ||
| require_once $dir . '/cache/BugzillaCacheSql.class.php'; | ||
| require_once $dir . '/cache/BugzillaCacheMysql.class.php'; | ||
| require_once $dir . '/cache/BugzillaCacheDummy.class.php'; |
There was a problem hiding this comment.
Almost everything else in this project is using require_once( ... ); (with parentheses), why not keeping that style? Consistency for teh win! :)
|
@AdrianGaudebert thanks for the feedback. |
|
Thanks for addressing all my comments @rdalverny! I'll test your pull request as soon as I can, @brandonsavage is fixing a bug with a recent version of mediawiki that prevents me from running the extension. |
|
There's a lot of good stuff in this request, and I think it deserves consideration on its merits. I tried to test this but I discovered that it conflicts with the work that we had to do to fix the MySQL cache. If you wouldn't mind updating from master and resolving the conflicts, then resubmitting, I'll consider this request ASAP. The way has been cleared by integrating the caching work that was pending for a long time. |
|
Just rebased from master and merged the changes. And it looks like it merges ok with master. But I wonder if it wouldn't be cleaner to push those changes in smaller, dedicated patches? |
|
Spent a good bit of the day looking this over. With regards to breaking it into smaller patches, what's the list of features we are adding here? Our typical policy is one commit per discrete feature. If these features are all interoperable, then we should commit them as a group. |
|
I was confused by the merges I did. Actually, the whole list of changes is consistent: I updated the features in the description of the PR. I'm trying to rebase the whole list of commits to have something more clean now. |
- new $wgBugzillaCacheType, replaces $wgBugzillaUseCache and $wgCacheObject with mysql, apc, memcache, postgresql, sqlite, dummy types; - new Bugzilla::getCacheClass() and ::getCache() static methods; - new BugzillaCacheI::setup() abstract method, called by BugzillaCreateCache(); - new BugzillaCacheSql class, handling any generic MW SQL-backend; BugzillaCacheMysql is dropped; - separate SQL CREATE TABLE definitions for MySQL, PostgreSQL and SQLite; - base64_encode/decode cached values, fixes SQL escaping issues; - drop cache db fetched_at column.
|
You might want to look at https://github.com/rdalverny/mediawiki-bugzilla/tree/rework_cache_again instead (I reapplied those changes commit by commit). I can submit it as a new PR. |
|
Do you need anything I could help with regarding this PR? |
|
What's the hold up here? |
More generic layout for caching.
and $wgCacheObject
BugzillaCacheMysql now derives from it (but could be dropped,
there's not much specific to MySQL here);