mooli tayer has posted comments on this change. Change subject: tools: notifier - simple first match include/exclude. ......................................................................
Patch Set 3: (18 comments) Thou shalt not take the name of thy java Lord in vain. http://gerrit.ovirt.org/#/c/24018/3/.install-dev-last.txt File .install-dev-last.txt: > ???? Removing and adding to global gitignore Line 1: Line 2: sudo su - postgres -c 'psql -d template1 -c "create database review_mooli_tayer_snmp_notifier owner engine template template0 encoding 'UTF8' lc_collate 'en_US.UTF-8' lc_ctype 'en_US.UTF-8';"' http://gerrit.ovirt.org/#/c/24018/3/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 76: false, Line 77: EventNotificationMethod.EMAIL.name(), Line 78: subscriber); Line 79: } Line 80: } > this should go before configurationFilters to not be filtered by the exclud Done Line 81: Line 82: for (AuditLogEvent event : eventsManager.getAuditLogEvents()) { Line 83: firstMatchSimpleFilter.processEvent(event); Line 84: eventsManager.updateAuditLogEventProcessed(event.getId()); http://gerrit.ovirt.org/#/c/24018/3/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/dao/EventsManager.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/dao/EventsManager.java: Line 127: " al.storage_domain_id, al.storage_domain_name, " + Line 128: " al.log_time, al.severity, al.message " + Line 129: "FROM audit_log al " + Line 130: "LEFT JOIN event_map em_down ON al.log_type_name = em_down.event_down_name " + Line 131: "WHERE al.processed = FALSE ;"); > this really need to be stp, eli? I can simplify this to select only from audit_log now that we have eventMap. then we don't need a stored procedure. Line 132: Line 133: rs = ps.executeQuery(); Line 134: while (rs.next()) { Line 135: auditLogEvents.add(extractAuditLogEvent(rs)); http://gerrit.ovirt.org/#/c/24018/3/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 { > please wrap it into the filter class. Classes should be internal when they are used only in the context of the wrapping class. FilterEntry is used in EventsManager. that makes me do FirstMatchSimpleFilter.FilterEntry which is ugly. Line 4: Line 5: private final String eventName; Line 6: Line 7: private final boolean exclude; Line 13: this.exclude = exclude; Line 14: this.recipient = recipient; Line 15: } Line 16: Line 17: public static FilterEntry createEntry( > no need to use createEntry, please use constructor. In the original code there are two public constructors and a private init method that does: this.property=property. that code is exactly what should be in a constructor. Line 18: String eventName, Line 19: boolean exclude, Line 20: String recipient) { Line 21: if ("*".equals(eventName)) { Line 22: eventName = null; Line 23: } Line 24: if ("*".equals(recipient)) { Line 25: recipient = null; Line 26: } > the '*' are part of parsing, null specify all, please revert to my code. I still don't see why recipient.split(":", 2) is inside the object and *.equals is out Line 27: Recipient concreteRecipient = null; Line 28: if (recipient != null) { Line 29: concreteRecipient = Recipient.createRecipient(recipient); Line 30: http://gerrit.ovirt.org/#/c/24018/3/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: Line 13: import org.ovirt.engine.core.notifier.transport.Transport; Line 14: Line 15: public class FirstMatchSimpleFilter { Line 16: Line 17: public static final Pattern PARSE_PATTERN = Pattern.compile( > THIS IS NOT PUBLIC!!! See comment in NotificationProperties.java Line 18: "(?<exclude>include|exclude):(?<event>\\*|\\w+)(\\((?<recipient>[^)]*)\\))?" Line 19: ); Line 20: private static final Logger log = Logger.getLogger(FirstMatchSimpleFilter.class); Line 21: private Map<String, Transport> transports = new HashMap<>(); Line 76: } Line 77: } Line 78: } Line 79: Line 80: public static List<FilterEntry> parse(String filters) { > why have you changed to list? this is static array once parse. Passing Lists around and not arrays is considered a better practice unless there is a reason to do so. Line 81: List<FilterEntry> ret = new LinkedList<>(); Line 82: Matcher m = PARSE_PATTERN.matcher(filters); Line 83: while (m.find()) { Line 84: log.debug(String.format("parse: handling '%s'", m.group(0))); 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.createEntry(event, exclude, recipient)); > still the '*' handling is not here... not sure what I should have waited fo Please see FilterEntry Line 89: } Line 90: return ret; Line 91: } http://gerrit.ovirt.org/#/c/24018/3/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/filter/Recipient.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/filter/Recipient.java: Line 12: } Line 13: Line 14: public static Recipient createRecipient(String transport, String name){ Line 15: return new Recipient(transport, name); Line 16: } > why do you keep changing the code I provide? See filter entry Line 17: Line 18: public static Recipient createRecipient(String recipient) { Line 19: String s[] = recipient.split(":", 2); Line 20: if (s.length != 2) { Line 45: } Line 46: Recipient recipient = (Recipient) o; Line 47: return (name != null ? name.equals(recipient.name) : recipient.name == null) && Line 48: (transport != null ? transport.equals(recipient.transport) : Line 49: recipient.transport == null); > I do not understand why you insist of changing variables name (in my case i Null checks where missing. will change to what you did before + StringUtils.equals. Line 50: Line 51: } Line 52: Line 53: @Override http://gerrit.ovirt.org/#/c/24018/3/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/transport/smtp/Smtp.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/transport/smtp/Smtp.java: Line 24 Line 25 Line 26 Line 27 Line 28 > changes in this comment do not belong to this patch. Removed. Line 76: log.debug(String.format("body:%n [%s]", Line 77: message.getMessageBody())); Line 78: } Line 79: Line 80: // Attempt 4 times > you added a constant and then specify its value in comment... either you mo Removed. Line 81: String errorMessage = null; Line 82: int attempts = 0; Line 83: boolean success = false; Line 84: while (!success && attempts < ATTEMPTS) { Line 92: } Line 93: Line 94: if (!success) { Line 95: try { Line 96: Thread.sleep(30000); > this should also go to configuration, never have anything static, we have L Not part of this patch. Will revisit with the thread issue. Line 97: } catch (InterruptedException e) { Line 98: log.error("Failed to suspend thread for 30 seconds while trying to resend a mail message.", e); Line 99: } Line 100: attempts++; http://gerrit.ovirt.org/#/c/24018/3/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 182 Line 183 Line 184 Line 185 Line 186 > not related to this patch Removing Line 120: Line 121: /** Line 122: * Validates properties values. Line 123: * Line 124: * @throws IllegalArgumentException if some properties has invalid values > not related to this patch Removing Line 125: */ Line 126: public void validate() { Line 127: validateCommon(); Line 128: Line 237: String.format("%s Illegal value for %s , (%s)", Line 238: GENERIC_MESSAGE, Line 239: FILTER, Line 240: filterElement)); Line 241: } > you should not validate the content only the format, the PARSE_PATTER is pr I see. What is missing in my opinion is validating that FILTER elements actually match the pattern (currently FILTER="kuku" passes) and also optionally to make sure we do not have include:x(*)(Include with *) So To make these changes inside parse? Line 242: } Line 243: } Line 244: } Line 245: } http://gerrit.ovirt.org/#/c/24018/3/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= > these should be removed from this patch Will move to a new patch 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: 3 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
