Closed Bug 530270 Opened 15 years ago Closed 15 years ago

Whining fails if mail queueing is enabled

Categories

(Bugzilla :: Email Notifications, defect)

3.4.4
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Bugzilla 3.4

People

(Reporter: justdave, Assigned: LpSolit)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Flags: blocking3.4.5?
Assignee: whining → email-notifications
Component: Whining → Email Notifications
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.
Flags: blocking3.4.5? → blocking3.4.5-
Flags: blocking3.4.5- → blocking3.4.5+
Target Milestone: --- → Bugzilla 3.4
Attached patch patch, v1 (obsolete) — Splinter Review
Create Bugzilla->main_dbh, which returns the main DB handle without switching to it.
Assignee: email-notifications → LpSolit
Status: NEW → ASSIGNED
Attachment #413823 - Flags: review?(mkanat)
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.
Attachment #413823 - Flags: review?(mkanat) → review-
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
(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.
Attached patch patch, v2Splinter Review
Attachment #413823 - Attachment is obsolete: true
Attachment #413934 - Flags: review?(mkanat)
(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.
Attachment #413934 - Flags: review?(mkanat) → review+
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.
(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.
Flags: approval3.4+
Flags: approval+
(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.
(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.
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.