Mailing-List: contact reviews-help@aurora.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.apache.org
Content-Type: multipart/alternative;
 boundary="===============4517959474708167515=="
MIME-Version: 1.0
Subject: Re: Review Request 55357: AURORA-1867 Consider reserving for multiple
 tasks per preemption round
From: Aurora ReviewBot <wfarner@apache.org>
To: Joshua Cohen <jcohen@apache.org>, David McLaughlin <david@dmclaughlin.com>,
 Stephan Erb <serb@apache.org>, Zameer Manji <zmanji@apache.org>
Cc: Aurora ReviewBot <wfarner@apache.org>,
 Mehrdad Nurolahzade <mehrdad@nurolahzade.com>,
 Aurora <reviews@aurora.apache.org>
Date: Wed, 11 Jan 2017 16:55:49 -0000
Message-ID: <20170111165549.1677.79935@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: Aurora ReviewBot <noreply@reviews.apache.org>
References: <20170111164832.1677.29018@reviews.apache.org>
In-Reply-To: <20170111164832.1677.29018@reviews.apache.org>
Reply-To: Aurora ReviewBot <wfarner@apache.org>
archived-at: Wed, 11 Jan 2017 16:55:52 -0000

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


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


Master (a94601a) is red with this patch.
  ./build-support/jenkins/build.sh

	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.gradle.launcher.bootstrap.ProcessBootstrap.runNoExit(ProcessBootstrap.java:60)
	at org.gradle.launcher.bootstrap.ProcessBootstrap.run(ProcessBootstrap.java:37)
	at org.gradle.launcher.GradleMain.main(GradleMain.java:23)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.gradle.wrapper.BootstrapMainStarter.start(BootstrapMainStarter.java:30)
	at org.gradle.wrapper.WrapperExecutor.execute(WrapperExecutor.java:127)
	at org.gradle.wrapper.GradleWrapperMain.main(GradleWrapperMain.java:61)
:pmdTest
:testI0111 16:55:25.908 [ShutdownHook, SchedulerMain] Stopping scheduler services. 


org.apache.aurora.scheduler.preemptor.PendingTaskProcessorTest > testMultipleTaskGroups FAILED
    java.lang.AssertionError at PendingTaskProcessorTest.java:212

1070 tests completed, 1 failed, 2 skipped
:test FAILED
:jacocoTestReport
Coverage report generated: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/jacoco/test/html/index.html
:analyzeReport
Instruction coverage is 0.8883483057827495, but must be greater than 0.89
Branch coverage is 0.8006291781360598, but must be greater than 0.835

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':test'.
> There were failing tests. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/tests/test/index.html

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.

BUILD FAILED

Total time: 4 mins 4.058 secs


I will refresh this build result if you post a review containing "@ReviewBot retry"

- Aurora ReviewBot


On Jan. 11, 2017, 4:48 p.m., Mehrdad Nurolahzade wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55357/
> -----------------------------------------------------------
> 
> (Updated Jan. 11, 2017, 4:48 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Joshua Cohen, Stephan Erb, and Zameer Manji.
> 
> 
> Bugs: AURORA-1867
>     https://issues.apache.org/jira/browse/AURORA-1867
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> To be fair, PendingTaskProcessor interleaves tasks from different groups. However, this fairness comes at the price of increasing reservation time. Even if reservations are being made for the same task group, the processor would still restart iterating through slaves for each task instance. This results in reevaluating all slaves already rejected in a previous search before it finds a new viable candidate.
> 
> This patch improves `PendingTaskProcessor` performance by reducing slave search/evaluation time, at the cost of reduced fairness. `PendingTaskProcessor` now does reservation for a configurable maximum of _N_ candidates per task group in each iteration over the list of slaves.
> 
> 
> Diffs
> -----
> 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java fa37236e68657b539b182519b9d46d96d5b0953a 
>   src/main/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessor.java f59f3fd8959b1ba3726b55a2943fb9228a049ac5 
>   src/main/java/org/apache/aurora/scheduler/preemptor/PreemptorMetrics.java 67822cafbe89f4798b4ea6da3856663cc4872798 
>   src/main/java/org/apache/aurora/scheduler/preemptor/PreemptorModule.java 23d1c120657d5cb9d294a80c63e8a04512d361ca 
>   src/test/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessorTest.java d11ae5883f2a00dca4c4b36f0ab58ea95c7ecb2e 
>   src/test/java/org/apache/aurora/scheduler/preemptor/PreemptorModuleTest.java 67b6d69e3ddd1028dfe9ff451b171cd888674920 
> 
> Diff: https://reviews.apache.org/r/55357/diff/
> 
> 
> Testing
> -------
> 
> As is, the cluster setup in our existing preemption benchmark does not reflect the improvements resulting from this patch. Currently, all existing victims can be preempted, therefore all `PendingTaskProcessor` has to is look at the next slave.
> 
> ```
> BEFORE
> Benchmark                                                       (numPendingTasks)   Mode  Cnt   Score   Error  Units
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark                  1  thrpt   10  75.386 ± 2.984  ops/s
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark                 10  thrpt   10  74.584 ± 2.598  ops/s
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark                100  thrpt   10  79.731 ± 2.182  ops/s
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark               1000  thrpt   10  66.386 ± 1.833  ops/s
> 
> AFTER
> Benchmark                                                       (numPendingTasks)   Mode  Cnt   Score   Error  Units
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark                  1  thrpt   10  78.266 ± 3.290  ops/s
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark                 10  thrpt   10  76.743 ± 2.073  ops/s
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark                100  thrpt   10  75.343 ± 1.943  ops/s
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark               1000  thrpt   10  68.284 ± 2.413  ops/s
> ```
> 
> I need to further imprpve the cluster setup for this benchmark to reflect the improvements in the patch. A more representative cluster setup would be one in which only a subset of potential victims pass `PreemptionVictimFilter.filterPreemptionVictims()` test.
> 
> 
> Thanks,
> 
> Mehrdad Nurolahzade
> 
>


--===============4517959474708167515==--