You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@falcon.apache.org by Narayan Periwal <na...@inmobi.com> on 2015/12/14 11:17:56 UTC
Review Request 41339: FALCON-1617: Enable SLA monitoring for
instances in past
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41339/
-----------------------------------------------------------
Review request for Falcon.
Bugs: FALCON-1617
https://issues.apache.org/jira/browse/FALCON-1617
Repository: falcon-git
Description
-------
Currently on enabling SLA monitoring it doesn't consider instances which had nominal time in past for SLA monitoring. With this JIRA we would like to enable this.
Diffs
-----
common/src/main/java/org/apache/falcon/entity/FeedHelper.java 29daff3
prism/src/main/java/org/apache/falcon/service/FeedSLAMonitoringService.java b302539
Diff: https://reviews.apache.org/r/41339/diff/
Testing
-------
Done.
Thanks,
Narayan Periwal
Re: Review Request 41339: FALCON-1617: Enable SLA monitoring for
instances in past
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41339/#review110477
-----------------------------------------------------------
Ship it!
Ship It!
- Ajay Yadava
On Dec. 15, 2015, 10:04 a.m., Narayan Periwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41339/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2015, 10:04 a.m.)
>
>
> Review request for Falcon.
>
>
> Bugs: FALCON-1617
> https://issues.apache.org/jira/browse/FALCON-1617
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Currently on enabling SLA monitoring it doesn't consider instances which had nominal time in past for SLA monitoring. With this JIRA we would like to enable this.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/FeedHelper.java 29daff3
> prism/src/main/java/org/apache/falcon/service/FeedSLAMonitoringService.java b302539
>
> Diff: https://reviews.apache.org/r/41339/diff/
>
>
> Testing
> -------
>
> Done.
>
>
> Thanks,
>
> Narayan Periwal
>
>
Re: Review Request 41339: FALCON-1617: Enable SLA monitoring for
instances in past
Posted by Narayan Periwal <na...@inmobi.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41339/
-----------------------------------------------------------
(Updated Dec. 15, 2015, 10:04 a.m.)
Review request for Falcon.
Bugs: FALCON-1617
https://issues.apache.org/jira/browse/FALCON-1617
Repository: falcon-git
Description
-------
Currently on enabling SLA monitoring it doesn't consider instances which had nominal time in past for SLA monitoring. With this JIRA we would like to enable this.
Diffs (updated)
-----
common/src/main/java/org/apache/falcon/entity/FeedHelper.java 29daff3
prism/src/main/java/org/apache/falcon/service/FeedSLAMonitoringService.java b302539
Diff: https://reviews.apache.org/r/41339/diff/
Testing
-------
Done.
Thanks,
Narayan Periwal
Re: Review Request 41339: FALCON-1617: Enable SLA monitoring for
instances in past
Posted by Narayan Periwal <na...@inmobi.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41339/
-----------------------------------------------------------
(Updated Dec. 15, 2015, 6:22 a.m.)
Review request for Falcon.
Bugs: FALCON-1617
https://issues.apache.org/jira/browse/FALCON-1617
Repository: falcon-git
Description
-------
Currently on enabling SLA monitoring it doesn't consider instances which had nominal time in past for SLA monitoring. With this JIRA we would like to enable this.
Diffs (updated)
-----
common/src/main/java/org/apache/falcon/entity/FeedHelper.java 29daff3
prism/src/main/java/org/apache/falcon/service/FeedSLAMonitoringService.java b302539
Diff: https://reviews.apache.org/r/41339/diff/
Testing
-------
Done.
Thanks,
Narayan Periwal
Re: Review Request 41339: FALCON-1617: Enable SLA monitoring for
instances in past
Posted by Narayan Periwal <na...@inmobi.com>.
> On Dec. 14, 2015, 5:17 p.m., Ajay Yadava wrote:
> > prism/src/main/java/org/apache/falcon/service/FeedSLAMonitoringService.java, line 330
> > <https://reviews.apache.org/r/41339/diff/1/?file=1162238#file1162238line330>
> >
> > I think your code is not rebased on the latest patch. This method has been renamed.
I have rebased the patch. "getRetentionFrequency" is the name of the new method that I have created.
- Narayan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41339/#review110257
-----------------------------------------------------------
On Dec. 14, 2015, 10:17 a.m., Narayan Periwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41339/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 10:17 a.m.)
>
>
> Review request for Falcon.
>
>
> Bugs: FALCON-1617
> https://issues.apache.org/jira/browse/FALCON-1617
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Currently on enabling SLA monitoring it doesn't consider instances which had nominal time in past for SLA monitoring. With this JIRA we would like to enable this.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/FeedHelper.java 29daff3
> prism/src/main/java/org/apache/falcon/service/FeedSLAMonitoringService.java b302539
>
> Diff: https://reviews.apache.org/r/41339/diff/
>
>
> Testing
> -------
>
> Done.
>
>
> Thanks,
>
> Narayan Periwal
>
>
Re: Review Request 41339: FALCON-1617: Enable SLA monitoring for
instances in past
Posted by Ajay Yadava <aj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41339/#review110257
-----------------------------------------------------------
common/src/main/java/org/apache/falcon/entity/FeedHelper.java (line 1013)
<https://reviews.apache.org/r/41339/#comment170072>
Write a helper method for calculating frequency for old retention. Logic is as below:
```
Frequency entityFrequency = entity.getFrequency();
Frequency defaultFrequency = new Frequency("hours(24)");
if (DateUtil.getFrequencyInMillis(entityFrequency) < DateUtil.getFrequencyInMillis(defaultFrequency)) {
coord.setFrequency("${coord:hours(6)}");
} else {
coord.setFrequency("${coord:days(1)}");
}
```
common/src/main/java/org/apache/falcon/entity/FeedHelper.java (line 1017)
<https://reviews.apache.org/r/41339/#comment170071>
@Narayan,
the way this part is currently written is not correct. You need to use the existing method to getLifeCycleRetentionFrequency and if that is null, then write another helper method(shown above) to calculate retention Frequency for old method.
common/src/main/java/org/apache/falcon/entity/FeedHelper.java (line 1021)
<https://reviews.apache.org/r/41339/#comment170069>
@Narayan
getLimit is not same as frequency, it's a frequency like expression used for specifying retention period but it is not same as frequency.
prism/src/main/java/org/apache/falcon/service/FeedSLAMonitoringService.java (line 330)
<https://reviews.apache.org/r/41339/#comment170073>
I think your code is not rebased on the latest patch. This method has been renamed.
prism/src/main/java/org/apache/falcon/service/FeedSLAMonitoringService.java (line 331)
<https://reviews.apache.org/r/41339/#comment170075>
You need to handle both lifecycle frequency and old frequency here.
- Ajay Yadava
On Dec. 14, 2015, 10:17 a.m., Narayan Periwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41339/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 10:17 a.m.)
>
>
> Review request for Falcon.
>
>
> Bugs: FALCON-1617
> https://issues.apache.org/jira/browse/FALCON-1617
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Currently on enabling SLA monitoring it doesn't consider instances which had nominal time in past for SLA monitoring. With this JIRA we would like to enable this.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/FeedHelper.java 29daff3
> prism/src/main/java/org/apache/falcon/service/FeedSLAMonitoringService.java b302539
>
> Diff: https://reviews.apache.org/r/41339/diff/
>
>
> Testing
> -------
>
> Done.
>
>
> Thanks,
>
> Narayan Periwal
>
>
Re: Review Request 41339: FALCON-1617: Enable SLA monitoring for
instances in past
Posted by Narayan Periwal <na...@inmobi.com>.
> On Dec. 14, 2015, 4:34 p.m., sandeep samudrala wrote:
> > common/src/main/java/org/apache/falcon/entity/FeedHelper.java, line 1021
> > <https://reviews.apache.org/r/41339/diff/1/?file=1162237#file1162237line1021>
> >
> > There is a small issue with getRetention().getLimit() w.r.t default frequency.
> > Can you change this method to call
> >
> > getRetentionFrequency(Feed feed, String clusterName)
> >
> > from this api?
@Sandeep, what is the issue?
- Narayan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41339/#review110250
-----------------------------------------------------------
On Dec. 14, 2015, 10:17 a.m., Narayan Periwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41339/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 10:17 a.m.)
>
>
> Review request for Falcon.
>
>
> Bugs: FALCON-1617
> https://issues.apache.org/jira/browse/FALCON-1617
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Currently on enabling SLA monitoring it doesn't consider instances which had nominal time in past for SLA monitoring. With this JIRA we would like to enable this.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/FeedHelper.java 29daff3
> prism/src/main/java/org/apache/falcon/service/FeedSLAMonitoringService.java b302539
>
> Diff: https://reviews.apache.org/r/41339/diff/
>
>
> Testing
> -------
>
> Done.
>
>
> Thanks,
>
> Narayan Periwal
>
>
Re: Review Request 41339: FALCON-1617: Enable SLA monitoring for
instances in past
Posted by sandeep samudrala <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41339/#review110250
-----------------------------------------------------------
common/src/main/java/org/apache/falcon/entity/FeedHelper.java (line 1021)
<https://reviews.apache.org/r/41339/#comment170055>
There is a small issue with getRetention().getLimit() w.r.t default frequency.
Can you change this method to call
getRetentionFrequency(Feed feed, String clusterName)
from this api?
- sandeep samudrala
On Dec. 14, 2015, 10:17 a.m., Narayan Periwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41339/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 10:17 a.m.)
>
>
> Review request for Falcon.
>
>
> Bugs: FALCON-1617
> https://issues.apache.org/jira/browse/FALCON-1617
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Currently on enabling SLA monitoring it doesn't consider instances which had nominal time in past for SLA monitoring. With this JIRA we would like to enable this.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/FeedHelper.java 29daff3
> prism/src/main/java/org/apache/falcon/service/FeedSLAMonitoringService.java b302539
>
> Diff: https://reviews.apache.org/r/41339/diff/
>
>
> Testing
> -------
>
> Done.
>
>
> Thanks,
>
> Narayan Periwal
>
>
Re: Review Request 41339: FALCON-1617: Enable SLA monitoring for
instances in past
Posted by sandeep samudrala <sa...@gmail.com>.
> On Dec. 14, 2015, 1:53 p.m., Peeyush Bishnoi wrote:
> > common/src/main/java/org/apache/falcon/entity/FeedHelper.java, line 1015
> > <https://reviews.apache.org/r/41339/diff/1/?file=1162237#file1162237line1015>
> >
> > If there is any code commonality between getRetentionFrequency and getLifecycleRetentionFrequency, can you think to merge that and invoke it appropriately from single function. I know that the getLifecycleRetentionFrequency is for lifecycle and getRetentionFrequency is for normal retention.
>
> Narayan Periwal wrote:
> If the retention stage is null, then in case of getLifecycleRetentionFrequency, we return null, whereas for the getRetentionFrequency, we return the retention limit of the cluster. If we try to merge them, we won't be able to handle this case. So, IMO its better if we keep them separate. What do you think?
@peeyush. I accept your point. There should be an api which would return retention frequency in general which would hide the hiererachy of various retentions.
But this can be tracked in another jira as an improvement and go ahead with this patch, as that would mean changes at many places to hide this hierarchy.
- sandeep
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41339/#review110231
-----------------------------------------------------------
On Dec. 14, 2015, 10:17 a.m., Narayan Periwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41339/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 10:17 a.m.)
>
>
> Review request for Falcon.
>
>
> Bugs: FALCON-1617
> https://issues.apache.org/jira/browse/FALCON-1617
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Currently on enabling SLA monitoring it doesn't consider instances which had nominal time in past for SLA monitoring. With this JIRA we would like to enable this.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/FeedHelper.java 29daff3
> prism/src/main/java/org/apache/falcon/service/FeedSLAMonitoringService.java b302539
>
> Diff: https://reviews.apache.org/r/41339/diff/
>
>
> Testing
> -------
>
> Done.
>
>
> Thanks,
>
> Narayan Periwal
>
>
Re: Review Request 41339: FALCON-1617: Enable SLA monitoring for
instances in past
Posted by Narayan Periwal <na...@inmobi.com>.
> On Dec. 14, 2015, 1:53 p.m., Peeyush Bishnoi wrote:
> > common/src/main/java/org/apache/falcon/entity/FeedHelper.java, line 1015
> > <https://reviews.apache.org/r/41339/diff/1/?file=1162237#file1162237line1015>
> >
> > If there is any code commonality between getRetentionFrequency and getLifecycleRetentionFrequency, can you think to merge that and invoke it appropriately from single function. I know that the getLifecycleRetentionFrequency is for lifecycle and getRetentionFrequency is for normal retention.
If the retention stage is null, then in case of getLifecycleRetentionFrequency, we return null, whereas for the getRetentionFrequency, we return the retention limit of the cluster. If we try to merge them, we won't be able to handle this case. So, IMO its better if we keep them separate. What do you think?
- Narayan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41339/#review110231
-----------------------------------------------------------
On Dec. 14, 2015, 10:17 a.m., Narayan Periwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41339/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 10:17 a.m.)
>
>
> Review request for Falcon.
>
>
> Bugs: FALCON-1617
> https://issues.apache.org/jira/browse/FALCON-1617
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Currently on enabling SLA monitoring it doesn't consider instances which had nominal time in past for SLA monitoring. With this JIRA we would like to enable this.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/FeedHelper.java 29daff3
> prism/src/main/java/org/apache/falcon/service/FeedSLAMonitoringService.java b302539
>
> Diff: https://reviews.apache.org/r/41339/diff/
>
>
> Testing
> -------
>
> Done.
>
>
> Thanks,
>
> Narayan Periwal
>
>
Re: Review Request 41339: FALCON-1617: Enable SLA monitoring for
instances in past
Posted by Peeyush Bishnoi <bp...@yahoo.co.in>.
> On Dec. 14, 2015, 1:53 p.m., Peeyush Bishnoi wrote:
> > common/src/main/java/org/apache/falcon/entity/FeedHelper.java, line 1015
> > <https://reviews.apache.org/r/41339/diff/1/?file=1162237#file1162237line1015>
> >
> > If there is any code commonality between getRetentionFrequency and getLifecycleRetentionFrequency, can you think to merge that and invoke it appropriately from single function. I know that the getLifecycleRetentionFrequency is for lifecycle and getRetentionFrequency is for normal retention.
>
> Narayan Periwal wrote:
> If the retention stage is null, then in case of getLifecycleRetentionFrequency, we return null, whereas for the getRetentionFrequency, we return the retention limit of the cluster. If we try to merge them, we won't be able to handle this case. So, IMO its better if we keep them separate. What do you think?
>
> sandeep samudrala wrote:
> @peeyush. I accept your point. There should be an api which would return retention frequency in general which would hide the hiererachy of various retentions.
> But this can be tracked in another jira as an improvement and go ahead with this patch, as that would mean changes at many places to hide this hierarchy.
Narayan, please create another Jira for improvement. If you can really do in this Jira, that will be good.
- Peeyush
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41339/#review110231
-----------------------------------------------------------
On Dec. 15, 2015, 6:22 a.m., Narayan Periwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41339/
> -----------------------------------------------------------
>
> (Updated Dec. 15, 2015, 6:22 a.m.)
>
>
> Review request for Falcon.
>
>
> Bugs: FALCON-1617
> https://issues.apache.org/jira/browse/FALCON-1617
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Currently on enabling SLA monitoring it doesn't consider instances which had nominal time in past for SLA monitoring. With this JIRA we would like to enable this.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/FeedHelper.java 29daff3
> prism/src/main/java/org/apache/falcon/service/FeedSLAMonitoringService.java b302539
>
> Diff: https://reviews.apache.org/r/41339/diff/
>
>
> Testing
> -------
>
> Done.
>
>
> Thanks,
>
> Narayan Periwal
>
>
Re: Review Request 41339: FALCON-1617: Enable SLA monitoring for
instances in past
Posted by Peeyush Bishnoi <bp...@yahoo.co.in>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41339/#review110231
-----------------------------------------------------------
common/src/main/java/org/apache/falcon/entity/FeedHelper.java (line 1015)
<https://reviews.apache.org/r/41339/#comment170027>
If there is any code commonality between getRetentionFrequency and getLifecycleRetentionFrequency, can you think to merge that and invoke it appropriately from single function. I know that the getLifecycleRetentionFrequency is for lifecycle and getRetentionFrequency is for normal retention.
- Peeyush Bishnoi
On Dec. 14, 2015, 10:17 a.m., Narayan Periwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41339/
> -----------------------------------------------------------
>
> (Updated Dec. 14, 2015, 10:17 a.m.)
>
>
> Review request for Falcon.
>
>
> Bugs: FALCON-1617
> https://issues.apache.org/jira/browse/FALCON-1617
>
>
> Repository: falcon-git
>
>
> Description
> -------
>
> Currently on enabling SLA monitoring it doesn't consider instances which had nominal time in past for SLA monitoring. With this JIRA we would like to enable this.
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/falcon/entity/FeedHelper.java 29daff3
> prism/src/main/java/org/apache/falcon/service/FeedSLAMonitoringService.java b302539
>
> Diff: https://reviews.apache.org/r/41339/diff/
>
>
> Testing
> -------
>
> Done.
>
>
> Thanks,
>
> Narayan Periwal
>
>