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