In order to support slave queries nicely, killes and I believe that we need to be able to temporarily disable slave-targeted queries for the duration of a page request when we know that certain data is very fresh. Therefore, the attached patch adds the ability to flag a given target as ignored. That allows us to simply disable slave servers whenever we need.
This patch also includes unit tests for same (yay!), but that required also adding a method to Database to add database credentials at runtime. That's a nice feature, too, and one that should actually help with other features and testing.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | database_test-fix-exception-302300-10.patch | 4.68 KB | webchick |
| #7 | mw.patch | 5.08 KB | moshe weitzman |
| #3 | db_ignore_target.patch | 4.99 KB | Crell |
| db_ignore_target.patch | 4.65 KB | Crell |
Comments
Comment #1
dries commentedFor those who want some background reading: this looks equivalent to solution 5 in the blog post I wrote last year http://buytaert.net/database-replication-lag (I think).
I was scratching my head quite a bit when I saw "addConnectionInfo()". I'm still not sure I understand when to use that function. The phpDoc could be a little bit more helpful.
Comment #2
Crell commentedBumping to critical as this is now a blocker for #298669: Add query logging per Connection.
Comment #3
Crell commentedIt is essentially a "smart" version of #5. Rather than each individual query knowing whether it is slave safe in given circumstances, a given query is marked as slave safe, period. Then we introduce a global kill-switch for slave servers. That allows us to do something like disable the slave server for 30 seconds but only for the user that just posted something. Doing the latter will come in another patch.
Attached is a revised version that includes better documentation for addConnectionInfo() and fixes a documentation bug in the unit test. No other changes.
Comment #4
webchickI am going to bed now, but subscribe for tomorrow.
Comment #5
moshe weitzman commentedBoth assertions have the same text - 'Both targets refer to the same connection.' I think we usually state the desired outcome in the assertion text so I've changed the first one to "Each target refers to a different connection.". I ran the DB test suite and all passes. Code looks good.
Reminder that this issue is marked critical because it blocks other progress. See above.
Comment #6
Crell commentedmoshe, I think the patch got lost in the mail. :-)
Comment #7
moshe weitzman commentedoops - here is the patch. just a text change in assertion.
Comment #8
Crell commentedThanks, Moshe!
Comment #9
webchickOk, PHPDoc improvements look good, tests look good.. I had asked about moving the connnection duplication to setUp() but Crell pointed out that we likely want to expand this test case with other types of tests that don't need this done.
Committed to HEAD. Thanks!
Comment #10
webchickI clearly was slacking and didn't run the test suite before committing this. D'oh.
This patch is from Crell via some pastebins and conversations on #drupal. Fixes the exception. Putting it here for him to review.
Comment #11
webchickCrell gave the thumbs-up over IRC, so committed.
Comment #12
Crell commentedFor the record, the unit tests DID pass successfully. There was just a bug in one of the unit tests itself that we didn't catch the first time around. So it's not all webchick's fault. :-)
Comment #13
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.