mooli tayer has uploaded a new change for review. Change subject: tools: implement and use transport.idle(). ......................................................................
tools: implement and use transport.idle(). As part of transitioning to more then one Transport, It is no longer possible to let a transport sleep and retry for as long as it wants. that might starve other transports especially in a short notification interval. Retrying transports (those that can detect sending failures) Should implement a queue and attempt to send it on idle. This patch also fixes a bug where smtp loging is done only before the first send attempt. Change-Id: I8c4bafb542d28cb584e0751446d3e327f93e8112 Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1051492 Signed-off-by: Mooli Tayer <mta...@redhat.com> --- M backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/NotificationService.java M backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/transport/Transport.java M backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/transport/smtp/Smtp.java M backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/utils/NotificationProperties.java M packaging/services/ovirt-engine-notifier/ovirt-engine-notifier.conf.in 5 files changed, 120 insertions(+), 35 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/72/24472/1 diff --git a/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/NotificationService.java b/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/NotificationService.java index 4109c1b..1b41512 100644 --- a/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/NotificationService.java +++ b/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/NotificationService.java @@ -1,6 +1,8 @@ package org.ovirt.engine.core.notifier; import java.sql.SQLException; +import java.util.Date; +import java.util.LinkedList; import java.util.List; import org.apache.commons.lang.StringUtils; @@ -9,6 +11,7 @@ import org.ovirt.engine.core.notifier.dao.EventsManager; import org.ovirt.engine.core.notifier.filter.AuditLogEvent; import org.ovirt.engine.core.notifier.filter.FirstMatchSimpleFilter; +import org.ovirt.engine.core.notifier.transport.Transport; import org.ovirt.engine.core.notifier.transport.smtp.Smtp; import org.ovirt.engine.core.notifier.utils.NotificationProperties; @@ -28,7 +31,11 @@ private List<FirstMatchSimpleFilter.FilterEntry> configurationFilters; + private final List<Transport> transports = new LinkedList<>(); + private int failedQueries = 0; + + private long lastIdle = 0; public NotificationService(NotificationProperties prop) throws NotificationServiceException { this.prop = prop; @@ -36,10 +43,16 @@ this.firstMatchSimpleFilter = new FirstMatchSimpleFilter(); Smtp smtp = new Smtp(prop); smtp.addObserver(this.eventsManager); + addTransport(smtp); this.firstMatchSimpleFilter.registerTransport(smtp); configurationFilters = FirstMatchSimpleFilter.parse(prop.getProperty(NotificationProperties.FILTER)); markOldEventsAsProcessed(); } + + private void addTransport(Transport transport) { + this.firstMatchSimpleFilter.registerTransport(transport); + this.transports.add(transport); + } private void markOldEventsAsProcessed() { eventsManager.markOldEventsAsProcessed(prop.getInteger(NotificationProperties.DAYS_TO_SEND_ON_STARTUP)); @@ -50,10 +63,25 @@ */ @Override public void run() { + log.debug("Start event notification service iteration"); + processEvents(); + long now = new Date(0).getTime(); + if (now - lastIdle >= getIdleInterval()) + for (Transport transport : transports) { + log.debug(String.format("Transport %s is idle", transport.getName())); + transport.idle(); + } + lastIdle = now; + log.debug("Finished event notification service iteration"); + } + + private long getIdleInterval() { + return Integer.parseInt(NotificationProperties.IDLE_INTERVAL) * 1000; + } + + private void processEvents() { try { try { - log.debug("Start event notification service iteration"); - // Clear filter chain firstMatchSimpleFilter.clearFilterEntries(); @@ -84,7 +112,6 @@ eventsManager.updateAuditLogEventProcessed(event.getId()); } deleteObsoleteHistoryData(); - log.debug("Finished event notification service iteration"); } catch (SQLException se) { distributeDbDownEvent(); throw se; @@ -92,6 +119,7 @@ } catch (Throwable t) { log.error(String.format("Failed to run the service."), t); } + } private void deleteObsoleteHistoryData() throws SQLException { diff --git a/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/transport/Transport.java b/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/transport/Transport.java index dae285c..cebc592 100644 --- a/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/transport/Transport.java +++ b/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/transport/Transport.java @@ -19,4 +19,9 @@ * an address understood by this transport */ void dispatchEvent(Event event, String address); + + /** + * Upon an idle call a transport performs background tasks if needed, + */ + void idle(); } diff --git a/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/transport/smtp/Smtp.java b/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/transport/smtp/Smtp.java index ce4c44b..be7bbf4 100644 --- a/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/transport/smtp/Smtp.java +++ b/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/transport/smtp/Smtp.java @@ -2,7 +2,9 @@ import java.net.InetAddress; import java.net.UnknownHostException; +import java.util.LinkedList; import java.util.Observable; +import java.util.Queue; import javax.mail.MessagingException; @@ -16,32 +18,36 @@ import org.ovirt.engine.core.notifier.utils.NotificationProperties; /** - * The class sends e-mails to event subscribers. - * In order to define a proper mail client, the following properties should be provided: - * <li><code>MAIL_SERVER</code> mail server name - * <li><code>MAIL_PORT</code> mail server port</li><br> + * The class sends e-mails to event subscribers. In order to define a proper mail client, the following properties + * should be provided: <li><code>MAIL_SERVER</code> mail server name <li><code>MAIL_PORT</code> mail server port</li><br> * The following properties are optional: <br> - * <li><code>MAIL_USER</code> user name includes a domain (e.g. u...@test.com)</li> - * <li><code>MAIL_PASSWORD</code> user's password</li> - * <ul>if failed to obtain or uses "localhost" if <code>MAIL_MACHINE_NAME</code> not provided</li> - * <li><code>MAIL_FROM</code> specifies "from" address in sent message, or uses value of property <code>MAIL_USER</code> if not provided</li> - * <ul><li>"from" address should include a domain, same as <code>MAIL_USER</code> property + * <li><code>MAIL_USER</code> user name includes a domain (e.g. u...@test.com)</li> <li><code>MAIL_PASSWORD</code> + * user's password</li> + * <ul> + * if failed to obtain or uses "localhost" if <code>MAIL_MACHINE_NAME</code> not provided</li> + * <li><code>MAIL_FROM</code> specifies "from" address in sent message, or uses value of property <code>MAIL_USER</code> + * if not provided</li> + * <ul> + * <li>"from" address should include a domain, same as <code>MAIL_USER</code> property * <li><code>MAIL_REPLY_TO</code> specifies "replyTo" address in outgoing message */ public class Smtp extends Observable implements Transport { private static final Logger log = Logger.getLogger(Smtp.class); - public static final int ATTEMPTS = 4; + private final Queue<DispatchAttempt> sendQueue = new LinkedList<>(); private JavaMailSender mailSender; private String hostName; private boolean isBodyHtml = false; + private int maxRetries; public Smtp(NotificationProperties mailProp) { mailSender = new JavaMailSender(mailProp); String isBodyHtmlStr = mailProp.getProperty(NotificationProperties.HTML_MESSAGE_FORMAT); + if (StringUtils.isNotEmpty(isBodyHtmlStr)) { isBodyHtml = Boolean.valueOf(isBodyHtmlStr); } + maxRetries = mailProp.getInteger("MAIL_RETRIES"); try { hostName = InetAddress.getLocalHost().getHostName(); } catch (UnknownHostException e) { @@ -57,6 +63,15 @@ @Override public void dispatchEvent(Event event, String address) { + sendQueue.add(new DispatchAttempt(event, address)); + } + + @Override + public void idle() { + DispatchAttempt dispatchAttempt = sendQueue.remove(); + Event event = dispatchAttempt.getEvent(); + String address = dispatchAttempt.getAddress(); + // throw an exception if not AuditLogEvent AuditLogEvent auditLogEvent; auditLogEvent = (AuditLogEvent) event; @@ -75,33 +90,58 @@ log.debug(String.format("body:%n [%s]", message.getMessageBody())); } - String errorMessage = null; - int attempts = 0; boolean success = false; - while (!success && attempts < ATTEMPTS) { - try { - mailSender.send(address, message.getMessageSubject(), message.getMessageBody()); - setChanged(); - notifyObservers(DispatchData.success(event, address, EventNotificationMethod.EMAIL)); - success = true; - } catch (MessagingException ex) { - errorMessage = ex.getMessage(); - } - - if (!success) { - try { - Thread.sleep(30000); - } catch (InterruptedException e) { - log.error("Failed to suspend thread for 30 seconds while trying to resend a mail message.", e); - } - attempts++; - } + try { + mailSender.send(address, message.getMessageSubject(), message.getMessageBody()); + setChanged(); + notifyObservers(DispatchData.success(event, address, EventNotificationMethod.EMAIL)); + success = true; + } catch (MessagingException ex) { + errorMessage = ex.getMessage(); } + dispatchAttempt.setAttempts(dispatchAttempt.getAttempts() + 1); // Could not send after ATTEMPTS attempts. if (!success) { - setChanged(); - notifyObservers(DispatchData.failure(event, address, EventNotificationMethod.EMAIL, errorMessage)); + if (dispatchAttempt.getAttempts() < maxRetries) { + sendQueue.add(dispatchAttempt); + } else { // fail + setChanged(); + notifyObservers(DispatchData.failure(event, address, EventNotificationMethod.EMAIL, errorMessage)); + } + + } + + } + + private class DispatchAttempt { + + private final Event event; + + private final String address; + + private int attempts; + + private DispatchAttempt(Event event, String address) { + this.event = event; + this.address = address; + this.attempts = 0; + } + + public Event getEvent() { + return event; + } + + public String getAddress() { + return address; + } + + public int getAttempts() { + return attempts; + } + + public void setAttempts(int attempts) { + this.attempts = attempts; } } } diff --git a/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/utils/NotificationProperties.java b/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/utils/NotificationProperties.java index 2ec89f7..a714bcd 100644 --- a/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/utils/NotificationProperties.java +++ b/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/utils/NotificationProperties.java @@ -26,6 +26,13 @@ public static final String SSL_IGNORE_HOST_VERIFICATION = "SSL_IGNORE_HOST_VERIFICATION"; /** + * Wait at least IDLE_INTERVAL seconds before allowing a Transport to become idle. + * An idle transport preforms background tasks like sending unsent queue. + */ + public static final String IDLE_INTERVAL = "IDLE_INTERVAL"; + + + /** * Comma separated list of recipients to be informed in case the notification service cannot connect to the DB. can * be empty. */ @@ -76,6 +83,8 @@ */ public static final String MAIL_SMTP_ENCRYPTION_TLS = "tls"; + public static final String MAIL_RETRIES = "MAIL_RETRIES"; + private static final String GENERIC_MESSAGE = "Check configuration file, "; // Default files for defaults and overridden values: diff --git a/packaging/services/ovirt-engine-notifier/ovirt-engine-notifier.conf.in b/packaging/services/ovirt-engine-notifier/ovirt-engine-notifier.conf.in index 8f3f3eb..f619eb3 100644 --- a/packaging/services/ovirt-engine-notifier/ovirt-engine-notifier.conf.in +++ b/packaging/services/ovirt-engine-notifier/ovirt-engine-notifier.conf.in @@ -112,6 +112,9 @@ # Specifies 'reply-to' address on sent mail in RFC822 format. MAIL_REPLY_TO= +# try before marking message as failed +MAIL_RETRIES=3 + # This parameter specifies how many days of old events are processed and sent # when the notifier starts. If set to 2, for example, the notifier will # process and send the events of the last two days, older events will just -- To view, visit http://gerrit.ovirt.org/24472 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I8c4bafb542d28cb584e0751446d3e327f93e8112 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: mooli tayer <mta...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
