mooli tayer has posted comments on this change. Change subject: tools: required changes in events data mapping. ......................................................................
Patch Set 5: Verified+1 (7 comments) http://gerrit.ovirt.org/#/c/23107/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/AuditLogEvent.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/AuditLogEvent.java: Line 4: import org.ovirt.engine.core.compat.Guid; Line 5: Line 6: import java.util.Date; Line 7: Line 8: public class AuditLogEvent { > Usually our business entities extends IVdcQueryable and implements Business Please see comment on AuditLogEventSubscriber.java Line 9: Line 10: private long id; Line 11: Line 12: private String logTypeName; http://gerrit.ovirt.org/#/c/23107/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/AuditLogEventSubscriber.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/AuditLogEventSubscriber.java: Line 4: import org.ovirt.engine.core.compat.Guid; Line 5: Line 6: import java.io.Serializable; Line 7: Line 8: public class AuditLogEventSubscriber implements Serializable, EventFilter { > Usually our business entities extends IVdcQueryable and implements Business This class is actually a refactor+split of EventAuditLogSubscriber which only implements Serializable. In the future it is worth having the same dao in notification service and engine and remove this. I'm working on this but in requires some work. Line 9: Line 10: private EventNotificationMethod eventNotificationMethod; Line 11: Line 12: private String methodAddress; http://gerrit.ovirt.org/#/c/23107/5/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/AuditLogEventType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/AuditLogEventType.java: Line 1: package org.ovirt.engine.core.common.businessentities; Line 2: Line 3: public enum AuditLogEventType { Line 4: Line 5: resolveMessage(0, "Issue Solved Notification."), > Please rename enum values to upper case In this patch I moved this class to be an individual class and not an inner one. What you suggest has backward comp implications (change of email messages we send) I'm not sure we want to do that although it make sense w/o the backward comp issue. Line 6: Line 7: alertMessage(1, "Alert Notification."); Line 8: Line 9: private String message; http://gerrit.ovirt.org/#/c/23107/5/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/NotificationServiceException.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/NotificationServiceException.java: Line 2: Line 3: /** Line 4: * An exception of the notification service Line 5: */ Line 6: public class NotificationServiceException extends RuntimeException { > Why RuntimeException? In most cases I don't want to catch this exception but to let it propagate. I do not throw it when recovery is needed. http://my.safaribooksonline.com/book/programming/java/9780137150021/exceptions/ch09lev1sec3 Line 7: Line 8: private static final long serialVersionUID = 1L; Line 9: Line 10: public NotificationServiceException(String message) { http://gerrit.ovirt.org/#/c/23107/5/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 27: import java.util.List; Line 28: Line 29: public class EventsManager { Line 30: Line 31: DataSource ds; > private Done Line 32: Line 33: public EventsManager() throws NotificationServiceException { Line 34: try { Line 35: ds = new StandaloneDataSource(); Line 35: ds = new StandaloneDataSource(); Line 36: } catch (SQLException e) { Line 37: throw new NotificationServiceException("Failed to obtain database connectivity", e); Line 38: } Line 39: } > I understand this is a standalone tool - but can you still use our dal infr As I wrote in a preview comment, I'm working on it but I don't see it going in to this patch set, so this is something which was not fixed by my (current) patches. Line 40: Line 41: public List<AuditLogEventSubscriber> getAuditLogEventSubscribers() { Line 42: Connection connection = null; Line 43: PreparedStatement ps = null; http://gerrit.ovirt.org/#/c/23107/5/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/utils/sender/mail/EventMailSender.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/utils/sender/mail/EventMailSender.java: Line 80: shouldRetry = true; Line 81: } Line 82: Line 83: // Attempt additional 3 retries in case of failure Line 84: for (int i = 0; i < 3 && shouldRetry; ++i) { > 3 - configuration? Done Line 85: shouldRetry = false; Line 86: try { Line 87: // hold the next send attempt for 30 seconds in case of a busy mail server Line 88: Thread.sleep(30000); -- To view, visit http://gerrit.ovirt.org/23107 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3350d1c1caa6d730a3c7916ee0581f1aaa4a582a Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: mooli tayer <[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
