mooli tayer has posted comments on this change. Change subject: tools: notifier - simple first match include/exclude. ......................................................................
Patch Set 2: (25 comments) 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. This class was deleted. 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. Done 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. Done 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? because of the log on line 93. The alternative is writing it twice. 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. Done 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 I would agree but It apears in other places e.g eventsManager.getAuditLogEventSubscribers. I will make subscriber an inner class of FilterEntry though. 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? To express the notion that at this point we did nothing to verify that this is a valid transport. Line 17: this.address = address; Line 18: } Line 19: Line 20: public static FilterEntry createEntryFromRecipient( 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 plai This is the idiomatic way of doing things in public classes. There is a chapter about it in effective java. And this is what java programmers expect to find. 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 Why? String format is less readable and less efficient. 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. Exactly as I did In a previous patch set. 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. Done 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. Done 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? You now call this "name" (which I accept) while until now the term recipient was sometimes this (name) and some times a transport:name pair. that's why I used address. so to be clear, from now: recipient= transport+name. 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 p I would prefer to encapsulate the logic about creating an object inside the object. if there are different ways of creating objects it is better to provide static factory methods with clear names and not overriding constructors. (effective java item 1) 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 Done 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? I'm using java's Observable and observer instead of creating my own. They are not ideal. Extending Observable means that the observer class takes care of managing the actual observer lists. 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. OK. here it means adding another boolean e.g 'successful' yes? 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); > you should put the messages within queue and have background thread to send OK. will do in a different patch set. 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? See next comment. 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 While I made a mistake in duplicating the pattern, we should verify validity of filter and fail to start with a proper warning message if needed. Like we do with the reset of validation in this class. That means to parse during validate and make sure FILTER is valid except for transport element which we only check during runtime (I'm not really sure why) 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. It should be here or the next one. it started with requireOne(NotificationProperties.MAIL_SERVER); to prepare for requireOne(NotificationProperties.MAIL_SERVER, NotificationProperties.SNMP_MANAGERS); Since this patch prepares notifier to actually have more then one transport. 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 Yes I will remove. I found out that the smtp test was not really doing what it is supposed todo. I will move to a different patch. Line 15: MAIL_SERVER=smtp.redhat.com Line 16: MAIL_PORT=25 Line 17: MAIL_TO=dev-n...@redhat.com 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 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 will move. Line 14: MAIL_SERVER=smtp.gmail.com Line 15: MAIL_PORT=465 Line 16: MAIL_USER=mailtest.red...@gmail.com 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: MAIL_ADDRESS=wr...@example.com Line 23: MAIL_ADDRESS_SOMEONE=ri...@example.com > not sure what these are They will go away Line 24: Line 25: FILTER="${FILTER} include:VDC_START(smtp:b...@example.com) 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? Yes. since filter was introduces here, I took the time to re order the different parts of this configuration file to make sense. 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 <mta...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: mooli tayer <mta...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
