Alon Bar-Lev has posted comments on this change. Change subject: tools: notifier - simple first match include/exclude. ......................................................................
Patch Set 2: (27 comments) Hi, thanks! much better, still I want some thing back to where I left them... Alon http://gerrit.ovirt.org/#/c/24018/2/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/EventFilter.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/EventFilter.java: Line 26: public String toString() { Line 27: return "EventFilter{" + Line 28: "include=" + include + Line 29: ", eventName='" + eventName + '\'' + Line 30: '}'; please use string.format. Line 31: } http://gerrit.ovirt.org/#/c/24018/2/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/NotificationService.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/NotificationService.java: Line 36: smtp.addObserver(this.eventsManager); Line 37: this.firstMatchSimpleFilter.registerTransport(smtp); Line 38: // SnmpTrap snmpTrap = new SnmpTrap(prop); Line 39: // snmpTrap.addObserver(this.eventsManager); Line 40: // firstMatchSimpleFilter.registerTransport(snmpTrap); please remove comments. Line 41: Line 42: markOldEventsAsProcessed(); Line 43: } Line 44: Line 61: firstMatchSimpleFilter.addFilterEntries(eventsManager.getAuditLogEventSubscribers()); Line 62: Line 63: // Read configurations subscription Line 64: firstMatchSimpleFilter.addFilterEntries( Line 65: FirstMatchSimpleFilter.parse(prop.getProperty(NotificationProperties.FILTER)) no need to parse every time... parse one time and add the array each time. Line 66: ); Line 67: Line 68: // Backward compatibility, aim to remove (can be replaced by "FILTER") Line 69: String dbDownSubscribers = Line 85: deleteObsoleteHistoryData(); Line 86: log.debug("Finished event notification service iteration"); Line 87: } catch (Throwable t) { Line 88: log.error("Exception while querying for notifications.", t); Line 89: if (t instanceof SQLException) { this is ugly! why don't you catch it instead? Line 90: distributeDbDownEvent(); Line 91: } Line 92: if (!Thread.interrupted()) { Line 93: log.error(String.format("Failed to run the service."), t); Line 101: Line 102: private void distributeDbDownEvent() { Line 103: firstMatchSimpleFilter.clearFilterEntries(); Line 104: firstMatchSimpleFilter.addFilterEntries( Line 105: FirstMatchSimpleFilter.parse(prop.getProperty(NotificationProperties.FILTER)) no nede to re-parse, use one parser result always. Line 106: ); Line 107: if (failedQueries == 0) { Line 108: try { Line 109: firstMatchSimpleFilter.processEvent(eventsManager.createDBDownEvent()); http://gerrit.ovirt.org/#/c/24018/2/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/filter/FilterEntry.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/filter/FilterEntry.java: Line 1: package org.ovirt.engine.core.notifier.filter; Line 2: Line 3: public class FilterEntry { this really can remain within the filter class, as no use without it. same principal as Map.Entry Line 4: Line 5: private final String eventName; Line 6: Line 7: private final boolean exclude; Line 12: Line 13: private FilterEntry(String eventName, boolean exclude, String transportCandidate, String address) { Line 14: this.eventName = eventName; Line 15: this.exclude = exclude; Line 16: this.transportCandidate = transportCandidate; why candidate? Line 17: this.address = address; Line 18: } Line 19: Line 20: public static FilterEntry createEntryFromRecipient( Line 16: this.transportCandidate = transportCandidate; Line 17: this.address = address; Line 18: } Line 19: Line 20: public static FilterEntry createEntryFromRecipient( I revised this in my last source I sent, please sync. Line 21: String eventName, Line 22: boolean exclude, Line 23: String recipient) { Line 24: if ("*".equals(eventName)) { Line 64: } Line 65: Line 66: public String getAddress() { Line 67: return address; Line 68: } no need for these getters and setters, it just add overhead, should be plain struct like structure. Line 69: Line 70: @Override Line 71: public String toString() { Line 72: return "FilterEntry{" + Line 74: ", exclude=" + exclude + Line 75: ", transportCandidate='" + transportCandidate + '\'' + Line 76: ", address='" + address + '\'' + Line 77: '}'; Line 78: } please use string.format http://gerrit.ovirt.org/#/c/24018/2/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/filter/FirstMatchSimpleFilter.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/filter/FirstMatchSimpleFilter.java: I sent you a new version please resync, Recipient is now a class. Line 1: package org.ovirt.engine.core.notifier.filter; Line 2: Line 3: import java.util.HashMap; Line 4: import java.util.HashSet; Line 44: } Line 45: } Line 46: Line 47: public void clearFilterEntries() { Line 48: recipients.clear(); you should clear also notify, see original source. Line 49: } Line 50: Line 51: public void processEvent(Event event) { Line 52: log.debug(String.format("Event: %s", event.getName())); Line 62: entry.getAddress().equals(recipient) Line 63: )) { Line 64: log.debug(String.format("Entry match((%s)): %s", entry.isExclude() ? "exclude" : "include", entry)); Line 65: if (!entry.isExclude()) { Line 66: Transport transport = transports.get(entry.getTransport()); this logic was modified in the last source I sent. Line 67: if (transport == null) { Line 68: log.debug(String.format("Ignoring recipient '%s' as transport not registered", recipient)); Line 69: } Line 70: else { Line 67: if (transport == null) { Line 68: log.debug(String.format("Ignoring recipient '%s' as transport not registered", recipient)); Line 69: } Line 70: else { Line 71: transport.dispatchEvent(event, entry.getAddress()); address? it is not address, why do you change terms? Line 72: } Line 73: } Line 74: break; Line 75: } Line 84: log.debug(String.format("parse: handling '%s'", m.group(0))); Line 85: boolean exclude = "exclude".equals(m.group("exclude")); Line 86: String event = m.group("event"); Line 87: String recipient = m.group("recipient"); Line 88: ret.add(FilterEntry.createEntryFromRecipient(event, exclude, recipient)); not sure why you removed '*' from the parsing, it belongs here... part of parsing, part of the format, has nothing to do with the java implementation. please put it back. Line 89: } Line 90: return ret; Line 91: } http://gerrit.ovirt.org/#/c/24018/2/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/transport/mail/EventMessageContent.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/transport/mail/EventMessageContent.java: Line 1: package org.ovirt.engine.core.notifier.transport.mail; please rename name space to mail->smtp Line 2: Line 3: import org.ovirt.engine.core.notifier.filter.AuditLogEvent; Line 4: Line 5: import java.util.Date; http://gerrit.ovirt.org/#/c/24018/2/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/transport/mail/SMTP.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/transport/mail/SMTP.java: Line 28: * <ul> Line 29: * <li>"from" address should include a domain, same as <code>MAIL_USER</code> property Line 30: * <li><code>MAIL_REPLY_TO</code> specifies "replyTo" address in outgoing message Line 31: */ Line 32: public class SMTP extends Observable implements Transport { why Observable is not an interface as well? Line 33: Line 34: private static final Logger log = Logger.getLogger(SMTP.class); Line 35: public static final int ATTEMPTS = 4; Line 36: private JavaMailSender mailSender; Line 84: try { Line 85: mailSender.send(address, message.getMessageSubject(), message.getMessageBody()); Line 86: setChanged(); Line 87: notifyObservers(DispatchData.success(event, address, EventNotificationMethod.EMAIL)); Line 88: return; please do not return in middle of function, use conditionals. Line 89: } catch (MessagingException ex) { Line 90: errorMessage = ex.getMessage(); Line 91: } Line 92: Line 90: errorMessage = ex.getMessage(); Line 91: } Line 92: Line 93: try { Line 94: Thread.sleep(30000); > This file used to be EventMailSender.java (is there any way to tell git tha you should put the messages within queue and have background thread to send these out. another alternative is to not use a thread, but add a method to Transport interface example idle(), that will be called by main once in a while to perform house keeping, such as re-send and such. Line 95: } catch (InterruptedException e) { Line 96: log.error("Failed to suspend thread for 30 seconds while trying to resend a mail message.", e); Line 97: } Line 98: attempts++; http://gerrit.ovirt.org/#/c/24018/2/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/utils/NotificationProperties.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/utils/NotificationProperties.java: Line 45: * Filter Line 46: */ Line 47: public static final String FILTER = "FILTER"; Line 48: public static final Pattern FILTER_ELEMENT_PATTERN = Pattern.compile( Line 49: "(?<include>include|exclude):(?<event>\\*|\\w+)(\\((?<method>snmp|smtp):((?<recipient>[^)]+))?\\))?"); why do you need pattern here? Line 50: Line 51: /** Line 52: * Email parameters Line 53: */ Line 224: if (!StringUtils.isEmpty(filter)) { Line 225: String[] filterElements = filter.split("\\s+"); Line 226: for (String filterElement : filterElements) { Line 227: if (!StringUtils.isEmpty(filterElement)) { Line 228: if (!FILTER_ELEMENT_PATTERN.matcher(filterElement).matches()) { this is not required, just parse using the provided class, nothing more, no duplication. Line 229: throw new IllegalArgumentException( Line 230: String.format("%s Illegal value for %s , (%s)", Line 231: GENERIC_MESSAGE, Line 232: FILTER, Line 292: propName), Line 293: ex); Line 294: } Line 295: } Line 296: } not sure why the above is part of this patch. Line 297: Line 298: /** Line 299: * Returns {@code true} if mail transport encryption type is correctly specified, otherwise {@code false} Line 300: */ http://gerrit.ovirt.org/#/c/24018/2/backend/manager/tools/src/test/resources/conf/notifier-mail-test-plain.conf File backend/manager/tools/src/test/resources/conf/notifier-mail-test-plain.conf: Line 10: Line 11: # oVirt lib location Line 12: engineLib=/usr/local/jboss-eap-5.1/jboss-as/server/default/deploy/engine.ear/lib Line 13: Line 14: HTML_MESSAGE_FORMAT=false I do not think this is related to this change Line 15: MAIL_SERVER=smtp.redhat.com Line 16: MAIL_PORT=25 Line 17: [email protected] Line 18: HTML_MESSAGE_FORMAT=true http://gerrit.ovirt.org/#/c/24018/2/backend/manager/tools/src/test/resources/conf/notifier-mail-test-secured.conf File backend/manager/tools/src/test/resources/conf/notifier-mail-test-secured.conf: Line 4 Line 5 Line 6 Line 7 Line 8 no related Line 9: Line 10: # oVirt lib location Line 11: engineLib=/usr/local/jboss-eap-5.1/jboss-as/server/default/deploy/engine.ear/lib Line 12: Line 13: HTML_MESSAGE_FORMAT=false I do not think this is related to this change Line 14: MAIL_SERVER=smtp.gmail.com Line 15: MAIL_PORT=465 Line 16: [email protected] Line 17: MAIL_PASSWORD="q1!w2@e3#!" http://gerrit.ovirt.org/#/c/24018/2/backend/manager/tools/src/test/resources/conf/notifier-prop-test.conf File backend/manager/tools/src/test/resources/conf/notifier-prop-test.conf: Line 19: # oVirt lib location Line 20: engineLib=/usr/local/jboss-eap-5.1/jboss-as/server/default/deploy/engine.ear/lib Line 21: Line 22: [email protected] Line 23: [email protected] not sure what these are Line 24: Line 25: FILTER="${FILTER} include:VDC_START(smtp:[email protected]) include:VDC_START" http://gerrit.ovirt.org/#/c/24018/2/packaging/services/ovirt-engine-notifier/ovirt-engine-notifier.conf.in File packaging/services/ovirt-engine-notifier/ovirt-engine-notifier.conf.in: Line 60: Line 61: # Comma separated list of email recipients to be informed in case Line 62: # the notification service cannot connect to the DB. can be empty. Line 63: # Deprecated, use FILTER with DATABASE_UNREACHABLE instead. Line 64: FAILED_QUERIES_NOTIFICATION_RECIPIENTS= is the above move related to this change? Line 65: Line 66: #--------------# Line 67: # Event Filter # Line 68: #--------------# -- To view, visit http://gerrit.ovirt.org/24018 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia0b94ee8665ff8030b36b463207e50beea44b47d Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: mooli tayer <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: mooli tayer <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
