Piotr Kliczewski 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. 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 time of the object. Here we have setters which can be used to modify the state at any time. It is better to set values in the constructor and have getters if needed. 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 constructor? 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: http://stackoverflow.com/questions/12786902/performance-javas-string-format Maybe it would be good to use something else. 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 this. Please take a look at jsonrpc Retryable class. 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 instead of moving them to the code? -- 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
