mooli tayer has posted comments on this change. Change subject: tools: required changes in events data mapping. ......................................................................
Patch Set 6: (6 comments) http://gerrit.ovirt.org/#/c/23107/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/EventFilter.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/EventFilter.java: Line 1: package org.ovirt.engine.core.common.businessentities; Line 2: Line 3: public interface EventFilter { > It is good to have javadocs at the interface level. Done Line 4: Line 5: boolean isSubscribed(AuditLogEvent event); Line 6: http://gerrit.ovirt.org/#/c/23107/6/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 1: package org.ovirt.engine.core.common.businessentities; Line 2: Line 3: /** Line 4: * Note: UPDownEventFilters are immutable. > Immutable means that the state in the class can't be changed during life ti Yes, This comment is a left over that didn't pass compilation. This Object is better off being immutable, but I got a GWT error that it does not have a default constructor. (Which doesn't make sense for an immutable object that must have one constructor with all fields) 1. I will update comment. 2. Die GWT Die! Line 5: */ Line 6: public class UpDownEventFilter implements EventFilter { Line 7: Line 8: private String eventUpName; http://gerrit.ovirt.org/#/c/23107/6/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 29: public class EventsManager { Line 30: Line 31: private DataSource ds; Line 32: Line 33: public EventsManager() throws NotificationServiceException { > Why do we want to have runtime exception in the signature of the constructo Leftover, Will fix. Line 34: try { Line 35: ds = new StandaloneDataSource(); Line 36: } catch (SQLException e) { Line 37: throw new NotificationServiceException("Failed to obtain database connectivity", e); http://gerrit.ovirt.org/#/c/23107/6/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 58: Line 59: String recipient = subscriber.getMethodAddress(); Line 60: Line 61: if (StringUtils.isEmpty(recipient)) { Line 62: log.error(String.format("Email recipient is not known, please check user table ( email )" + > String format is the slowest String building code: Well string builder is used by the compiler. I usually like to keep exception strings as simple as possible(using not even String.format) and I see the same In many libraries. I guess the most efficient thing to do here is to remove the String.format and do sb.append("some string ").append(someVar) .... I'm not sure if it's worth it thought? Line 63: " or event_subscriber ( method_address )," + Line 64: " unable to send email for subscriber %s ," + Line 65: " message was %s:", Line 66: subscriber.getSubscriberId(), message.getMessageSubject(), message.getMessageBody())); Line 82: result.setReason(ex.getMessage()); Line 83: shouldRetry = true; Line 84: } Line 85: Line 86: for (int i = 0; i < RETRY_COUNT && shouldRetry; ++i) { > It would be good to use common retry logic which would simplify code like t Not part of this patch. I will consider doing a patch against master. Could you provide a link please? Line 87: shouldRetry = false; Line 88: try { Line 89: // hold the next send attempt for 30 seconds in case of a busy mail server Line 90: Thread.sleep(30000); http://gerrit.ovirt.org/#/c/23107/6/backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/utils/sender/mail/MessageHelper.java File backend/manager/tools/src/main/java/org/ovirt/engine/core/notifier/utils/sender/mail/MessageHelper.java: Line 5 Line 6 Line 7 Line 8 Line 9 > Wouldn't be better to make those strings final and use them as there were i OK will do. -- 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: 6 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: Piotr Kliczewski <[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
