> On Sept. 14, 2015, 1:48 p.m., Ajay Yadava wrote: > > 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)
Thanks! Ajay for reviewing the patch. I have incorporated your comments in the patch. > On Sept. 14, 2015, 1:48 p.m., Ajay Yadava wrote: > > client/src/main/java/org/apache/falcon/entity/v0/EntityNotification.java, > > line 29 > > <https://reviews.apache.org/r/38105/diff/5/?file=1068323#file1068323line29> > > > > It will be more useful to have marshalled xml in toString, since it is > > a type for xsd and is used in xmls. I have checked other xsd type toString and found that even they are not marshalled xml. Will create separate issue to collectively perform this for all xsd type. > On Sept. 14, 2015, 1:48 p.m., Ajay Yadava wrote: > > client/src/main/java/org/apache/falcon/entity/v0/EntityNotification.java, > > line 26 > > <https://reviews.apache.org/r/38105/diff/5/?file=1068323#file1068323line26> > > > > What is type? What is it used for? Here type is related to Notification Type from xsd (<notification type="email" to="[email protected]"/>). EntityNotification is the base class for Feed Notification (org.apache.falcon.entity.v0.feed.Notification) and Process Notification (org.apache.falcon.entity.v0.process.Notification). So getType from EntityNotification should match to JAXB generated Notification. > On Sept. 14, 2015, 1:48 p.m., Ajay Yadava wrote: > > prism/src/main/java/org/apache/falcon/util/NotificationUtil.java, line 44 > > <https://reviews.apache.org/r/38105/diff/5/?file=1068341#file1068341line44> > > > > 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,})$" Earlier I have used the similar regex which you have suggested for email pattern. But I have relaxed this so that other required email pattern like falcon@localhost etc. should match. > On Sept. 14, 2015, 1:48 p.m., Ajay Yadava wrote: > > prism/src/main/java/org/apache/falcon/plugin/EmailNotification.java, line > > 110 > > <https://reviews.apache.org/r/38105/diff/5/?file=1068338#file1068338line110> > > > > This should be under the "if - else if" to avoid sending email for > > alerts other than "wf-instance-succeeded" and "wf-instance-failed" Check for this is already available in class EmailNotificationPlugin. Please check. > On Sept. 14, 2015, 1:48 p.m., Ajay Yadava wrote: > > prism/src/main/java/org/apache/falcon/plugin/EmailNotification.java, line 45 > > <https://reviews.apache.org/r/38105/diff/5/?file=1068338#file1068338line45> > > > > 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. To send the notification events through email we have to extend the monitoring framework by implementing org.apache.falcon.plugin.MonitoringPlugin interface. Due to this EmailNotificationPlugin require MonitoringPlugin(similar to DefaultMonitoringPlugin). But for actual notification implementation we require to implement the NotificationPlugin interface for sending the notification events and in future it should be used by any other notification mechanism like Kafka, HTTP etc. > On Sept. 14, 2015, 1:48 p.m., Ajay Yadava wrote: > > oozie/src/test/java/org/apache/falcon/oozie/process/OozieProcessWorkflowBuilderTest.java, > > line 222 > > <https://reviews.apache.org/r/38105/diff/5/?file=1068334#file1068334line222> > > > > Should be part of ProcessEntityParserTest. Moved the testcase to class ProcessEntityParserTest. > On Sept. 14, 2015, 1:48 p.m., Ajay Yadava wrote: > > oozie/src/test/java/org/apache/falcon/oozie/feed/OozieFeedWorkflowBuilderTest.java, > > line 286 > > <https://reviews.apache.org/r/38105/diff/5/?file=1068333#file1068333line286> > > > > this test should be part of FeedEntityParserTest. It has nothing to do > > with workflowbuilding or oozie. Moved the testcase to class FeedEntityParserTest. > On Sept. 14, 2015, 1:48 p.m., Ajay Yadava wrote: > > metrics/src/main/java/org/apache/falcon/plugin/NotificationPlugin.java, > > line 27 > > <https://reviews.apache.org/r/38105/diff/5/?file=1068330#file1068330line27> > > > > This interface has no characterstics of a plugin unlike others e.g. > > MonitoringPlugin. It should be named as just Notification. Notification class has already been used by notification element from feed/process xsd, so can't reuse the name again. Also if going forward, other notification (like Kafka/HTTP) implementation required to send notification for captured events, this class can be implemented. > On Sept. 14, 2015, 1:48 p.m., Ajay Yadava wrote: > > metrics/src/main/java/org/apache/falcon/plugin/NotificationPlugin.java, > > line 25 > > <https://reviews.apache.org/r/38105/diff/5/?file=1068330#file1068330line25> > > > > 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. Done. > On Sept. 14, 2015, 1:48 p.m., Ajay Yadava wrote: > > client/src/main/resources/process-0.1.xsd, line 412 > > <https://reviews.apache.org/r/38105/diff/5/?file=1068326#file1068326line412> > > > > same as feed.xsd done. > On Sept. 14, 2015, 1:48 p.m., Ajay Yadava wrote: > > client/src/main/resources/feed-0.1.xsd, line 304 > > <https://reviews.apache.org/r/38105/diff/5/?file=1068324#file1068324line304> > > > > It will be good to document that the to list can contain multiple email > > addresseses separated by comma. Done. > On Sept. 14, 2015, 1:48 p.m., Ajay Yadava wrote: > > client/src/main/resources/feed-0.1.xsd, line 303 > > <https://reviews.apache.org/r/38105/diff/5/?file=1068324#file1068324line303> > > > > type should be of xs:enumeration instead of xs:string because any > > random string won't be accepted by falcon. Done. Currently only type ="email" will be accepted. Any other type entered will not be accepted by xsd. > On Sept. 14, 2015, 1:48 p.m., Ajay Yadava wrote: > > client/src/main/java/org/apache/falcon/entity/v0/EntityNotification.java, > > line 23 > > <https://reviews.apache.org/r/38105/diff/5/?file=1068323#file1068323line23> > > > > nit: this documentation can use some more explanation. > > What is meant by notification list - "recepients list" or "type of > > notifications"? Done. > On Sept. 14, 2015, 1:48 p.m., Ajay Yadava wrote: > > metrics/src/main/java/org/apache/falcon/util/EmailNotificationArgs.java, > > line 24 > > <https://reviews.apache.org/r/38105/diff/5/?file=1068331#file1068331line24> > > > > These are not arguments for Email Notification, these are just > > constants for SMTP properties. Should be renamed accordingly. Renamed to EmailNotificationProps. Done. - Peeyush ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38105/#review98677 ----------------------------------------------------------- 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 > >
