|
[
Permlink
| « Hide
]
Norman Maurer added a comment - 05/May/06 04:27 PM
I notice when working on spool management commands that the RemoteDelivery mailet throws an Exception if the message_error was set to "0" and the message_state is "error" . I wrote a patch to let it try to deliver the email immediately if the error_message is "0" and the message_state is "error". So it will throw no exception and set it to "1" if the retry fails.
This patch add 3 new features to RemoteManager.
1. List all emails (key,sender,recipient) in a SpoolRepository which have the error_state. 2. Try to resend all emails in a SpoolRepository immediately ( kick the queue) or try to send one email geven the key as argument! This feature needs the previour attached RemoteDelivery patch! 3. Delete all messages from the SpoolRepository or delete one email that is assigned to the given key Any of you guys see drawbacks? Maybe we should refactor the RemoteHandler code first to use a HandlerChain ?
Yes, I would like to refactor the RemoteHandler to use the command pattern like we did for SMTPHandler.
This would also give us more commands to look at when trying to refactor the CmdHandler stuff. Unfortunately the RemoteHandler has slight differences from SMTP/POP3 and NNTP handler that make it more difficult to port to the new pattern, but I plan to have a look at this in a week. Just commit the RemoteDelivery-fix.patch and add a bugreport
My refactoring is work in progress and will take much longer than I thought. I'm currently not happy with the HandlerChain pattern applied to the RemoteDelivery so I want to play with it a bit more.
Please feel free to apply your new commands to the trunk. Stefano need more time.. so i applied it
Sorry, I'm late with this comment but since we are practicing Commit-Then-Review...
While being absolutly +1 on these new features, I oppose bulk copy-pastes as a coding technique. So I propose the following patch for RemoteHandler. (Refactored under happy assistance of IntelliJ IDEA... but I know Eclipse would surely do the same... ;-) ) This make sense at all. I will not commit the changes cause you have also a commiter access. So feel free to commit the changes.. If you ant me to commit the patch just give me a sign ;-)
After review and test your patch i notice that with your patch it was not possible to delete mails from spool by key. I fixed it, add javadocs and commited it.
The problem was in the removeMail() method. Bernd plz review. Did not commit the changes myself because I could not test them and thought it was not fair to break something only for the beauty of the code ;-)
I'm curious, how do you test these features? In particular, how do you set the ERROR state? Regarding your latest commit: a. You seem to have changed the behavior of the DELETESPOOL command. Before the latest commit, every mail could be deleted if you gave the key. Now, only ERROR mails can be removed. I see this is now aligned with the other commands, but why do we restrict to ERROR mails? b. I ask myself, if it is neccessary to put the .unlock() calls in finally blocks? In cases where messages are not in error state, only lock() is called, but no matching unlock(). c. should we move remove[Error]Mail() & resendErrorMail() to the SpoolRepository or some utility class? Please tell me if I'm nitpicking... ;-) I test it by set configure a gateway which was not accept connections. So the emails resist in the outgoing spool with error state.
a. Maybe im blind but when im not wrong no email could be removed with the given key with your patch. Or do you mean latest commit from me ? i consider to list only mails with ERROR State cause other mails will me send by itself at soon as possible. I also want to get sure that this feature will only be used on spool repositories. b.Good questions at all . Maybe Stefano can give us a answer ;-) c.I whould it keep in the RemoteHandler. 1) I see this code:
m.setErrorMessage(0 + ""); m.setLastUpdated(new Date()); Imho the best way is to set the error message to error message -1, and NOT updated the lastupdated. This will add exactly 1 retry (done now) to the queued mail, otherwise you restart the queuing from the beginning. 2) Yes, if you lock you MUST also unlock (or delete), so: if (m.getState().equals(Mail.ERROR)) { .... } else { spoolRepository.unlock(key); } 3) The delete already unlocks: spoolRepository.remove(key); spoolRepository.unlock(key); synchronized (spoolRepository) { spoolRepository.notify(); } The unlock method could be removed. Furthermore the notify should not be called: the notify must be called only when you know you applied changes that will create new work for the spooling threads: deleting a message does not need a spool thread to do anything, so we don't need to notify it. 1) Thats not right i tested it it will not start try to delivery it now. It will pull down the delay. But thats not really what i want. With pulldown the delay it could take ages for the next retry too. Only set it to 0 will try to delivery it now
2) So the behaviour is correct 3) changed here. Will commit it later. 1) No, the problem is you hit another bug: when using the *file* repositories the lastUpdated is updated at each store because the writeTo method of the MailImpl object updates that field. So using db repositories it should work like I described, while the lastupdated thing should be fixed because now we have a consistency problem (we should add a JIRA issue for the lastUpdated attribute updated in the write method of the MailImpl object).
2) NO, the current behaviour is not correct: you lock a mail, retrieve it, it if is in ERROR state you store and unlock it, otherwise you currently don't unlock it.
1) I test it with db and it not work! I agree for add this bugreport. plz can you do it ?
2) Changed. I overseen it 1) Can you please check it again being sure that:
a. you use db and not file or dbfile b. you don't update the lastupdated field. Imho this should work, otherwise we have further bugs and I would like to know if we have further bugs. just checked to get sure..
Before FLUSHSPOOL: mysql> select message_name,message_state,error_message,last_updated from spool; +---------------------+---------------+---------------+---------------------+ | message_name | message_state | error_message | last_updated | +---------------------+---------------+---------------+---------------------+ | Mail1147772661914-0 | error | 4 | 2006-05-16 12:59:23 | +---------------------+---------------+---------------+---------------------+ 1 row in set (0.00 sec) After: mysql> select message_name,message_state,error_message,last_updated from spool; +---------------------+---------------+---------------+---------------------+ | message_name | message_state | error_message | last_updated | +---------------------+---------------+---------------+---------------------+ | Mail1147772661914-0 | error | 3 | 2006-05-16 12:59:23 | +---------------------+---------------+---------------+---------------------+ 1 row in set (0.00 sec) No retry to see from logs: 6/05/06 12:59:23 INFO James.Mailet: RemoteDelivery: Could not connect to SMTP host: 213.157.26.172, port: 2525 16/05/06 12:59:23 INFO James.Mailet: RemoteDelivery: Temporary exception delive ring mail (Mail1147772661914-0: 16/05/06 12:59:23 INFO James.Mailet: RemoteDelivery: Storing message Mail114777 2661914-0 into outgoing after 3 retries root@debbox:/tmp/james-2.3-dev/bin# date Tue May 16 13:08:35 CEST 2006 Right, I understand it now! thank you for the in depth informations!
error_message should be decreased by 1 and last_updated should be decreased enough so that "last_updated + current attempt retry time < now". I'm still not sure that resetting the error_message to 0 is good. What about decreasing error_message by 1 and put 0 in the last updated (or decrease it by 1 day or something similar) I'm not even sure we should decrease the error_message at all: if an administrator keeps "flushing" the queue the temporary errors will remain in the spool forever and the number of attempts will be too high! now you got it.
1. Maybe we should reset the error message to 0 and regenerate the old error_message with the delays in the RemoteDelivery? 2. If we whould change the last_update to 0 the email whould be permanent tried to deliver .. im right ? 3. I whould decrese the error_message cause this retries are manually and have nothing todo with the maxattemp. all by all i must think about it a bit more. 1. to do so you should store the old error_message somewhere.. maybe as mail attributes.
2. no: after the first attempt, if it results in a temporary failure the lastUpdated is updated, otherwise it is removed from the spool because it is a permanent success or failure. 3. I tend to think that this command is used to anticipate the next attempt to now: if I call the flush 5 times in 5 minutes I did 5 attempts and should be counted in the attempts calculation (I know this has pros and cons, but it is importat we understand them). If we reset the counter to 0 then the command should be called "requeue" because it resets the current queue and restart a new queue. ---- Evaluating the above my *current* idea is that the best would be to fix Here is a next solution wich work. What you guys think about it ?
1) See the patch i posted again
2) I see this is also maybe a solution 3) In the current new patch i did like you say. But im not sure if we should raise the count. Plz someone give suggestions Yes we should fix the I don't like the last patch: I think this is more an hack than resetting the last_updated.
Let's say I don't know james internals and I look at the db, before and after the flush, imho it is easier to understand that a flush change the last_updated field to 0 than understand what does it means that every error_message has now a minus in front. Furthermore this could create problems "------error_message" if you call the flush method multiple times in a sequence. It works by now but we should reopen it and wait for
Stefano fixed the MailImpl. So we can now set the last_update field for flushing the mail.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
New Feature
Resolved
Major
Spool managing commands for remote manager
