More generic layout for caching. #13

Merged
merged 1 commit into from Apr 4, 2013

Conversation

Projects
None yet
4 participants
Contributor

rdalverny commented Aug 6, 2012

  • new $wgBugzillaCacheType, replaces dropped $wgBugzillaUseCache
    and $wgCacheObject
  • 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 now derives from it (but could be dropped,
    there's not much specific to MySQL here);
  • separate SQL CREATE TABLE definitions for MySQL, PostgreSQL and SQLite.
  • base64_encode/_decode() cached values (prevents issues about escaping data for SQL storage)
Contributor

rdalverny commented Aug 8, 2012

Do we need fetched_at column at all?

Collaborator

brandonsavage commented Aug 8, 2012

I believe the intent of the "fetched_at" column was for cache invalidation, but I don't think we need it at all.

Contributor

rdalverny commented Aug 8, 2012

Actually, even if we had a cron job later, it could use the expires column.

Collaborator

brandonsavage commented Aug 8, 2012

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.

Contributor

rdalverny commented Aug 8, 2012

Agree, but it comes in handy as well for a quick setup.

Bugzilla.class.php
+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';
@adngdb

adngdb Aug 10, 2012

Member

Almost everything else in this project is using require_once( ... ); (with parentheses), why not keeping that style? Consistency for teh win! :)

Bugzilla.class.php
@@ -48,5 +53,33 @@ public static function create($config=array(), $opts=array(), $title='') {
return $b;
}
+
+ /**
+ */
@adngdb

adngdb Aug 10, 2012

Member

Empty docstrings are pretty useless, could you please add a small comment about what that function and other uncommented ones do?

$wgBugzillaTagName = 'bugzilla'; // The tag name for your Bugzilla installation (default: 'bugzilla')
$wgBugzillaMethod = 'REST'; // XML-RPC and JSON-RPC may be supported later
-$wgBugzillaUseCache = TRUE; // Use the built-in cache (default: TRUE)
@adngdb

adngdb Aug 10, 2012

Member

Setting $wgBugzillaCacheType to dummy is the equivalent of setting $wgBugzillaUseCache to false in the previous version? If so, can you please add a comment saying that?

BugzillaOutput.class.php
@@ -144,10 +143,10 @@ public function setup_template_data() {
global $wgBugzillaChartUrl;
- $key = md5($this->query->id . $this->_get_size() . get_class($this));
+ $key = md5($this->query->id . $this->_get_size() . get_class($this));
@adngdb

adngdb Aug 10, 2012

Member

This is something I really don't like, mainly because it makes diffs bigger for no really good reasons. I agree it makes code a little better and easier to read, but as soon as you add something that is too long, you'll need to change a lot of lines just for style consistency, and that's a bad result to me.

Anyway, that's just a comment from me and not a real expectation on code quality. Just sayin', in a way... :)

@rdalverny

rdalverny Aug 11, 2012

Contributor

I agree about the diff, increased size. I guess that should be pushed as a separate, dedicated commit for style updates :-p Code style & readability matters as well as a readable diff.

cache/BugzillaCacheMysql.class.php
- protected $_master;
-
- public function __construct()
+ /**
@adngdb

adngdb Aug 10, 2012

Member

This is 2-spaces indented when everything else is 4-spaces indented. Can you please move to 4-spaces everywhere?

@@ -0,0 +1,120 @@
+<?php
@adngdb

adngdb Aug 10, 2012

Member

This entire file is mixing 2-spaces indentation and 4-spaces indentation. Can you please turn everything to 4-spaces indentation?

cache/sql/postgresql.sql
@@ -0,0 +1,6 @@
+CREATE TABLE bugzilla_cache (
+ id SERIAL PRIMARY KEY,
+ key VARCHAR(255) UNIQUE NOT NULL DEFAULT '',
@adngdb

adngdb Aug 10, 2012

Member

As key is a reserved keyword in SQL, you should here add tildes (```) around the field names as you did it for other sql files.

Contributor

rdalverny commented Aug 11, 2012

@adriangaudebert thanks for the feedback.

Member

adngdb commented Aug 11, 2012

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.

Collaborator

brandonsavage commented Dec 11, 2012

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.

Contributor

rdalverny commented Dec 12, 2012

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?

Collaborator

brandonsavage commented Dec 13, 2012

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.

Contributor

rdalverny commented Dec 14, 2012

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.

More generic layout for caching.
- 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.
Contributor

rdalverny commented Dec 15, 2012

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.

Contributor

rdalverny commented Feb 11, 2013

Do you need anything I could help with regarding this PR?

Contributor

reedy commented Mar 30, 2013

What's the hold up here?

brandonsavage added a commit that referenced this pull request Apr 4, 2013

Merge pull request #13 from rdalverny/rework_cache
More generic layout for caching.

@brandonsavage brandonsavage merged commit 7249470 into mozilla:master Apr 4, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment