falcon-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ajay Yadava" <ajayn...@gmail.com>
Subject Re: Review Request 38105: FALCON-1425: Provide Email based notification plugin to send notification when Falcon instance completes.
Date Mon, 14 Sep 2015 13:48:44 GMT

-----------------------------------------------------------
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. ajay.yadav@gmail.com.1
    
    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
> 
>


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