falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Peeyush Bishnoi" <bpeey...@yahoo.co.in>
Subject Re: Review Request 38105: FALCON-1425: Provide Email based notification plugin to send notification when Falcon instance completes.
Date Tue, 08 Sep 2015 07:33:20 GMT


> On Sept. 7, 2015, 4:51 a.m., Pallavi Rao wrote:
> > client/src/main/resources/feed-0.1.xsd, line 117
> > <https://reviews.apache.org/r/38105/diff/2/?file=1064612#file1064612line117>
> >
> >     Add doc here as you have done in process xsd.

done.


> On Sept. 7, 2015, 4:51 a.m., Pallavi Rao wrote:
> > prism/src/main/java/org/apache/falcon/plugin/EmailNotification.java, line 43
> > <https://reviews.apache.org/r/38105/diff/2/?file=1064626#file1064626line43>
> >
> >     This class and other classes following this are completely missing UTs. Can
we add some UTs, please?

UT's added for class EmailNotification using GreenEmail server.


> On Sept. 7, 2015, 4:51 a.m., Pallavi Rao wrote:
> > metrics/src/main/java/org/apache/falcon/util/EmailNotificationArgs.java, line 28
> > <https://reviews.apache.org/r/38105/diff/2/?file=1064619#file1064619line28>
> >
> >     Nit : Add to startup.properties as a commented line, so users know the arg to
set.

added the auth arg in statup.properties. Also done changes in class EmailNotification to handle
auth.


> On Sept. 7, 2015, 4:51 a.m., Pallavi Rao wrote:
> > oozie/src/test/java/org/apache/falcon/oozie/feed/OozieFeedWorkflowBuilderTest.java,
line 287
> > <https://reviews.apache.org/r/38105/diff/2/?file=1064621#file1064621line287>
> >
> >     Nit : Rename the method to reflect what it is doing.. "testEmailNotification"
should suffice.

done


> On Sept. 7, 2015, 4:51 a.m., Pallavi Rao wrote:
> > metrics/pom.xml, line 66
> > <https://reviews.apache.org/r/38105/diff/2/?file=1064617#file1064617line66>
> >
> >     Why 1.4 when 1.4.7 is the latest?

done


> On Sept. 7, 2015, 4:51 a.m., Pallavi Rao wrote:
> > client/src/main/resources/feed-0.1.xsd, line 297
> > <https://reviews.apache.org/r/38105/diff/2/?file=1064612#file1064612line297>
> >
> >     I see that you have kept the notification generic (it can be http tomorrow).
In that case, should we call this "destination" instead?

I am of the opinion to keep it "to" only. for e.g: to:<email address>, to:<kafka
topic>, to:<http url> .


- Peeyush


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38105/#review97889
-----------------------------------------------------------


On Sept. 5, 2015, 12:12 p.m., Peeyush Bishnoi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38105/
> -----------------------------------------------------------
> 
> (Updated Sept. 5, 2015, 12:12 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 748fb97 
>   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
d727ca3 
>   oozie/src/test/resources/config/process/process-notification-0.1.xml PRE-CREATION 
>   oozie/src/test/resources/feed/feed-notification.xml PRE-CREATION 
>   prism/pom.xml be04ac9 
>   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 
>   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.
> 
> 
> Thanks,
> 
> Peeyush Bishnoi
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message