Mailing-List: contact dev-help@falcon.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@falcon.apache.org
Content-Type: multipart/alternative;
 boundary="===============0301712901427773548=="
MIME-Version: 1.0
Subject: Re: Review Request 38105: FALCON-1425: Provide Email based
 notification
 plugin to send notification when Falcon instance completes.
From: "Peeyush Bishnoi" <bpeeyush@yahoo.co.in>
To: "Venkat Ranganathan" <n.r.v@live.com>,
 "Peeyush Bishnoi" <bpeeyush@yahoo.co.in>,
 "Falcon" <dev@falcon.incubator.apache.org>
Date: Fri, 11 Sep 2015 15:07:12 -0000
Message-ID: <20150911150712.12750.85081@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Peeyush Bishnoi" <noreply@reviews.apache.org>
References: <20150911020611.12751.19858@reviews.apache.org>
In-Reply-To: <20150911020611.12751.19858@reviews.apache.org>
Reply-To: "Peeyush Bishnoi" <bpeeyush@yahoo.co.in>

--===============0301712901427773548==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> 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

Thanks for the review. I have increased the coverage of unit tests for EmailNotification.


> 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?

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


> On Sept. 11, 2015, 2:06 a.m., Venkat Ranganathan wrote:
> > client/src/main/resources/feed-0.1.xsd, line 117
> > <https://reviews.apache.org/r/38105/diff/4/?file=1065396#file1065396line117>
> >
> >     We are adding new eleements to an XSD without modifying the version.  It does not look like a good practice.   As I mentioned during the lifecycle discussions, we should have a version managmenet approach of XSDs.
> >     
> >     Something like:
> >     
> >     Major version change - incompatible changes
> >     Minor version change - compatible changes
> >     (We don't need to have more than one level of schema versioning unless it is felt otherwise by the community)

Venkat, I agree that XSD version should be changed when new element is added. Even I have tried to change the xsd version to 0.2 but looks like version mapping internally is tightly bound as I am seeing lot of issues and build is getting failed. I have created separate issue FALCON-1444 for bumping the XSD version.


- Peeyush


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


On Sept. 8, 2015, 6:50 p.m., Peeyush Bishnoi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38105/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2015, 6:50 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.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 
>   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
> 
>


--===============0301712901427773548==--