Alon Bar-Lev has posted comments on this change. Change subject: tools: implement and use transport.idle(). ......................................................................
Patch Set 2: (10 comments) http://gerrit.ovirt.org/#/c/24472/2/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 34 Line 35 Line 36 Line 37 Line 38 well, if you put two lines within function... why the above is not added to that function as well? Line 39 Line 40 Line 41 Line 42 Line 43 function that is used one time and is private and is trivial, no reason to keep, move these two lines to the function above. Line 42: this.eventsManager = new EventsManager(); Line 43: this.firstMatchSimpleFilter = new FirstMatchSimpleFilter(); Line 44: Smtp smtp = new Smtp(prop); Line 45: smtp.addObserver(this.eventsManager); Line 46: addTransport(smtp); if transports is private, there is not really need for a method that wraps list add. Line 47: this.firstMatchSimpleFilter.registerTransport(smtp); Line 48: configurationFilters = FirstMatchSimpleFilter.parse(prop.getProperty(NotificationProperties.FILTER)); Line 49: markOldEventsAsProcessed(); Line 50: } Line 76: } Line 77: Line 78: private long getIdleInterval() { Line 79: return Integer.parseInt(NotificationProperties.IDLE_INTERVAL) * 1000; Line 80: } why is that a function? Do I understand correctly that you case the "IDLE_INTERVAL" string? Why not use LocalConfig.getInteger()? Line 81: Line 82: private void processEvents() { Line 83: try { Line 84: try { http://gerrit.ovirt.org/#/c/24472/2/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 98 Line 99 Line 100 Line 101 Line 102 this entire not success block should go within catch... no reason to mix procedural and exceptional flows. Line 18: import org.ovirt.engine.core.notifier.utils.NotificationProperties; Line 19: Line 20: /** Line 21: * The class sends e-mails to event subscribers. In order to define a proper mail client, the following properties Line 22: * should be provided: <li><code>MAIL_SERVER</code> mail server name <li><code>MAIL_PORT</code> mail server port</li><br> not part of this patch. Line 23: * The following properties are optional: <br> Line 24: * <li><code>MAIL_USER</code> user name includes a domain (e.g. u...@test.com)</li> <li><code>MAIL_PASSWORD</code> Line 25: * user's password</li> Line 26: * <ul> Line 27: * if failed to obtain or uses "localhost" if <code>MAIL_MACHINE_NAME</code> not provided</li> Line 28: * <li><code>MAIL_FROM</code> specifies "from" address in sent message, or uses value of property <code>MAIL_USER</code> Line 29: * if not provided</li> Line 30: * <ul> Line 31: * <li>"from" address should include a domain, same as <code>MAIL_USER</code> property not part of this patch. Line 32: * <li><code>MAIL_REPLY_TO</code> specifies "replyTo" address in outgoing message Line 33: */ Line 34: public class Smtp extends Observable implements Transport { Line 35: Line 67: } Line 68: Line 69: @Override Line 70: public void idle() { Line 71: DispatchAttempt dispatchAttempt = sendQueue.remove(); I am unsure I follow... within this attempt you need to attempt to send the entire queue, leaving entries that failed to next interval. Line 72: Event event = dispatchAttempt.getEvent(); Line 73: String address = dispatchAttempt.getAddress(); Line 74: Line 75: // throw an exception if not AuditLogEvent Line 113: } Line 114: Line 115: } Line 116: Line 117: private class DispatchAttempt { in this case this is pure pure pure private data only class. there is absolutely no reason to have something more than: private class DispatchAttempt { public final Event event; public final String address; public int attempts = 0; private DispatchAttempt(Event event, String address) { this.event = event; this.address = address; } } The java god will be happy, you will be good at the next world, or you can blame me. Line 118: Line 119: private final Event event; Line 120: Line 121: private final String address; http://gerrit.ovirt.org/#/c/24472/2/packaging/services/ovirt-engine-notifier/ovirt-engine-notifier.conf.in File packaging/services/ovirt-engine-notifier/ovirt-engine-notifier.conf.in: Line 62: # the notification service cannot connect to the DB. can be empty. Line 63: # Deprecated, use FILTER with DATABASE_UNREACHABLE instead. Line 64: FAILED_QUERIES_NOTIFICATION_RECIPIENTS= Line 65: Line 66: # Wait at least IDLE_INTERVAL seconds before allowing a Transport to become idle. we need two intervals... # # Idle task interval # Interval in seconds to perform low priority tasks. # IDLE_INTERVAL=30 # # Interval to send smtp messages # per # of IDLE_INTERVAL # MAIL_SEND_INTERVAL=1 Line 67: # An idle transport preforms background tasks like sending unsent queue. Line 68: IDLE_INTERVAL=30 Line 69: Line 70: #--------------# -- 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: 2 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: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
