Mailing-List: contact reviews-help@aurora.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.incubator.apache.org
Content-Type: multipart/alternative;
 boundary="===============3125101754013485081=="
MIME-Version: 1.0
Subject: Re: Review Request 21349: Starting SLA calculations on
 SchedulerActive event.
From: "Maxim Khutornenko" <maxim@apache.org>
To: "Bill Farner" <wfarner@apache.org>
Cc: "Aurora" <reviews@aurora.incubator.apache.org>,
 "Maxim Khutornenko" <maxim@apache.org>
Date: Wed, 14 May 2014 01:19:22 -0000
Message-ID: <20140514011922.16359.67501@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Maxim Khutornenko" <noreply@reviews.apache.org>
References: <20140514004208.16360.25312@reviews.apache.org>
In-Reply-To: <20140514004208.16360.25312@reviews.apache.org>
Reply-To: "Maxim Khutornenko" <maxim@apache.org>

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


> On May 14, 2014, 12:42 a.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java, line 198
> > <https://reviews.apache.org/r/21349/diff/2/?file=580984#file580984line198>
> >
> >     This is problematic in two ways - it synthetically slows the build by 5 seconds, and can never be sped up.  It also will be flaky depending on the behavior of the machine running tests.
> >     
> >     I'm actually not sure there's a clean way to cover this through SchedulerIT.  Ideally the test coverage is narrow enough to mock the ScheduledExecutorService and test without actually waiting for the delay.  I think this is doable within the sla module without too much plumbing (you just need to be able to inject the ScheduledExecutorService to SlaModule).
> 
> Maxim Khutornenko wrote:
>     This is actually 5 msec not seconds. I could set it even lower, just wanted to control the CPU load. Also, the awaitSlaReady() loops at 50 msec interval and bails out immediately after detecting a specified sla stat.
> 
> Bill Farner wrote:
>     Ah, my mistake.  The flakiness concern remains, though.  Making assumptions about thread scheduling is likely to fail at some point.

Not sure I get your point. The SchedulerIT is already heavily multithreaded in the way it detects the running scheduler (including the use of executors). The awaidSlaReady() does not kick in until all other checks are done and the scheduler is up. By that time, the SchedulerActive event is already fired and the SLA thread most likely done a few iterations (given the 5 msec interval). I'd say this test is quite safe given it's 10 sec timeout on the callable get.  


- Maxim


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


On May 13, 2014, 10:09 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21349/
> -----------------------------------------------------------
> 
> (Updated May 13, 2014, 10:09 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-410
>     https://issues.apache.org/jira/browse/AURORA-410
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Starting SLA calculations on SchedulerActive event.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 3dc15aded2743dd39ca307e1ab5589fa2e4507a3 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java bf3d7a36a575bb9d64f4dd851c63fbebda1e61b8 
>   src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java 13449ccae12bd906a4d948f4b4a83cf755eeca77 
>   src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java 8a0707ec2dfb695847d93a6e60adbbb488c22195 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java df5a5da120b754f7da5122feed99039c6ce7b613 
> 
> Diff: https://reviews.apache.org/r/21349/diff/
> 
> 
> Testing
> -------
> 
> gradle build
> gradle run
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>


--===============3125101754013485081==--