> On April 16, 2014, 11:57 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java, line 71
> > <https://reviews.apache.org/r/20398/diff/1/?file=560703#file560703line71>
> >
> >     How about Multimap, ImmutableMultimap.Builder?

Great idea. Done.


> On April 16, 2014, 11:57 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java, line 
> > 103
> > <https://reviews.apache.org/r/20398/diff/1/?file=560703#file560703line103>
> >
> >     Does it make sense to just replace this with AtomicDouble?  Seems like 
> > it would simplify code, and forcing integer-supplying metrics to use double 
> > shouldn't be a problem.

I am not sure there is much simplification to gain from AtomicDouble. The only 
difference would be in type (Number -> Double) but I would still have to use 
gauge as there is nothing similar to Stat.makeCounter() that would expose 
AtomicDouble.


> On April 16, 2014, 11:57 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java, line 
> > 129
> > <https://reviews.apache.org/r/20398/diff/1/?file=560703#file560703line129>
> >
> >     checkNotNull, ditto for statsProvider

Added for settings. The statProvider is already checked.


> On April 16, 2014, 11:57 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java, line 
> > 142
> > <https://reviews.apache.org/r/20398/diff/1/?file=560703#file560703line142>
> >
> >     This log line and the one below will result in log spam, consider 
> > dropping.

That was a cheap shot at profiling. Replaced with a @Timed stat.


> On April 16, 2014, 11:57 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java, line 
> > 147
> > <https://reviews.apache.org/r/20398/diff/1/?file=560703#file560703line147>
> >
> >     How about intervalStartMs?  lastRunMs is slightly weird for run zero.

Sure, changed.


> On April 16, 2014, 11:57 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/sla/SlaAlgorithm.java, line 94
> > <https://reviews.apache.org/r/20398/diff/1/?file=560704#file560704line94>
> >
> >     Seems like it would be fairly straightforward to retrofit this for 
> > median as opposed to mean.  How would you feel about doing that?
> 
> Maxim Khutornenko wrote:
>     Need to think if there are any downsides to this but off the top I think 
> it would make sense.

Converted from mean to median.


> On April 16, 2014, 11:57 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/sla/SlaAlgorithm.java, line 96
> > <https://reviews.apache.org/r/20398/diff/1/?file=560704#file560704line96>
> >
> >     fits on one line

Yep, it does now.


> On April 16, 2014, 11:57 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/sla/SlaAlgorithm.java, line 144
> > <https://reviews.apache.org/r/20398/diff/1/?file=560704#file560704line144>
> >
> >     I find myself having to read aloud "Mean time to assigned" when i see 
> > Mtta.  How about embracing the verbosity for readability and calling it 
> > MeanTimeToAssigned?  Ditto elsewhere.

Expanded.


> On April 16, 2014, 11:57 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/sla/SlaInstance.java, line 32
> > <https://reviews.apache.org/r/20398/diff/1/?file=560706#file560706line32>
> >
> >     It's not clear to me that this class is worth having, does it make 
> > sense to consider removing it?

Agree, the purpose of this class has degraded over a few iterations. Dropped.


> On April 16, 2014, 11:57 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java, line 90
> > <https://reviews.apache.org/r/20398/diff/1/?file=560707#file560707line90>
> >
> >     remove extra WS

Removed.


> On April 16, 2014, 11:57 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/sla/MetricCalculatorTest.java, 
> > line 39
> > <https://reviews.apache.org/r/20398/diff/1/?file=560708#file560708line39>
> >
> >     Does this need to be static?  Prefer non-static.

Changed.


- Maxim


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


On April 16, 2014, 1:39 a.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20398/
> -----------------------------------------------------------
> 
> (Updated April 16, 2014, 1:39 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.
> 
> 
> Bugs: AURORA-293
>     https://issues.apache.org/jira/browse/AURORA-293
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> High level overview:
> - MetricCalculator runs periodically (every minute), pulls all task history, 
> packages it into SlaInstance list and updates stats;
> - Stat calculation is handled by a pair of: SlaAlgorithm and a set of 
> applicable SlaGroups (logical groupings by job, cluster, resource and etc.);
> - Stat name is generated by combining group and algorithm name parts.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> eeafc784e915137cacd5f64df1252ccbaf6c0f6c 
>   src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java 
> bc77bf2e6fbc1ff4159d049b0c28afd6832499ef 
>   src/main/java/org/apache/aurora/scheduler/base/Tasks.java 
> fae2d237d18f945d4dd73ec56cd42981359bea46 
>   src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/sla/SlaAlgorithm.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/sla/SlaGroup.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/sla/SlaInstance.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/sla/MetricCalculatorTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/sla/SlaAlgorithmTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/sla/SlaTestUtil.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/20398/diff/
> 
> 
> Testing
> -------
> 
> gradle build
> gradle run
> 
> Sample from local scheduler:
> 
> sla_cluster_mtta 3271.493
> sla_cluster_mttr 3273.497
> sla_cluster_platform_uptime_percent 100.0
> sla_cpu_small_mtta 3271.493
> sla_cpu_small_mttr 3273.497
> sla_disk_small_mtta 3271.493
> sla_disk_small_mttr 3273.497
> sla_mesos_test_serviceJob0_job_uptime_75_00_sec 0
> sla_mesos_test_serviceJob0_job_uptime_90_00_sec 0
> sla_mesos_test_serviceJob0_job_uptime_95_00_sec 0
> sla_mesos_test_serviceJob0_job_uptime_99_00_sec 0
> sla_mesos_test_serviceJob0_mtta 0.0
> sla_mesos_test_serviceJob0_mttr 0.0
> sla_mesos_test_serviceJob0_platform_uptime_percent 100.0
> sla_mesos_test_serviceJob10_job_uptime_75_00_sec 0
> sla_mesos_test_serviceJob10_job_uptime_90_00_sec 0
> sla_mesos_test_serviceJob10_job_uptime_95_00_sec 0
> sla_mesos_test_serviceJob10_job_uptime_99_00_sec 0
> sla_mesos_test_serviceJob10_mtta 0.0
> sla_mesos_test_serviceJob10_mttr 0.0
> sla_mesos_test_serviceJob10_platform_uptime_percent 100.0
> sla_mesos_test_serviceJob12_job_uptime_75_00_sec 0
> sla_mesos_test_serviceJob12_job_uptime_90_00_sec 0
> sla_mesos_test_serviceJob12_job_uptime_95_00_sec 0
> sla_mesos_test_serviceJob12_job_uptime_99_00_sec 0
> sla_mesos_test_serviceJob12_mtta 0.0
> sla_mesos_test_serviceJob12_mttr 0.0
> sla_mesos_test_serviceJob12_platform_uptime_percent 100.0
> sla_mesos_test_serviceJob14_job_uptime_75_00_sec 0
> sla_mesos_test_serviceJob14_job_uptime_90_00_sec 0
> sla_mesos_test_serviceJob14_job_uptime_95_00_sec 0
> sla_mesos_test_serviceJob14_job_uptime_99_00_sec 0
> sla_mesos_test_serviceJob14_mtta 0.0
> sla_mesos_test_serviceJob14_mttr 0.0
> sla_mesos_test_serviceJob14_platform_uptime_percent 100.0
> sla_mesos_test_serviceJob16_job_uptime_75_00_sec 0
> sla_mesos_test_serviceJob16_job_uptime_90_00_sec 0
> sla_mesos_test_serviceJob16_job_uptime_95_00_sec 0
> sla_mesos_test_serviceJob16_job_uptime_99_00_sec 0
> sla_mesos_test_serviceJob16_mtta 0.0
> sla_mesos_test_serviceJob16_mttr 0.0
> sla_mesos_test_serviceJob16_platform_uptime_percent 100.0
> sla_mesos_test_serviceJob18_job_uptime_75_00_sec 0
> sla_mesos_test_serviceJob18_job_uptime_90_00_sec 0
> sla_mesos_test_serviceJob18_job_uptime_95_00_sec 0
> sla_mesos_test_serviceJob18_job_uptime_99_00_sec 0
> sla_mesos_test_serviceJob18_mtta 0.0
> sla_mesos_test_serviceJob18_mttr 0.0
> sla_mesos_test_serviceJob18_platform_uptime_percent 100.0
> sla_mesos_test_serviceJob2_job_uptime_75_00_sec 0
> sla_mesos_test_serviceJob2_job_uptime_90_00_sec 0
> sla_mesos_test_serviceJob2_job_uptime_95_00_sec 0
> sla_mesos_test_serviceJob2_job_uptime_99_00_sec 0
> sla_mesos_test_serviceJob2_mtta 0.0
> sla_mesos_test_serviceJob2_mttr 0.0
> sla_mesos_test_serviceJob2_platform_uptime_percent 100.0
> sla_mesos_test_serviceJob4_job_uptime_75_00_sec 0
> sla_mesos_test_serviceJob4_job_uptime_90_00_sec 0
> sla_mesos_test_serviceJob4_job_uptime_95_00_sec 0
> sla_mesos_test_serviceJob4_job_uptime_99_00_sec 0
> sla_mesos_test_serviceJob4_mtta 0.0
> sla_mesos_test_serviceJob4_mttr 0.0
> sla_mesos_test_serviceJob4_platform_uptime_percent 100.0
> sla_mesos_test_serviceJob6_job_uptime_75_00_sec 7125
> sla_mesos_test_serviceJob6_job_uptime_90_00_sec 7125
> sla_mesos_test_serviceJob6_job_uptime_95_00_sec 7125
> sla_mesos_test_serviceJob6_job_uptime_99_00_sec 7125
> sla_mesos_test_serviceJob6_mtta 3271.493
> sla_mesos_test_serviceJob6_mttr 3273.497
> sla_mesos_test_serviceJob6_platform_uptime_percent 100.0
> sla_mesos_test_serviceJob8_job_uptime_75_00_sec 0
> sla_mesos_test_serviceJob8_job_uptime_90_00_sec 0
> sla_mesos_test_serviceJob8_job_uptime_95_00_sec 0
> sla_mesos_test_serviceJob8_job_uptime_99_00_sec 0
> sla_mesos_test_serviceJob8_mtta 0.0
> sla_mesos_test_serviceJob8_mttr 0.0
> sla_mesos_test_serviceJob8_platform_uptime_percent 100.0
> sla_ram_small_mtta 3271.493
> sla_ram_small_mttr 3273.497
> 
> 
> File Attachments
> ----------------
> 
> Coverage report
>   
> https://reviews.apache.org/media/uploaded/files/2014/04/16/ffe00c63-bb3a-4b90-95f8-f23878b0fdab__SLA_coverage_report.png
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to