Whining fails if mail queueing is enabled

RESOLVED FIXED in Bugzilla 3.4

Status

()

Bugzilla
Email Notifications
--
critical
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: justdave, Assigned: Frédéric Buclin)

Tracking

3.4.4
Bugzilla 3.4
Bug Flags:
approval +
approval3.4 +
blocking3.4.5 +

Details

Attachments

(1 attachment, 1 obsolete attachment)

2.66 KB, patch
Max Kanat-Alexander
: review+
Details | Diff | Splinter Review
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?
Blocks: 528846
Assignee: whining → email-notifications
Component: Whining → Email Notifications

Comment 1

8 years ago
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-

Updated

8 years ago
Flags: blocking3.4.5- → blocking3.4.5+

Updated

8 years ago
Target Milestone: --- → Bugzilla 3.4
(Assignee)

Comment 2

8 years ago
Created attachment 413823 [details] [diff] [review]
patch, v1

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 3

8 years ago
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
(Assignee)

Comment 5

8 years ago
(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.
(Assignee)

Comment 6

8 years ago
Created attachment 413934 [details] [diff] [review]
patch, v2
Attachment #413823 - Attachment is obsolete: true
Attachment #413934 - Flags: review?(mkanat)

Comment 7

8 years ago
(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.

Updated

8 years ago
Attachment #413934 - Flags: review?(mkanat) → review+

Comment 8

8 years ago
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.
(Assignee)

Comment 9

8 years ago
(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+

Comment 10

8 years ago
(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.
(Assignee)

Comment 11

8 years ago
(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.
(Assignee)

Comment 12

8 years ago
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
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.