Last Comment Bug 530270 - Whining fails if mail queueing is enabled
: Whining fails if mail queueing is enabled
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Email Notifications (show other bugs)
: 3.4.4
: All All
: -- critical (vote)
: Bugzilla 3.4
Assigned To: Frédéric Buclin
: default-qa
:
Mentors:
Depends on:
Blocks: bmo-regressions-0911
  Show dependency treegraph
 
Reported: 2009-11-20 23:51 PST by Dave Miller [:justdave] (justdave@bugzilla.org)
Modified: 2010-02-28 10:42 PST (History)
7 users (show)
LpSolit: approval+
LpSolit: approval3.4+
mkanat: blocking3.4.5+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (2.56 KB, patch)
2009-11-21 08:14 PST, Frédéric Buclin
mkanat: review-
Details | Diff | Splinter Review
patch, v2 (2.66 KB, patch)
2009-11-22 09:15 PST, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review

Description Dave Miller [:justdave] (justdave@bugzilla.org) 2009-11-20 23:51:09 PST
whine.pl is still using the shadow database at the point where it tries to send mail.  This causes it to try to insert into the slave when adding to the queue, and the insert fails.

Inserting a send_mail job into the Job Queue failed with the following
error: Failed to execute INSERT INTO ts_job (funcid, arg, uniqkey,
insert_time, run_after, grabbed_until, priority, coalesce) VALUES (?,
?, ?, ?, ?, ?, ?, ?) with funcid, arg, uniqkey, insert_time, run_after,
grabbed_until, priority, coalesce: DBD::mysql::st execute failed: The
MySQL server is running with the --read-only option so it cannot
execute this statement at
/usr/lib/perl5/vendor_perl/5.8.8/Data/ObjectDriver/Driver/DBI.pm line
380.

The proper place to fix this is probably in the queue management code.  The queue insert routines should never be talking to the slave database.
Comment 1 Max Kanat-Alexander 2009-11-21 00:03:57 PST
Yeah, agreed with your suggested resolution. We should have an on_main_database routine that takes a block and performs the code using the main database.
Comment 2 Frédéric Buclin 2009-11-21 08:14:10 PST
Created attachment 413823 [details] [diff] [review]
patch, v1

Create Bugzilla->main_dbh, which returns the main DB handle without switching to it.
Comment 3 Max Kanat-Alexander 2009-11-21 13:09:59 PST
Comment on attachment 413823 [details] [diff] [review]
patch, v1

Let's call it dbh_main, consistently. (It has two names in this patch, main_dbh and dbh_main.)

Also, I was thinking that instead, we'd create a function called on_main_db(&) that takes a block and makes Bugzilla->dbh be the main db for that block. I think that might be more useful in the general case, because that would also make any subroutines called use the main dbh.
Comment 4 Bradley Baetz (:bbaetz) 2009-11-21 18:56:10 PST
Please add a comment to JobQueue.pm explaining this.

Also, this is going to make the QueueRunner run on the main db too, but I don't think that that's a problem - certainly won't make any difference for the current job we run
Comment 5 Frédéric Buclin 2009-11-22 09:08:53 PST
(In reply to comment #3)
> (From update of attachment 413823 [details] [diff] [review])
> Let's call it dbh_main, consistently. (It has two names in this patch, main_dbh
> and dbh_main.)

Haha, yes, I wrote it quickly. :)


> Also, I was thinking that instead, we'd create a function called on_main_db

This wouldn't be really useful as we already have switch_to_main_db() for that. I will just update my patch to fix the function name.
Comment 6 Frédéric Buclin 2009-11-22 09:15:58 PST
Created attachment 413934 [details] [diff] [review]
patch, v2
Comment 7 Max Kanat-Alexander 2009-11-22 14:00:38 PST
(In reply to comment #5)
> This wouldn't be really useful as we already have switch_to_main_db() for that.

  It's true, but that doesn't do this:

  1) Are we running on the shadow? If so, switch to the main and remember that we were running on the shadow.
  2) At the end of the block, switch back to using the shadow, only if we were on the shadow before.

  It's not a *lot* of code, but it would simplify using the main dbh in a lot of places.

  For this bug, though, I agree, main_dbh is all we need.
Comment 8 Max Kanat-Alexander 2009-11-22 14:02:04 PST
Comment on attachment 413934 [details] [diff] [review]
patch, v2

>Index: Bugzilla/JobQueue.pm
>+    $class->request_cache->{dbh} = $class->dbh_main;
>+    return $class->{dbh_main};

  That needs to be $class->dbh_main, on the last line there.

  Otherwise this looks fine.
Comment 9 Frédéric Buclin 2009-11-22 14:05:22 PST
(In reply to comment #8)
> >+    $class->request_cache->{dbh} = $class->dbh_main;
> >+    return $class->{dbh_main};
> 
>   That needs to be $class->dbh_main, on the last line there.

This doesn't *need* to be, as all it would do would be to return ->{dbh_main}, which I do explicitly here.
Comment 10 Max Kanat-Alexander 2009-11-22 14:06:23 PST
(In reply to comment #9)
> This doesn't *need* to be, as all it would do would be to return ->{dbh_main},
> which I do explicitly here.

  No, it does. There is no such thing as $class->{dbh_main}. $class is a string.
Comment 11 Frédéric Buclin 2009-11-22 14:08:26 PST
(In reply to comment #9)
> This doesn't *need* to be, as all it would do would be to return ->{dbh_main},
> which I do explicitly here.

Ah right, I meant $class->request_cache->{dbh_main}. I will use $class->dbh_main, then.
Comment 12 Frédéric Buclin 2009-11-22 14:26:16 PST
tip:

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.84; previous revision: 1.83
done
Checking in Bugzilla/JobQueue.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/JobQueue.pm,v  <--  JobQueue.pm
new revision: 1.4; previous revision: 1.3
done


3.4.4:

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.73.2.3; previous revision: 1.73.2.2
done
Checking in Bugzilla/JobQueue.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/JobQueue.pm,v  <--  JobQueue.pm
new revision: 1.1.2.1; previous revision: 1.1
done

Note You need to log in before you can comment on or make changes to this bug.