Alon Bar-Lev has posted comments on this change. Change subject: tools: implement and use transport.idle(). ......................................................................
Patch Set 3: (4 comments) http://gerrit.ovirt.org/#/c/24472/3/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/Notifier.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/Notifier.java: Line 83: NotificationService notificationService = new NotificationService(prop); Line 84: Smtp smtp = new Smtp(prop); Line 85: notificationService.registerTransport(smtp); Line 86: NotificationService.Idle idleHandler = new NotificationService.Idle(); Line 87: idleHandler.register(smtp, prop.getInteger(NotificationProperties.MAIL_SEND_INTERVAL)); In object oriented design the object should take care of its properties and behaviours. Unless there is some java style guide that wrongly present that. In different words, the transport should register it-self into the idle task, not the other way around. Line 88: EngineMonitorService engineMonitorService = new EngineMonitorService(prop); Line 89: Line 90: // add notification service to scheduler with its configurable interval Line 91: handler.addServiceHandler(notifyScheduler.scheduleWithFixedDelay(notificationService, http://gerrit.ovirt.org/#/c/24472/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 72: try { Line 73: doDispatch(attempt); Line 74: notifyObservers(DispatchResult.success(attempt.event, attempt.address, EventNotificationMethod.EMAIL)); Line 75: iterator.remove(); Line 76: } catch (Exception ex) { you should print exception to log in any case. Line 77: attempt.attempts++; Line 78: if (attempt.attempts >= ATTEMPTS) { Line 79: notifyObservers(DispatchResult.failure(attempt.event, Line 80: attempt.address, Line 93: message.prepareMessage(hostName, event, isBodyHtml); Line 94: Line 95: if (StringUtils.isEmpty(attempt.address)) { Line 96: log.error("Address is empty, cannot distribute message." + event.getName()); Line 97: return;//remove please do not return at middle of function. as this is internal... just don't put these entries in queue. Line 98: } Line 99: log.info(String.format("Send email to [%s]%n subject:%n [%s]", Line 100: attempt.address, Line 101: message.getMessageSubject())); http://gerrit.ovirt.org/#/c/24472/3/packaging/services/ovirt-engine-notifier/ovirt-engine-notifier.conf.in File packaging/services/ovirt-engine-notifier/ovirt-engine-notifier.conf.in: Line 58: # Line 59: # Interval to send smtp messages Line 60: # per # of IDLE_INTERVAL Line 61: # Line 62: MAIL_SEND_INTERVAL=1 move to mail section? Line 63: Line 64: # Amount of days to keep dispatched events on history table. If 0, events remain on history table for ever. Line 65: DAYS_TO_KEEP_HISTORY=0 Line 66: -- To view, visit http://gerrit.ovirt.org/24472 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8c4bafb542d28cb584e0751446d3e327f93e8112 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: 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
