Alon Bar-Lev has posted comments on this change. Change subject: tools: Support snmp trap as a notification method. ......................................................................
Patch Set 18: (9 comments) http://gerrit.ovirt.org/#/c/22909/18/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/UpDownEventFilter.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/UpDownEventFilter.java: Line 18: // @Override Line 19: // public boolean isSubscribed(AuditLogEvent event) { Line 20: // return event.getLogTypeName().equals(eventUpName) || event.getLogTypeName().equals(eventDownName); Line 21: // } Line 22: //} no need to commit such code. http://gerrit.ovirt.org/#/c/22909/18/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/WhitelistFilter.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/WhitelistFilter.java: Line 1: package org.ovirt.engine.core.common.businessentities; Line 2: Line 3: import java.util.Set; Line 4: Line 5: public class WhitelistFilter implements EventFilter { no need to abstract this, why filter cannot return true/false based on data it has? we discussed common data structure: msg include/exclude transport transport_data no need to abstract the data. Line 6: Line 7: private Set<String> values; Line 8: Line 9: public void setValues(Set<String> values) { http://gerrit.ovirt.org/#/c/22909/18/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/LocalConfig.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/LocalConfig.java: Line 280: * allowMissing is false. Line 281: */ Line 282: public String getProperty(String key, String optionalSuffix, boolean allowMissing) { Line 283: String property = getProperty(key + "_" + optionalSuffix, true); Line 284: if(property == null) { use StringUtils and isBlank to check for both null or "" Line 285: property = getProperty(key , allowMissing); Line 286: } Line 287: return property; Line 288: } http://gerrit.ovirt.org/#/c/22909/18/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 126: if (subscriber.isSubscribed(event)) { Line 127: log.debug(sb.append("address:") Line 128: .append(subscriber.getMethodAddress()) Line 129: .append(" subscribed to event: ") Line 130: .append(event.getLogTypeName()).toString() String.format is much better. Line 131: ); Line 132: EventSender sender = notificationMethodsMapper.getEventSender(subscriber.getEventNotificationMethod()); Line 133: EventSenderResult sendResult; Line 134: try { http://gerrit.ovirt.org/#/c/22909/18/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/sender/snmp/SNMPEventSender.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/sender/snmp/SNMPEventSender.java: Line 36: Line 37: try { Line 38: // Create a new session and define it's transport. Line 39: snmp = new Snmp(new DefaultUdpTransportMapping()); Line 40: snmp.listen(); why do you listen? Line 41: } catch (IOException e) { Line 42: throw new NotificationServiceException("error creating " + getClass().getName()); Line 43: } Line 44: } http://gerrit.ovirt.org/#/c/22909/18/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/subscribers/ConfigurationSubscribersProvider.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/subscribers/ConfigurationSubscribersProvider.java: Line 74: String method = matcher.group("METHOD"); Line 75: EventNotificationMethod eventNotificationMethod = null; Line 76: if (method != null) { Line 77: switch (method) { Line 78: case "MAIL": can we have this smtp and snmp lower case and consistent based on protocol? Line 79: subscribers.add(createSubscriber(EventNotificationMethod.EMAIL, profile, names, include));//eventNotificationMethod = EventNotificationMethod.EMAIL; Line 80: break; Line 81: case "SNMP": Line 82: subscribers.add(createSubscriber(EventNotificationMethod.SNMP_TRAP, profile, names, include));//eventNotificationMethod = EventNotificationMethod.SNMP_TRAP; Line 93: } Line 94: } Line 95: } Line 96: Line 97: private AuditLogEventSubscriber createSubscriber( why don't you use the same method to add data read from the database? Line 98: EventNotificationMethod eventNotificationMethod, String profile, String[] names, boolean include) { Line 99: AuditLogEventSubscriber newSubscriber = new AuditLogEventSubscriber(); Line 100: WhitelistFilter eventFilter = new WhitelistFilter(); Line 101: HashSet<String> values; Line 114: Line 115: private String resolveAddress(EventNotificationMethod eventNotificationMethod, String profile) { Line 116: switch (eventNotificationMethod) { Line 117: case EMAIL: Line 118: return properties.getProperty(NotificationProperties.MAIL_ADDRESS, profile, false); I think this is too match farther.... no need for profile to email. it should be: if (isblank(profile)) { return getProperty(NotificationProperties.MAIL_ADDRESS); } Line 119: case SNMP_TRAP: Line 120: return profile; Line 121: } Line 122: return null; http://gerrit.ovirt.org/#/c/22909/18/packaging/dbscripts/upgrade/03_04_0510_event_notification_methods.sql File packaging/dbscripts/upgrade/03_04_0510_event_notification_methods.sql: Line 26: INNER JOIN users u ON es2.subscriber_id = u.user_id Line 27: WHERE (es.method_address is NULL OR trim(both from es.method_address) = ''); Line 28: Line 29: -- change events history table Line 30: ALTER TABLE event_notification_hist DROP COLUMN subscriber_id; well, after discussion with barak, I truly think we should not touch the database at all removing these, not that important if you want to keep, but... still there is no use for that. -- To view, visit http://gerrit.ovirt.org/22909 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0cd22d022ae535f45e046b09a2cbfadd837b465c Gerrit-PatchSet: 18 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: mooli tayer <[email protected]> Gerrit-Reviewer: Alon Bar-Lev <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Liran Zelkha <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: mooli tayer <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
