[Zope-dev] Re: Have you checked on the latest commits?
Hi! On Mon, 2008-04-07 at 20:02 +0300, Marius Gedminas wrote: I want to get teh branch to the point where it gets merged. Don't feel that my review is somehow necessary for it being merged. I tend to procrastinate. :-( Could you please tell me what has to happen to get the merge done? I believe there is some consensus that needs to be reached with the other zope.sendmail maintainers? Cheers and Thanks, Matthew signature.asc Description: This is a digitally signed message part ___ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )
[Zope-dev] Have you checked on the latest commits?
Dear Marius, I was just wondering when you would like to review my updates to zope.sendmail based on your review. I want to get teh branch to the point where it gets merged. Cheers, and many thanks for your time. It is really appreciated. Best Regards, Matthew Grant signature.asc Description: This is a digitally signed message part ___ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )
Re: [Zope-dev] Re: zope.sendmail grantma-retryfixes branch review
Chris, FYI - Test coverage in zope.sendmail was originally pretty thin. The work I have done is a big improvement. Zope.sendmail is a corner of Zope3 which has not been implemented that well, and this show up in the terms of its quality and sparse unit test coverage. Patience please! Cheers, Matthew On Tue, 2008-03-25 at 12:50 +, Chris Withers wrote: Matthew Grant wrote: Just done edits you suggested. Some Unit tests still to be written. If you wrote the tests first *AS YOU SHOULD* this wouldn't be a problem. Tests written after the case have a tendency to omit edge cases... Won't be doing one for that /1000 as tehre are more important things to check like exception functionality. eh? cheers, Chris signature.asc Description: This is a digitally signed message part ___ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )
[Zope-dev] Re: zope.sendmail grantma-retryfixes branch review
Working on this.
tehre are some things I will get back to you about, because what you
suggest breaks the unit test code...
Cheers,
Matthew
On Wed, 2008-03-19 at 23:32 +0200, Marius Gedminas wrote:
I'm starting a new thread, because the original one got hijacked by the
doctest/unittest flamefest.
I appear to have unsubscribed from zope3-checkins a while ago, which is
why I'm posting here.
Disclaimer: I'm not very good at doing code reviews. I tend to mention
every little detail that could've been improved and forget to praise
everything that's already perfect.
There are two checkins on the branch.
=
rev 84645
=
http://svn.zope.org/zope.sendmail/branches/grantma-retryfixes/?rev=84645view=rev
Matthew, Subversion thinks the two PDF files you added are plain-text
(svn:eol-style is native, and svn diff shows the actual contents of
the PDFs, which isn't very useful to human readers). I would suggest
svn propdel svn:eol-style *.pdf
svn propset svn:mime-type application/pdf *.pdf
Also, the Quue_*.pdf seems to have a typo in the file name.
At first sight those diagrams look scary and complicated. I'd look into
ways to express their content in a text file with plain-text
explanations and ASCII-art diagrams.
=
rev 84551
=
http://svn.zope.org/zope.sendmail/branches/grantma-retryfixes/?rev=84551view=rev
Index: src/zope/sendmail/configure.zcml
===
--- src/zope/sendmail/configure.zcml(revision 84550)
+++ src/zope/sendmail/configure.zcml(revision 84551)
@@ -14,11 +14,18 @@
!--
To send mail, uncomment the following directive and be sure to
-create the queue directory.
+create the queue directory.
+
+Other parameters sepcify the polling interval, and the retry
sepcify?
+interval used when a temporary failure is detected. Lock links
+can also be cleared on server start.
mail:queuedDelivery permission=zope.SendMail
queuePath=./queue
- mailer=smtp /
+ retryInterval=300
+ pollingInterval=3000
+ cleanLockLinks=False
+ mailer=smtp /
--
Please avoid literal tab characters in source files.
Index: src/zope/sendmail/zcml.py
===
--- src/zope/sendmail/zcml.py (revision 84550)
+++ src/zope/sendmail/zcml.py (revision 84551)
@@ -70,7 +70,35 @@ class IQueuedDeliveryDirective(IDelivery
description=uDefines the path for the queue directory.,
required=True)
-def queuedDelivery(_context, permission, queuePath, mailer, name=Mail):
+pollingInterval = Int(
+title=u'Polling Interval',
+description=u'Defines the polling interval for queue processing in'
+' milliseconds',
+default=3000,
+required=False)
...
-thread = QueueProcessorThread()
+thread = QueueProcessorThread(interval=pollingInterval/1000,
You're using integer division here and losing sub-second precision. If you
don't want that, write interval=pollingInterval/1000.0.
+ retry_interval=retryInterval,
+ clean_lock_links=cleanLockLinks)
thread.setMailer(mailerObject)
thread.setQueuePath(queuePath)
thread.start()
Index: src/zope/sendmail/mailer.py
===
--- src/zope/sendmail/mailer.py (revision 84550)
+++ src/zope/sendmail/mailer.py (revision 84551)
...
@@ -41,22 +49,105 @@ class SMTPMailer(object):
self.password = password
self.force_tls = force_tls
self.no_tls = no_tls
+self.logger = None
-def send(self, fromaddr, toaddrs, message):
-connection = self.smtp(self.hostname, str(self.port))
+def set_logger(self, logger):
+if not isinstance(logger, logging.Logger):
+raise DoesNotImplement('Invalid logger given')
I don't think DoesNotImplement is right here -- there are no interfaces
you're looking for. Better use the built-in TypeError.
But is the type check necessary at all? The Pythonic thing is to rely on duck
typing.
+self.logger = logger
+
+def _log(self, log_level, *args):
+if self.logger is None:
+return
+# This is not elegant, but it can be fixed later
In my experience later usually doesn't happen. Why not fix it now?
+if log_level == 'debug':
+self.logger.debug(*args)
+elif log_level =='info':
+self.logger.info(*args)
+elif log_level =='error':
+self.logger.error(*args)
+
Re: [Zope-dev] zope.sendmail Retry fixes and new state machine.
Hi! On Sun, 2008-03-09 at 20:54 -0400, Gary Poster wrote: I did have one somewhat trivial thought. I generally prefer durations and intervals expressed as datetime.timedeltas myself, because they convey their meaning without having to look it up docs (is that number value a number of seconds? milliseconds? minutes?). There might even be a zcml built in for schema field for that; I believe I remember that there is in ZConfig. Minor detail - yes it has warts - there are other things that will need doing to, its just time that this code was put out there. Also, some variety of doctest would be nice. Even when a package is not using doctests, I add new tests as doctest unless there's a really good reason not to. It was the only way I could debug and introspect the problems from the Unit tests - the python debugger does not run properly from the doctests, and the original test are all unit tests... In this case, it looks like you've made the code significantly more robust, which has added some probably necessary complexity. The code looks readable, but I recommend a maintainer-oriented overview/ introduction as a doctest, at the least. For instance, perhaps you could think about documentation about the rationale for the approach and about the dance that this code participates in (with the lock files and all the possible SMTP error conditions and the code's responses). Of course, even more friendly docs than that would be nice, but I'm only asking for what I myself tend to give, unfortunately. A little does need adding there - I have scanned state machine diagrams and the design rationale. I will go and put those in the checkin when I have a spare moment. Regards, Matthew signature.asc Description: This is a digitally signed message part ___ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )
[Zope-dev] zope.sendmail Retry fixes and new state machine.
Hi! Just created a new branch for zope.sendmail. This is a result of the state machine analysis work I did on it after finding that it was creating problems on sending mail to our smart host... svn+ssh://[EMAIL PROTECTED]/repos/main/zope.sendmail/branches/grantma-retryfixes with lots of juicy new fixes including: - Restructured SMTP mailer and QueueProcessorThread so that all SMTP error logic is in the mailer. Clears the way for another mailer for /usr/sbin/sendmail command line can be used with QueueProcessorThread. - Added ability for QueueProcessorThread so that it can handle temporary failures in delivery to its smart host - ie administrator reconfiguring mailserver, mail server reboot/restart This was the main error path that was missing from the trunk code's state machine. - Formatted log messages in a consistent fashion so that they can be grepped out of z3.log - Added maildir message filename to log messages as message id - allows easy analysis/triage of mail message sending problems - Added optional cleaning of lock links to QueueProcessorThread so that messages can be resent immediately on Zope3 restart. - Added pollingInterval (ms), cleanLockLinks (boolean), and retryInterval (seconds) configure options to configure.zcml. Comments welcomed. I know it is a major portion of code and a bit of a restructure, but I have created unit tests for all my major changes. It includes all the stuff that was missing and that was needed for our production server. I imagine that all of this will prove to be really useful when it is added to the trunk zope.sendmail Cheers, Matthew Grant signature.asc Description: This is a digitally signed message part ___ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )
[Zope-dev] Request for commit access for updating zope.sendmail
Hi! I have recently been doing a lot of work on zope.sendmail, and would like to commit a lot of fixes, namely for: 1) Handling of SMTP error 451 etc. 2) Cleaning stale lock links out of the queue 3) Handling Server disconnection 4) Handling of connect() failure The above are fixes for Bug #157104 https://bugs.launchpad.net/zope3/+bug/157104 5) A bit of a clean up QueueProcessorThread so that it complies with IMailQueueProcessor in interfaces.py This is a condition were Zope is restarted - there is a little bit of a race here with duplicate messages where if the zope.sendmail delivery thread exits for some reason after a successful SMTP delivery and has not yet removed the lock link, but that has such a small chance of happening - compared to all the other faults in zope.sendmail this is minor ... I would be willing to become the maintainer for zope.sendmail Best Regards, Matthew Grant signature.asc Description: This is a digitally signed message part ___ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )
[Zope-dev] Retry Fixes for zope.sendmail
Hi There! During the last few days I have been doing some fixes for zope.sendmail to do with the trouble we are having with it on our Zope 3 application, which actually sends a lot of emails. Problems I in the fix are: 1) Handling of SMTP error 451 etc. 2) Cleaning stale lock links out of the queue 3) Handling Server disconnection 4) Handling of connect() failure 5) A bit of a clean up QueueProcessorThread so that it complies with IMailQueueProcessor in interfaces.py Events 1, 3 and 4 are handled by waiting for 5 minutes, and then restarting processing. I would appreciate hearing from the zope.sendmail developers. Once I have the code complete I will gladly post a diff tomorrow. Best regards, Matthew Grant signature.asc Description: This is a digitally signed message part ___ Zope-Dev maillist - Zope-Dev@zope.org http://mail.zope.org/mailman/listinfo/zope-dev ** No cross posts or HTML encoding! ** (Related lists - http://mail.zope.org/mailman/listinfo/zope-announce http://mail.zope.org/mailman/listinfo/zope )
