Mailing-List: contact dev-help@falcon.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@falcon.apache.org
Content-Type: multipart/alternative;
 boundary="===============8228828546005993441=="
MIME-Version: 1.0
Subject: Re: Review Request 38794: FALCON-1473 REST API for feed sla
 monitoring
From: "Ajay Yadava" <ajaynsit@gmail.com>
To: "Srikanth Sundarrajan" <sriksun@hotmail.com>,
 "Falcon" <dev@falcon.incubator.apache.org>,
 "Ajay Yadava" <ajaynsit@gmail.com>
Date: Tue, 29 Sep 2015 06:10:39 -0000
Message-ID: <20150929061039.23563.26041@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Ajay Yadava" <noreply@reviews.apache.org>
References: <20150928130858.1943.70660@reviews.apache.org>
In-Reply-To: <20150928130858.1943.70660@reviews.apache.org>
Reply-To: "Ajay Yadava" <ajaynsit@gmail.com>

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


> On Sept. 28, 2015, 1:08 p.m., Srikanth Sundarrajan wrote:
> > client/src/main/java/org/apache/falcon/cli/FalconCLI.java, line 723
> > <https://reviews.apache.org/r/38794/diff/2/?file=1085576#file1085576line723>
> >
> >     Is this only sla miss for feed ? Also for a process. From the result it seems to cover all schedulable entities.

Currently, this is only for feed. We can use the same type for process also later on.


> On Sept. 28, 2015, 1:08 p.m., Srikanth Sundarrajan wrote:
> > client/src/main/java/org/apache/falcon/resource/SchedulableEntityInstanceResult.java, line 33
> > <https://reviews.apache.org/r/38794/diff/2/?file=1085580#file1085580line33>
> >
> >     Should this be extending InstanceResult instead ? Will InstanceResult not suffice in this case ?

InstanceResult is very oozie instance specific and contains parameters like status, logFile, actions, wfParams etc. I can somehow fit this data in that but SchedulableEntityInstance is much cleaner and sufficient for this purpose.


> On Sept. 28, 2015, 1:08 p.m., Srikanth Sundarrajan wrote:
> > prism/src/main/java/org/apache/falcon/service/FeedSLAMonitoringService.java, line 454
> > <https://reviews.apache.org/r/38794/diff/2/?file=1085590#file1085590line454>
> >
> >     Am assuming over time when an instance becomes available it would be removed from missingInstances. In this case would SLA-Miss return nothing even though the data arrived quite late ? In other words, will this feature only report missing SLA while the feed is unavailable, or will it also be able to report misses even after the data became available.

It returns only pending feed instances which have missed sla. I had documented this behavior in FeedSLA.twiki appropriately but I had missed in many other places, fixed it. 

This API is useful for alerting usecases where users need to take actions e.g. to unblock pipelines. Once the data becomes available it is not so much useful from the perspective of requiring action from user but is still useful for reporting purposes. This API enables only the former usecase.


> On Sept. 28, 2015, 1:08 p.m., Srikanth Sundarrajan wrote:
> > prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java, line 118
> > <https://reviews.apache.org/r/38794/diff/2/?file=1085589#file1085589line118>
> >
> >     Should the path be sla-status instead ?

Converted to sla-alert. I intend to use sla-status for reporting status of all feed instances in a given time range, which is not solved by this API.


> On Sept. 28, 2015, 1:08 p.m., Srikanth Sundarrajan wrote:
> > client/src/main/java/org/apache/falcon/cli/FalconCLI.java, line 473
> > <https://reviews.apache.org/r/38794/diff/2/?file=1085576#file1085576line473>
> >
> >     slaMiss for a method name is a bit misleading

Converted to getFeedSLAMissPendingAlerts.


> On Sept. 28, 2015, 1:08 p.m., Srikanth Sundarrajan wrote:
> > prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java, line 160
> > <https://reviews.apache.org/r/38794/diff/2/?file=1085588#file1085588line160>
> >
> >     Shouldn't be accessing the FeedSLAMonitoringService instance via ServiceRegistry instead of adding a static method to the class ?

Fixed it.


- Ajay


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


On Sept. 29, 2015, 6:10 a.m., Ajay Yadava wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38794/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2015, 6:10 a.m.)
> 
> 
> Review request for Falcon.
> 
> 
> Bugs: FALCON-1473
>     https://issues.apache.org/jira/browse/FALCON-1473
> 
> 
> Repository: falcon-git
> 
> 
> Description
> -------
> 
> This api lists all the pending feed instances in a given time range which have missed sla.
> 
> 
> Diffs
> -----
> 
>   client/src/main/java/org/apache/falcon/ResponseHelper.java a13682b 
>   client/src/main/java/org/apache/falcon/cli/FalconCLI.java c914649 
>   client/src/main/java/org/apache/falcon/client/FalconClient.java 981559b 
>   client/src/main/java/org/apache/falcon/entity/v0/SchemaHelper.java 62b810c 
>   client/src/main/java/org/apache/falcon/resource/SchedulableEntityInstance.java f5be63d 
>   client/src/main/java/org/apache/falcon/resource/SchedulableEntityInstanceResult.java PRE-CREATION 
>   common/src/main/java/org/apache/falcon/entity/FeedHelper.java 572923b 
>   common/src/main/java/org/apache/falcon/entity/parser/FeedEntityParser.java 4f5599e 
>   common/src/main/resources/startup.properties 9db460c 
>   docs/src/site/twiki/FalconCLI.twiki 4f72bf8 
>   docs/src/site/twiki/restapi/FeedSLA.twiki PRE-CREATION 
>   docs/src/site/twiki/restapi/ResourceList.twiki ea3e3b6 
>   prism/src/main/java/org/apache/falcon/resource/AbstractEntityManager.java 2682257 
>   prism/src/main/java/org/apache/falcon/resource/AbstractSchedulableEntityManager.java 3280789 
>   prism/src/main/java/org/apache/falcon/resource/proxy/SchedulableEntityManagerProxy.java 47038e5 
>   prism/src/main/java/org/apache/falcon/service/FeedSLAMonitoringService.java 8bf43b8 
>   prism/src/test/java/org/apache/falcon/service/FeedSLAMonitoringTest.java PRE-CREATION 
>   src/conf/startup.properties 8f3bc35 
>   webapp/src/main/java/org/apache/falcon/resource/SchedulableEntityManager.java 1c0fc74 
> 
> Diff: https://reviews.apache.org/r/38794/diff/
> 
> 
> Testing
> -------
> 
> Unit tests added.
> Tested manually by deploying.
> 
> 
> Thanks,
> 
> Ajay Yadava
> 
>


--===============8228828546005993441==--