Alon Bar-Lev has posted comments on this change. Change subject: tools: notifier - simple first match include/exclude. ......................................................................
Patch Set 2: (11 comments) 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 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) { > because of the log on line 93. The alternative is writing it twice. or: try { try { } catch() { } } catch() { } or... have common functions. Line 90: distributeDbDownEvent(); Line 91: } Line 92: if (!Thread.interrupted()) { Line 93: log.error(String.format("Failed to run the service."), t); 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 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; > To express the notion that at this point we did nothing to verify that this it is the transport of the recipient... for the recipient this is the transport, not a candidate. Line 17: this.address = address; Line 18: } Line 19: Line 20: public static FilterEntry createEntryFromRecipient( 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; Line 17: this.address = address; this is also wrong, using address... most will not use address, but some name, which means something only to provider. Line 18: } Line 19: Line 20: public static FilterEntry createEntryFromRecipient( Line 21: String eventName, Line 64: } Line 65: Line 66: public String getAddress() { Line 67: return address; Line 68: } > This is the idiomatic way of doing things in public classes. only constructor should be public, all the other should be package protected, and can be accessed directly by the filter class. 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: } > Why? String format is less readable and less efficient. much more readable than this! java people needs glasses. believe me, you do not see the world properly. 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: 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()); > You now call this "name" (which I accept) while until now the term recipien yes. 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)); > I would prefer to encapsulate the logic about creating an object inside the the '*' is part of parsing, not part of class interface, please move it back here. 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/SMTP.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/transport/mail/SMTP.java: 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; > OK. here it means adding another boolean e.g 'successful' yes? whatever makes flow consistent... one entry two leave (normal, exception). Line 89: } catch (MessagingException ex) { Line 90: errorMessage = ex.getMessage(); Line 91: } Line 92: 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 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()) { > While I made a mistake in duplicating the pattern, please use the class to parse and validate, do not create duplication. it is perfectly ok if someone specify unsupported transport. 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: } > It should be here or the next one. it started with requireOne(NotificationP not sure I follow, but this preparation probably belong to own 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/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= > Yes. since filter was introduces here, I took the time to re order the diff reorder is a different 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: 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
