Alon Bar-Lev has posted comments on this change. Change subject: tools: notifier - simple first match include/exclude. ......................................................................
Patch Set 3: (9 comments) 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 ;"); > I can simplify this to select only from audit_log now that we have eventMap the entire concept of ovirt is to use stored procedures to abstract database layer. 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 { > Classes should be internal when they are used only in the context of the wr on the opposite... it is the entry of the filter, just like Map.Entry or any other nested scope. please move it back. 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( > In the original code there are two public constructors and a private init m please remove the createXXXX. 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: } > I still don't see why recipient.split(":", 2) is inside the object and *.eq because the format of recipient is name and transport. if you like, remove the split and the function that gets the string. 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 76: } Line 77: } Line 78: } Line 79: Line 80: public static List<FilterEntry> parse(String filters) { > Passing Lists around and not arrays is considered a better practice unless it is not better practive for something that should not be modified, as in this case the parsing result, which could be also stored as reference within the instance, modifying it will cause modification of state which is invalid. 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)); > Please see FilterEntry I saw, and disagree. 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: } > See filter entry I saw and disagree. 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); > Null checks where missing. no need for check null if you use stringutils. 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/utils/NotificationProperties.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/utils/NotificationProperties.java: Line 237: String.format("%s Illegal value for %s , (%s)", Line 238: GENERIC_MESSAGE, Line 239: FILTER, Line 240: filterElement)); Line 241: } > I see. non existing transports should not be validated in my opinion, if you want to do that, just iterate on the parsing result. syntax issues should be handled by the parse method, I suggest *NOT DO IT ANY MORE COMPLEX* and leave it as-is. Line 242: } Line 243: } Line 244: } Line 245: } -- 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
