284184 - process_bug.cgi is slow because it sends mail immediately

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect, P1)

Description

[:glob ✱]

currently we send email in the middle of a database lock (bug 181059) and before
the pages are returned to the browser.

ok, here's a off-the-wall approach to fixing this problem.

instead of sendmail email when messageToMTA is called, the message is pushed
into a pending queue, which is processed when the script ENDs.

this should ensure that we're alway outside of database locks and that as much
content of the page as possible has been display before we start email
delivery.

however this may also have an impact memory-wise for bugs that trigger a lot of
emails.


i've thought about forking for email delivery however there's two major
drawbacks with this:

  - windows. windows emulates forking with threads, and from my experience
    there a problems with database handles and threads being terminated
    prematurely.

  - error messages.  right now we email error management is limited to
    outputting to stderr or stdout.  if we fork, the error goes to /dev/null

Comment 1

[:glob ✱]

Attachment: late email sending v1

Comment 2

[Joel Peshkin]

What happens when...
a) You have error messages from the MTA?
b) The cgi gets killed before the mail goes out, but lastdiffed is already
updated in the db?

Comment 3

[:glob ✱]

> a) You have error messages from the MTA?

they will be displayed at the end of the html, after the footer (currently they
are displayed before the header).

> b) The cgi gets killed before the mail goes out, but lastdiffed is already
> updated in the db?

the email is lost :(  moral of the story: don't kill running cgi's.

i could update lastdiffed in the delivery sub..

Comment 4

[Max Kanat-Alexander]

No, don't touch the database in the END block -- that would be bad. We have no
guarantee that your END block will run before the END block that kills $dbh.

CGI's normally can't be killed in Bugzilla, since we trap TERM and so forth.

Comment 5

[:glob ✱]

Attachment: late email sending v2

fixes bitrot.

this also has a slight change to the error management.

currently if we fail to pass the message to the mta (rare with sendmail, not so
rare with smtp), the logic is to die and stop processing any further actions
(which may result in database corruption -- bug 225042).

with this patch, all database work is done first, so we don't have sanitycheck
to fall back on to deliver the unsent emails.

any errors that happen during delivery of a particular message do not stop
processing of the unsent message queue, otherwise we'll lose emails for sure.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 176097 [details] [diff] [review]
late email sending v2

>+our @pendingMessages;

  This needs to be explicitly initialzed, for mod_perl compatibility.

>+        printf "<pre>%s</pre>", html_quote($@) if $@;

  OK, so this will notify somebody if the message fails to send... but they
might not ever see it! Particularly because it's at the bottom of the page. :-(
And when they miss it, there's no way to know that an email wasn't sent. We
can't put the error message at the top of the page... because the page has
already rendered. Also, this will probably put any error messages past the
final </html> -- will that cause a problem in any browser?

  Other than that, I don't think it introduces any new bugs that otherwise
wouldn't exist.

  I think that we should add a "check_mail_connection" sub somewhere in
BugMail.pm and either call it from Send() or somewhere in process_bug. That
assures us that at least we can connect to send mail right now, once. It would
be nice if it happened before any actual bug processing in post_bug or
process_bug, because then we could also avoid any changes happening if email
can't be sent for them. I think that would head off most of the problem.

  This would also need a pretty good amount of testing...

Comment 7

[:glob ✱]

> .. but they might not ever see it! 
> Particularly because it's at the bottom of the page. :-(

yeah :(  but i don't think we have any other option.

> this will probably put any error messages past the
> final </html> -- will that cause a problem in any browser?

perhaps; for the record both firefox and ie show the error.

the current process puts the errors before the opening <html> -- isn't that the
same problem?

> I think that we should add a "check_mail_connection" sub somewhere in
> BugMail.pm and either call it from Send() or somewhere in process_bug.
> assures us that at least we can connect to send mail right now, once.

yes and no.  it would ensure that we can *connect* to the mta, not that we can
send email.  with sendmail, that'll good enough.  with smtp, it's possible that
the smtp server will reject the message after the connection has been established.

> I think that would head off most of the problem.

i don't think so.  

the default from address for bugzilla - "bugzilla-daemon" - will be rejected by
pretty well all smtp servers (no domain), and 'relaying denied' is also common.

the only sure fire way to test the mta is to get it to deliver a valid email.

hrm.. we could perform the validation when the maildeliverymethod parameter is
set, by sending a test email to the maintainer.  that way we'll catch
mta-related configuration issues.

> This would also need a pretty good amount of testing...

roger that!

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> the default from address for bugzilla - "bugzilla-daemon" - will be rejected 
> by pretty well all smtp servers (no domain), and 'relaying denied' is also 
> common.

  Yeah. The default address should probably be bugzilla-daemon@localhost.

> hrm.. we could perform the validation when the maildeliverymethod parameter is
> set, by sending a test email to the maintainer.  that way we'll catch
> mta-related configuration issues.

  I think that's a good idea. Of course, we'd also have to do it during
checksetup for people who never touch maildeliverymethod.


  As far as the errors, I think that having them before the opening <html> is
more likely to work well than having them before the closing </html>, because
before that, we print out something that doesn't even look like well-formed
HTML, so the browser assumes that we're silly and gave it bad headers. While if
we do it after the </html>, it looks like we're just strangely broken in some
other way.

  I'm more concerned with the fact that process_bug is a long page, and people
will miss the errors.

Comment 9

[:glob ✱]

discussed this with max on irc.

the errors probably aren't very useful to the end user, however the bugzilla
admin needs to know about them.

we should write the errors to STDERR (so the http error log gets them) and also
to data/errors.log and add the ability to view errors.log when logged in a admin.

Comment 10

[Max Kanat-Alexander]

I've modified the summary of this bug to more accurately reflect the problem rather than the suggested resolution.

We've also thought about making a queue-running daemon to handle mail. We could just insert info about the mail into the database, and then let a daemon run that queue.

For the daemon, we should look into TheSchwartz, which is a general-purpose job queue module:

http://search.cpan.org/dist/TheSchwartz/lib/TheSchwartz.pm

Comment 11

[Frédéric Buclin]

(In reply to comment #10)
> I've modified the summary of this bug to more accurately reflect the problem
> rather than the suggested resolution.

With such a bug summary, this clearly makes it a dupe of bug 181059.

Comment 12

[Max Kanat-Alexander]

Yes, agreed. Since we've decided not to go the way of the END block, this is now a dupe.

Comment 13

[Dave Miller [:justdave]]

Actually, I don't think it is...  The other bug appears to be specifically about flag mail, not bug change mail.  Those are two separate entities, and although the fix might end up fixing both, there's no guarantee it will, and until we have a patch in hand that does, they're best off remaining separate issues to make sure they both get addressed.

Comment 14

[Dave Miller [:justdave]]

Just to keep things together, there was a major discussion about this on IRC on 2007-12-03

http://bugzilla.glob.com.au/irc/?a=date&s=2007-12-03&e=2007-12-03&h=
starting about 07:06 and going on for a few hours.

Comment 15

[Mark Smith [:xb95]]

Attachment: use_mail_queue parameter and mailer change

Comment 16

[Mark Smith [:xb95]]

Attachment: cron job for sending mail

Comment 17

[Mark Smith [:xb95]]

Just attached, my attempt at solving this.  Introduces a new parameter called "use_mail_queue" which causes all mails to be enqueued in the database.

The queue contains an object frozen with Storable.  The process runner (cron job) is designed to run every minute or two and processes all of the mails that it can dig out of the databases.

Mails are not retried, it's a best effort approach.  If it fails, it fails, we move on.

This is a pretty simple patch, please comment.  If you don't like the approach in any way, let me know.

THIS PATCH IS AGAINST 3.0.  As my primary goal is to patch for b.m.o, I figured this is the best bet.  I've no idea if this applies against 3.2, but I will investigate that.

Comment 18

[Max Kanat-Alexander]

Comment on attachment 299024 [details] [diff] [review]
use_mail_queue parameter and mailer change

I don't think we should use Storable. I know that you used nfreeze so we could move things around to different machines, but generally, using blobs has just caused more trouble than it's worth. Also, blobs always require special handling in different DBs, so I try to avoid them as much as possible.

I think it'd be better to store just the mail text, and then pull it back out and send it to MessageToMTA.

Also, I don't think a cron job will cut it. Many Bugzillas send so much email that if we only send mail once per minute, over time they'll become hopelessly lagged. I think we need a daemon.

Comment 19

[Max Kanat-Alexander]

Or, really, I think we need something that can be a cron job or a daemon, so small installs can get the easy option of a cron job, but large ones have to use the daemon.

Also, Windows "Task Scheduler" can't do anything more often than once a day. So the cron job would be problematic there.

Comment 20

[Max Kanat-Alexander]

Comment on attachment 299025 [details]
cron job for sending mail

>#!/usr/bin/perl -w

  I'm wondering if this ought to be in taint mode.

>    # always delete it; if there's a problem with the mail, we don't really
>    # care too much to think it worth retrying
>    $dbh->do('DELETE FROM mail_queue WHERE mailid = ?', undef, $hr->{mailid});

  Noooo. That means if there's a problem with the MTA for two days, all the mail is just "gone" for two days.

Comment 21

[Mark Smith [:xb95]]

(In reply to comment #20)
> >    # always delete it; if there's a problem with the mail, we don't really
> >    # care too much to think it worth retrying
> >    $dbh->do('DELETE FROM mail_queue WHERE mailid = ?', undef, $hr->{mailid});
> 
>   Noooo. That means if there's a problem with the MTA for two days, all the
> mail is just "gone" for two days.
> 

Is that any different than it is now?  If the MTA is having issues, the CGIs are going to be non-functional trying to send.  "Make mail durable/guaranteed" wasn't a design constraint of my patch.  Should it be?

Comment 22

[Mark Smith [:xb95]]

Thank you for the feedback.  It is fairly easy to take this into account, so I will do a second revision:

1) Store mail as plaintext, do not use Storable.  This will require some more munging of the Mailer code to abstract the Email::Send construction so it doesn't have to be duplicated.  Perhaps some other fields stored in the table such as To, Date, Subject, etc as well.

2) Use TheSchwartz modules for inserting/getting jobs.  This provides the durability as well as the ability to run many daemons.  Of course it's an added optional dependency for asynchronous mail.

Comment if this sounds wrong.  :-)

Comment 23

[Max Kanat-Alexander]

(In reply to comment #21)
> Is that any different than it is now? 

  When I mail is sent, Bugzilla sets "lastdiffed" in the bugs table. That way, we know if a mail was sent or not, and there's a utility to send mail that was never sent.

> "Make mail durable/guaranteed"
> wasn't a design constraint of my patch.  Should it be?

  Well, only in the sense that we should retry the sending if it fails, not just skip over it.

Comment 24

[Max Kanat-Alexander]

(In reply to comment #22)
> 1) Store mail as plaintext, do not use Storable.  This will require some more
> munging of the Mailer code to abstract the Email::Send construction so it
> doesn't have to be duplicated.  Perhaps some other fields stored in the table
> such as To, Date, Subject, etc as well.

  I wouldn't store anything in the table. What gets passed to MessageToMTA is something that you could just re-submit to MessageToMTA--I think that'd be the simplest. That is, just catch the text (or get the message ->as_string) before it goes through anything in MessageToMTA, and then have a parameter to MessageToMTA (or something like that) that means, "Actually really send this now." (Or probably better, a parameter that means, "queue this.")

> 2) Use TheSchwartz modules for inserting/getting jobs.  This provides the
> durability as well as the ability to run many daemons.  Of course it's an added
> optional dependency for asynchronous mail.

  Great, that's fine.

Comment 25

[:glob ✱]

> Also, Windows "Task Scheduler" can't do anything more often than once a day. So
> the cron job would be problematic there.

not true -- you can configure a task to run as frequently as every 5 minutes with window's task scheduler.

Comment 26

[Mark Smith [:xb95]]

Attachment: patch to add TheSchwartz support to MessageToMTA

Comment 27

[Mark Smith [:xb95]]

Attachment: Bugzilla/Worker/Mailer.pm (new file)

Comment 28

[Mark Smith [:xb95]]

Attachment: Bugzilla/TheSchwartz.pm (new file)

Comment 29

[Mark Smith [:xb95]]

Attachment: theschwartz_worker.pl (new file)

Comment 30

[Mark Smith [:xb95]]

(Sorry for attachment spam, surely there's a preferred way of doing this.  tgz?)

The first file is the patch to Bugzilla itself to support using the new modules.  In effect, there's now a Bugzilla->theschwartz() method which can be called to get a TheSchwartz handle.  You can then insert jobs.

The Bugzilla::Worker::Mailer module is the actual worker that does the sending mail.  In this case, unpackaging the TheSchwartz job and passing it back to MessageToMTA with a flag to force "send now!"

The small script at the end is a daemon.  This can be run as many times as you want, and it will process and send mails from each daemon.  One thing of particular note, there is no mechanism for spawning workers.  I could write something, but I'm not sure if that'd be preferred, or just assume each site will have its own method of running daemons.

Again, this is tested against 3.0 branch.  If this passes review, then I will do the work to test and patch it against 3.2 as well, to ensure that trunk can benefit from this functionality.

Please take a few minutes to review this, as there is a goal of testing this in b.m.o staging tomorrow sometime.  Thanks!

Comment 31

[Mark Smith [:xb95]]

Also of note, TheSchwartz is only loaded when requested.  It's an optional dependency, and is in no way required.  If someone wishes to turn it on they will have to configure it and have the modules.

I tried to annotate the configuration options as "advanced" so that people aren't likely to touch them.  I'm happy to add documentation as well, but I'm still not quite sure of the proper places to put things like that.  (Code is easy, documentation escapes me.)

Comment 32

[Reed Loden [:reed]]

(In reply to comment #30)
> (Sorry for attachment spam, surely there's a preferred way of doing this. 
> tgz?)

Separate files are preferred for review purposes.

Comment 33

[Max Kanat-Alexander]

(In reply to comment #30)
> (Sorry for attachment spam, surely there's a preferred way of doing this. 
> tgz?)

  diff -Nu /dev/null path/to/file >> patch.diff

> The first file is the patch to Bugzilla itself to support using the new
> modules.  In effect, there's now a Bugzilla->theschwartz() method which can be
> called to get a TheSchwartz handle.  You can then insert jobs.

  Cool. Could we call it Bugzilla->job_queue?

> The Bugzilla::Worker::Mailer

  Hrm, I'd call it Bugzilla::Mailer::Queue if we could.

> The small script at the end is a daemon.  This can be run as many times as you
> want, and it will process and send mails from each daemon.  One thing of
> particular note, there is no mechanism for spawning workers.  I could write
> something, but I'm not sure if that'd be preferred, or just assume each site
> will have its own method of running daemons.

  Hrm. Does that mean it doesn't daemonize itself?

> Please take a few minutes to review this, as there is a goal of testing this in
> b.m.o staging tomorrow sometime.  Thanks!

  That's probably a somewhat unrealistic goal, if you want to test the same thing that gets checked into trunk.

Comment 34

[Mark Smith [:xb95]]

Attachment: job queue system w/TheSchwartz support

This is the third take.

The job queue system is now platform agnostic.  It is possible to add non-TheSchwartz job queue systems by implementing a new Bugzilla::JobQueue::SOMETHING module.  You are then responsible for building your own workers that patch the job back to the appropriate point in Bugzilla.

For TheSchwartz, it works like this:

* new param "use_mailer_queue" indicates that mails should be dispatched to the job queue

* theschwartz_worker.pl script (which now can daemonize) will pull jobs

* jobs are NOT handled in the workers, they are instead passed back to Bugzilla::Mailer::Queue

This design is fairly clean.  The implementation done for the job queue system is ONLY to actually interact with the queue software.  All of the work to do whatever task you're making async is done in Bugzilla itself.

Read through the patch file, it's fairly easy to follow.  Right now the only module is TheSchwartz, but it'd be easy for someone to add something else.  Could even tie it into a proprietary internal system (i.e. at big users) really easily.

Comment 35

[Max Kanat-Alexander]

Comment on attachment 299292 [details] [diff] [review]
job queue system w/TheSchwartz support

>Index: Bugzilla/Mailer.pm
> sub MessageToMTA {
>-    my ($msg) = (@_);
>+    my ($msg, $send_now) = (@_);

  I think instead of $send_now, we should have a $queue (that is, reverse the sense of it). Other than bugmails in process_bug, all mail should be sent instantly (such as, say, password reset mails and new account tokens, which don't have or cause a performance problem).

>Index: Bugzilla/Install/Localconfig.pm

  Hrm. Does TheSchwartz support SQLite? If it does, I was thinking that perhaps we could just make it use a SQLite database that we store in the data/ directory. Otherwise, can it just create tables in our current DB instead of having its own DB?

  Basically, I want to eliminate these configuration variables entirely, if possible.

>+  use_mailer_queue => "If enabled, mail is spooled to the Bugzilla job queue.  This requires " _
>+                      "you to have already configured the job queue system.  If you have not done " _
>+                      "this, or don't know what this means, then turning on this option is a bad " _
>+                      "idea and will cause your email to stop being sent." }

  Cool. The job queue system will have to be explained in the extraconfig section of the docs, I think, and we'll want a link there.

>+++ Bugzilla/Mailer/Queue.pm	2008-01-25 13:50:01.000000000 -0800
>+    eval {
>+        MessageToMTA($args{msg}, 1);
>+    };
>+
>+    return undef if $@;

  Hrm, so if something goes wrong, how does the admin figure out what's going wrong, or even that something is going wrong?

  We have a file called data/errorlog that you can write to if it exists. (And not write to if it doesn't exist.)

>+Bugzilla::Mailer::Queue - Worker module for sending emails

  You don't really have to POD this module, I think, if you don't want. It's pretty straightforward and I can't imagine why anybody would want to read about it in the API docs.

>--- /dev/null	2008-01-02 14:15:29.981474736 -0800
>+    # for now, just assume TheSchwartz job queue; this should be
>+    # expanded to support an options panel for selecting a job queueing
>+    # system ... but there's only one for now :-)

  Since that's the case, let's just eliminate Bugzilla::JobQueue for now. Since it's always accessed as Bugzilla->job_queue, people could still easily modify it if necessary.

>--- /dev/null	2008-01-02 14:15:29.981474736 -0800
>+sub _map_job_type {
>+    return {
>+        send_mail => 'Bugzilla::TheSchwartz::Mailer',

  Shouldn't that be Bugzilla::Mailer::Queue or whatever?

>--- /dev/null	2008-01-02 14:15:29.981474736 -0800
>+GetOptions( 'foreground' => \$opts{foreground},
>+            'debug' => \$opts{debug},
>+            'help' => \$opts{help}, ) or help();

  There's actually some way to just tell it to put all those in the hash--I use it in checksetup.pl

>+$::DEBUG = $opts{debug};

  Hrm, please declare this as "our $DEBUG".

>+    unless ref $jq eq 'Bugzilla::JobQueue::TheSchwartz' &&

  Might as well do $jq->isa.

>+# control never returns from here
>+print "[$$] Shuffling off to wait for work...\n"
>+    if $::DEBUG;

  Hrm, let's use something a bit more exact there, for people who don't know much about TheSchwartz.

>+sub help {

  Let's use Pod::Usage instead. (I do it in checksetup.)

>+sub daemonize {
>+    my ( $pid, $sess_id, $i );

  What's $i?

>+    print "[$$] Daemonizing...\n" if $opts{debug};
>+
>+    # Fork and exit parent
>+    if ($pid = fork) { exit 0; }

  Hrm, Windows doesn't really fork, though, right? It threads. Will that be a problem?

>+    # Detach ourselves from the terminal
>+    die "Cannot detach from controlling terminal"
>+        unless $sess_id = POSIX::setsid();

  Hrm, also, what's that do on Windows?

>+    # Prevent possibility of acquiring a controlling terminal
>+    $SIG{'HUP'} = 'IGNORE';

  Seems like HUP ought to restart us, though, right?

>+    # Change working directory
>+    chdir "/";

  Hrm, is that really a good idea? Maybe changing to $HOME (which doesn't work on Windows...sigh)?

>+    open(STDIN,  "+>/dev/null");

  That almost certainly won't work on Windows, or there could be a file called /dev/null.

>+    open(STDOUT, "+>&STDIN");
>+    open(STDERR, "+>&STDIN");

  Shouldn't we have a log somewhere?


>+package Bugzilla::TheSchwartz::Mailer;
>+    print "[$$] Bugzilla::TheSchwartz::Mailer() got job\n"
>+        if $::DEBUG;

  Hrm, maybe print out some info about the job, also?

>+sub grab_for { 300 }
>+sub keep_exit_status_for { 0 }
>+sub max_retries { 10 }
>+sub retry_delay { (10, 30, 60, 300, 600, 3600, 2*3600, 8*3600, 24*3600, 48*3600)[$_[1]] }

  Hrm, could you comment these magic numbers?


  Also, I think we should have a --pidfile argument that allows us to specify a place to store our PID and check if we're already running. That way people can run a cron job that restarts the daemon in case it's crashed (kind of like one does for IRC bots).

Comment 36

[Max Kanat-Alexander]

Oh, and by the way, when I refer to the data/ directory, you have to use bz_locations()->{'datadir'} to get it.

Comment 37

[Dave Miller [:justdave]]

Does TheSchwartz actually work on Windows?  Is there a reason we can't add high-performance functionality for Linux/Unix just because there's no equivalent on Windows?  If there is an equivalent on Windows I have a feeling it's going to be something else, and require a fork in the code path anyway.

Comment 38

[Mark Smith [:xb95]]

I can think of no reason for it NOT to, it doesn't actually do anything platform specific inside.  However, I am less convinced about Data::ObjectDriver, which is a dependency.  That would bear some looking into.

But in reality, it doesn't matter too much if Windows is not supported by TheSchwartz.  The implementation I did gives Bugzilla an agnostic approach, defining its own procedure for inserting a job and later processing them.  I someone were to write Bugzilla::JobQueue::WindowsSpecificModule then they could easily do so.

The only thing lacking at this point is a way to configure which job queueing system to use.  Presently it just instantiates TheSchwartz.  If someone does the leg work to implement an alternate solution, it would be easy to do so within the constraints of this suggested patch.

Comment 39

[Max Kanat-Alexander]

Having interacted with many, many Bugzilla-using organizations, I think it really does matter whether or not this works on Windows. I already dislike having to tell people that they can't use mod_perl on Windows. Though of course, since they can't, we could possibly always just say that Windows is not the high-performance side of things for Bugzilla, and leave it at that. I don't really want to go down that road, though, if we don't have to. (I'd even like our mod_perl support to work on Windows one day.)

If it's possible to get this working on Windows without modifying the CPAN dependencies, I'd like to do it. Also, being more platform-agnostic might help us on other non-Linux systems.

Comment 40

[:glob ✱]

i vote for leaving it in, but not presenting it as an option to windows users.

iis has a simple mail queuing facility that we could use on windows instead (bug 414466).

Comment 41

[Max Kanat-Alexander]

Hey Mark, I'm going to take this over and just do a few small revisions, if that's OK.

Comment 42

[Max Kanat-Alexander]

Attachment: New Work In Progress

Here's my current work in progress. It uses sqlite, eliminates the possibility of having other queueing systems (which greatly simplified the code), and automatically sets up the DB schema.

However, it's currently not working, because I get an error that apache can't access the sqlite DB file, when it clearly *can* access it. So I don't know what's up.

Comment 43

[Max Kanat-Alexander]

Attachment: v7

Okay, this one's working! The problem before was that sqlite has to be able to create a journal file in the data/queue/ directory, so the webserver needs write permissions to that directory.

This is pretty full-featured--in particular, the daemon has lots of nice features, like if you do "./jobqueue.pl check" it will tell you how many jobs are in the queue.

Comment 44

[Frédéric Buclin]

Comment on attachment 353873 [details] [diff] [review]
v7

>Index: Bugzilla/Config/MTA.pm
>   {
>+   name => 'use_mailer_queue',
>+   type => 'b',
>+   default => 0
>+  },

This parameter needs a checker. If there are missing Perl modules, prevent the param from being enabled.

Comment 45

[Max Kanat-Alexander]

Note that the current patch requires bug 470442 to be fixed first (or for the tester to apply that patch first).

Comment 46

[Max Kanat-Alexander]

(In reply to comment #44)
> This parameter needs a checker. If there are missing Perl modules, prevent the
> param from being enabled.

  Ah, yes, that's right! I was going to do that and forgot. :-)

Comment 47

[Yann]

Funny that you ended up doing a job queue. I implemented that on windows a few years ago by using Byron's fake sendmail:
1) one instance was on D drive and was writing emails to a directory (this is the one called by our Bugzilla), so that was instantaneous
2) another instance on C drive was called by a Windows scheduled task to process the directory as a queue and send emails through smtp.

Comment 48

[:glob ✱]

Comment on attachment 353873 [details] [diff] [review]
v7

this looks ok to me.

i initially had issues with sqlite and locking on write; however dbd::mysql implements a 30 second busy timeout which covers this.

i'd like to see admin pages [in the future] to view the queue and errors -- if email isn't working it should be obvious what the admin needs to do to see the error messages.

Comment 49

[Max Kanat-Alexander]

Nice. :-) I will implement the parameter checker in a patch after this, too.

Comment 50

[Max Kanat-Alexander]

You know, I've been thinking about this, and I think the only really good reason to use SQLite was that Data::ObjectDriver doesn't support Oracle.

But requiring SQLite adds complexity for people in the multiple-webhead situation, it means that we have to maintain a whole separate schema, it adds another module dependency to Bugzilla (and a particularly strange one, since the DB code itself is bundled into the package), and it may be performance-limiting since SQLite only allows one writer at a time.

Instead of all that complexity, let's just say, "Hey, we don't support Oracle with this unless somebody wants to write a Data::ObjectDriver::DBD::Oracle." (And even that probably wouldn't be all that hard, honestly--at least to get one that would work with TheSchwartz.)

I'm going to submit another patch that just uses the current Bugzilla database.

Comment 51

[Max Kanat-Alexander]

Attachment: v8

Okay, I made it use the normal DB instead. This removes a fair bit of code complexity, but I had to add a special "private_bz_dsn" attribute to the dbh handle, because there's no way to get the DSN of a database out of DBI after it's been created, and TheSchwartz can't re-use an existing DBI handle. (Anyway, we wouldn't want it to, I'm sure Data::ObjectDriver does crazy things with dbh handles.)

It doesn't work on Oracle as far as I know, because there is no Data::ObjectDriver for Oracle.

It also currently doesn't work in Pg, but that's just because of a bug in Data::ObjectDriver:

  http://rt.cpan.org/Public/Bug/Display.html?id=41880

Comment 52

[:glob ✱]

Comment on attachment 354214 [details] [diff] [review]
v8

i agree -- i'm much happier with this approach.

Comment 53

[Xiaoou]

I will try to write Data::ObjectDriver::DBD::Oracle then:-)

Comment 54

[Max Kanat-Alexander]

Woo! :-)

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.72; previous revision: 1.71
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/jobqueue.pl,v
done
Checking in jobqueue.pl;
/cvsroot/mozilla/webtools/bugzilla/jobqueue.pl,v  <--  jobqueue.pl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/JobQueue.pm,v
done
Checking in Bugzilla/JobQueue.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/JobQueue.pm,v  <--  JobQueue.pm
initial revision: 1.1
done
Checking in Bugzilla/Mailer.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Mailer.pm,v  <--  Mailer.pm
new revision: 1.26; previous revision: 1.25
done
Checking in Bugzilla/Config/Common.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Common.pm,v  <--  Common.pm
new revision: 1.24; previous revision: 1.23
done
Checking in Bugzilla/Config/MTA.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/MTA.pm,v  <--  MTA.pm
new revision: 1.17; previous revision: 1.16
done
Checking in Bugzilla/DB/Mysql.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Mysql.pm,v  <--  Mysql.pm
new revision: 1.71; previous revision: 1.70
done
Checking in Bugzilla/DB/Oracle.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Oracle.pm,v  <--  Oracle.pm
new revision: 1.20; previous revision: 1.19
done
Checking in Bugzilla/DB/Pg.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Pg.pm,v  <--  Pg.pm
new revision: 1.31; previous revision: 1.30
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.109; previous revision: 1.108
done
Checking in Bugzilla/Install/Filesystem.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Filesystem.pm,v  <--  Filesystem.pm
new revision: 1.32; previous revision: 1.31
done
Checking in Bugzilla/Install/Requirements.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Requirements.pm,v  <--  Requirements.pm
new revision: 1.54; previous revision: 1.53
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Job/Mailer.pm,v
done
Checking in Bugzilla/Job/Mailer.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Job/Mailer.pm,v  <--  Mailer.pm
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/JobQueue/Runner.pm,v
done
Checking in Bugzilla/JobQueue/Runner.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/JobQueue/Runner.pm,v  <--  Runner.pm
initial revision: 1.1
done
Checking in template/en/default/admin/params/mta.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/mta.html.tmpl,v  <--  mta.html.tmpl
new revision: 1.13; previous revision: 1.12
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.108; previous revision: 1.107
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.80; previous revision: 1.79
done

Comment 55

[Xiaoou]

Okay, I've filed a bug for Data::ObjectDriver Oracle driver,  see https://rt.cpan.org/Public/Bug/Display.html?id=41929 .