----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38105/#review98677 -----------------------------------------------------------
This is a very useful feature. It will be very useful to have a one page documentation in the docs on how to use it and it's behavior (e.g. email coming only after all retries or otherwise) client/src/main/java/org/apache/falcon/entity/v0/EntityNotification.java (line 23) <https://reviews.apache.org/r/38105/#comment155436> nit: this documentation can use some more explanation. What is meant by notification list - "recepients list" or "type of notifications"? client/src/main/java/org/apache/falcon/entity/v0/EntityNotification.java (line 26) <https://reviews.apache.org/r/38105/#comment155437> What is type? What is it used for? client/src/main/java/org/apache/falcon/entity/v0/EntityNotification.java (line 29) <https://reviews.apache.org/r/38105/#comment155438> It will be more useful to have marshalled xml in toString, since it is a type for xsd and is used in xmls. client/src/main/resources/feed-0.1.xsd (line 303) <https://reviews.apache.org/r/38105/#comment155439> type should be of xs:enumeration instead of xs:string because any random string won't be accepted by falcon. client/src/main/resources/feed-0.1.xsd (line 304) <https://reviews.apache.org/r/38105/#comment155447> It will be good to document that the to list can contain multiple email addresseses separated by comma. client/src/main/resources/process-0.1.xsd (line 412) <https://reviews.apache.org/r/38105/#comment155440> same as feed.xsd metrics/src/main/java/org/apache/falcon/plugin/NotificationPlugin.java (line 25) <https://reviews.apache.org/r/38105/#comment155445> There is no "Notification concrete class". Perhaps you meant EmailNotification.java. In any case it is not a good documentation for this interface. A short description of what is the contract that this interface provides will be useful. metrics/src/main/java/org/apache/falcon/plugin/NotificationPlugin.java (line 27) <https://reviews.apache.org/r/38105/#comment155444> This interface has no characterstics of a plugin unlike others e.g. MonitoringPlugin. It should be named as just Notification. metrics/src/main/java/org/apache/falcon/util/EmailNotificationArgs.java (line 24) <https://reviews.apache.org/r/38105/#comment155446> These are not arguments for Email Notification, these are just constants for SMTP properties. Should be renamed accordingly. oozie/src/test/java/org/apache/falcon/oozie/feed/OozieFeedWorkflowBuilderTest.java (line 286) <https://reviews.apache.org/r/38105/#comment155441> this test should be part of FeedEntityParserTest. It has nothing to do with workflowbuilding or oozie. oozie/src/test/java/org/apache/falcon/oozie/process/OozieProcessWorkflowBuilderTest.java (line 222) <https://reviews.apache.org/r/38105/#comment155442> Should be part of ProcessEntityParserTest. prism/src/main/java/org/apache/falcon/plugin/EmailNotification.java (line 45) <https://reviews.apache.org/r/38105/#comment155443> There are 2 classes: 1) EmailNotification implements NotificationPlugin 2) EmailNotificationPlugin implements MonitoringPlugin They are both confusing in terms of name. There should be just one class which implements both monitoring plugin and NotificationPlugin and called EmailNotificationPlugin. prism/src/main/java/org/apache/falcon/plugin/EmailNotification.java (line 110) <https://reviews.apache.org/r/38105/#comment155455> This should be under the "if - else if" to avoid sending email for alerts other than "wf-instance-succeeded" and "wf-instance-failed" prism/src/main/java/org/apache/falcon/util/NotificationUtil.java (line 44) <https://reviews.apache.org/r/38105/#comment155454> Just an observation, this pattern will allow some invalid domains. e.g. [email protected] Although a perfect validation is not required as this is still a runtime check and mail will fail in any case. Following might be a better regex. "^[_A-Za-z0-9-\+]+(\.[_A-Za-z0-9-]+)*@" + "[A-Za-z0-9-]+(\.[A-Za-z0-9]+)*(\.[A-Za-z]{2,})$" - Ajay Yadava On Sept. 11, 2015, 3:09 p.m., Peeyush Bishnoi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38105/ > ----------------------------------------------------------- > > (Updated Sept. 11, 2015, 3:09 p.m.) > > > Review request for Falcon. > > > Bugs: FALCON-1425 > https://issues.apache.org/jira/browse/FALCON-1425 > > > Repository: falcon-git > > > Description > ------- > > Provide Email based notification plugin to send notification when Falcon > instance completes. > > > Diffs > ----- > > client/src/main/java/org/apache/falcon/entity/v0/EntityNotification.java > PRE-CREATION > client/src/main/resources/feed-0.1.xsd 4ff8baa > client/src/main/resources/jaxb-binding.xjb f644f40 > client/src/main/resources/process-0.1.xsd c81d6f7 > common/src/main/java/org/apache/falcon/entity/EntityUtil.java 25d9008 > common/src/main/resources/startup.properties c48188c > metrics/pom.xml 3d558fc > metrics/src/main/java/org/apache/falcon/plugin/NotificationPlugin.java > PRE-CREATION > metrics/src/main/java/org/apache/falcon/util/EmailNotificationArgs.java > PRE-CREATION > metrics/src/main/java/org/apache/falcon/util/NotificationType.java > PRE-CREATION > > oozie/src/test/java/org/apache/falcon/oozie/feed/OozieFeedWorkflowBuilderTest.java > 2f7787d > > oozie/src/test/java/org/apache/falcon/oozie/process/OozieProcessWorkflowBuilderTest.java > 3aaf304 > oozie/src/test/resources/config/process/process-notification.xml > PRE-CREATION > oozie/src/test/resources/feed/feed-notification.xml PRE-CREATION > prism/pom.xml 52b558d > prism/src/main/java/org/apache/falcon/plugin/EmailNotification.java > PRE-CREATION > prism/src/main/java/org/apache/falcon/plugin/EmailNotificationPlugin.java > PRE-CREATION > prism/src/main/java/org/apache/falcon/plugin/NotificationHandler.java > PRE-CREATION > prism/src/main/java/org/apache/falcon/util/NotificationUtil.java > PRE-CREATION > prism/src/test/java/org/apache/falcon/plugin/EmailNotificationTest.java > PRE-CREATION > src/conf/startup.properties 9925373 > > Diff: https://reviews.apache.org/r/38105/diff/ > > > Testing > ------- > > Yes, manual testing has been done for this after configuring > startup.properties with SMTP properties. > Also test cases has been added to test Falcon feed/process entity with > notification tag. > Unit test has been added to test Email Notification. > > > Thanks, > > Peeyush Bishnoi > >
