Alon Bar-Lev has posted comments on this change. Change subject: tools: notifier - simple first match include/exclude. ......................................................................
Patch Set 3: (18 comments) well, I am very tired. you change code by the name of some java god I do not believe in... please stop doing that. http://gerrit.ovirt.org/#/c/24018/3/.install-dev-last.txt File .install-dev-last.txt: ???? 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 exclude * 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? 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. 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. 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. 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!!! please revert. 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. please revert. 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 for, once again the '*' handling is part of parsing, and should be here. 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? what's wrong with constructor? use createXXX if constructor is private for some [valid] reason, otherwise use constructors. 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 it was this and that), which are way better than implicit. anyway.... please use StringUtils.equals for much better readability. 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. 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 move this [in different patch] to configuration as it should be, or remove the above comment. 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 LocalConfig which gets defaults from the default file, it can be used easily. 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 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 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 private and should not be used. only parse should be used per the interface. exception from parse should be considered as invalid, all other should be good. if you need more syntax validation, the parse method can be improved, anyway the location of doing that is not here. 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 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
