> On Sept. 11, 2015, 2:06 a.m., Venkat Ranganathan wrote: > > Thanks for working on this. Also can you more unit tests were applicable > > (for example, email address validation, malformed emain notification list > > (trailing comma ) etc > > Peeyush Bishnoi wrote: > Thanks for the review. I have increased the coverage of unit tests for > EmailNotification.
Thanks! Venkat for the review. I have incorporated your comments in the patch. > On Sept. 11, 2015, 2:06 a.m., Venkat Ranganathan wrote: > > client/src/main/resources/feed-0.1.xsd, line 304 > > <https://reviews.apache.org/r/38105/diff/4/?file=1065396#file1065396line304> > > > > Initially we were discussing an option to send notification for all > > retries vs sending after exhaustion of all retries (which will be the > > default). Don't we need some attribute to distringuish that? > > Peeyush Bishnoi wrote: > Venkat, I am of the opinion that if user has defined the retry tag in > Entity, then we should send notification after exhaustion of all retries. If > user has not defined the retry in Entity, we will send after Instance failure > only(one attempt). We will document this so that user should be aware of > properly. So having the retry tag in Entity will help to distinguish itself. > To handle this issue, I am working on FALCON-1431/FALCON-1433. Thoughts please > > Venkat Ranganathan wrote: > If I defined retries, then I can expect an email if ever attempt fails or > only if all the retries are exhausted right? Isn't that what we thought we > will provide in future with only one notification after exhaustion of all > retries being the default? > > Peeyush Bishnoi wrote: > 1. If retry is defined along with notification and Falcon instance fail > <process name ....> > ... > ... > <retry policy="periodic" delay="minutes(2)" attempts="3"/> > <notification type="email" to="[email protected]"/> > </process> > > then failure email will be sent only if all the retries attempt (3) are > exhausted for Falcon instance. > > 2. If retry is not defined along with notification and Falcon instance > fail, then failure email will be sent even if attempt fails. > > Please let me know if this approach is fine. > > > Presently, there are issues with failure retry which I am working in > separate issue FALCON-1431/FALCON-1433 once that get fix, step 1 will work. As per discussion we had, I have added a new "limit" property in notification element of Feed/process xsd with value: "attempt" and "final". We will put details around this property once FALCON-1433 get fix. - Peeyush ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38105/#review98397 ----------------------------------------------------------- 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 > >
