Re: Review Request 66190: Fix 'PreemptorSlotSearchBenchmark', remove 'isProduction' references in benchmark

2018-03-21 Thread David McLaughlin 


> On March 21, 2018, 12:10 a.m., Jordan Ly wrote:
> > src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java
> > Lines 386-387 (original), 386-387 (patched)
> > 
> >
> > Changed these values to be a bit forward facing. I plan on adding a 
> > patch that reduces the number of hosts we look at when doing preemptions.

One (non-blocking) observation: wouldn't it best to keep the old values around, 
and add tests with the new settings too? That way you can measure performance 
of different algorithms for different distributions of tasks.


- David


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


On March 21, 2018, 12:26 a.m., Jordan Ly wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66190/
> ---
> 
> (Updated March 21, 2018, 12:26 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, and 
> Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This benchmark was using the deprecated `production` flag when building the 
> tasks for the cluster. `PendingTaskProcessor` depends on `tier` instead, so 
> this benchmark ended up not testing the correct codepath.
> 
> Removed references to `production` and added `tier` instead. Additionally, 
> removed some unused options.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/BenchmarkSettings.java 
> ddab2eccb22a93ecb67481f399707d2d82df5db2 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
> 1f9a5764b502f939f0345ff99fb0fc2830b4c2f0 
>   src/jmh/java/org/apache/aurora/benchmark/Tasks.java 
> 60c62bbf3061650a5dd8654045dc8189293d0190 
> 
> 
> Diff: https://reviews.apache.org/r/66190/diff/2/
> 
> 
> Testing
> ---
> 
> Old:
> ```
> # Run complete. Total time: 00:08:32
> 
> Benchmark 
>(numPendingTasks)   Mode  Cnt Score Error  
>  Units
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark
>1  thrpt   1057.670 ±  20.451  
>  ops/s
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.alloc.rate 
>1  thrpt   10   595.374 ± 211.805  
> MB/sec
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.alloc.rate.norm
>1  thrpt   10  10830342.916 ± 380.919
> B/op
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Eden_Space
>1  thrpt   10   593.530 ± 222.002  MB/sec
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Eden_Space.norm
>   1  thrpt   10  10717947.102 ± 1280229.296B/op
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Survivor_Space
>1  thrpt   10 0.305 ±   1.264  MB/sec
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Survivor_Space.norm
>   1  thrpt   10 13552.434 ±   61403.918B/op
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.count  
>1  thrpt   1060.000
> counts
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.time   
>1  thrpt   10   202.000
> ms
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·stack 
>1  thrptNaN
>---
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark
>   10  thrpt   1052.161 ±   8.526  
>  ops/s
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.alloc.rate 
>   10  thrpt   10   550.771 ±  89.939  
> MB/sec
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.alloc.rate.norm
>   10  thrpt   10  11074211.352 ± 318.376
> B/op
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Eden_Space
>   10  thrpt   10   550.125 ± 107.470  MB/sec
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Eden_Space.norm
>  10  thrpt   10  11073792.311 ± 1621636.993B/op
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Survivor_Space
>

Re: Review Request 66139: Speedup regular Thermos observer checkpoint refresh

2018-03-21 Thread Reza Motamedi 

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


Ship it!




Ship It!

- Reza Motamedi


On March 20, 2018, 10:41 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66139/
> ---
> 
> (Updated March 20, 2018, 10:41 p.m.)
> 
> 
> Review request for Aurora, Jordan Ly, Renan DelValle, and Reza Motamedi.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Profiling indicates that a significant part of the refresh time os spend in 
> `os.path.realpath`. 
> This was introduced in https://reviews.apache.org/r/35580/ to properly handle 
> the `latest`
> symlink in the Mesos folder layout. 
> 
> This patch takes a slightly different approach to solve this problem based on 
> `os.path.islink`.
> The latter is faster as it just needs to look at a single folder rather than 
> an entire path.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/path_detector.py 
> ed264d74ef5a5a7aa681a56b340f9b16504a88ad 
>   src/test/python/apache/aurora/executor/common/test_path_detector.py 
> 7b5ef0cf552d22d4cfbf3357071de036551026dc 
> 
> 
> Diff: https://reviews.apache.org/r/66139/diff/2/
> 
> 
> Testing
> ---
> 
> I have tested this build on a node with 55 running tasks and 2004 finished 
> ones.
> 
> Before this patch:
> 
> D0320 22:20:44.887248 25771 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.92s
> D0320 22:20:50.746316 25771 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.93s
> D0320 22:20:56.590157 25771 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.89s
>  
> With this patch:
> 
> D0320 22:18:53.545236 16250 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.48s
> D0320 22:18:59.031919 16250 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.49s
> D0320 22:19:04.512358 16250 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.48s
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 66139: Speedup regular Thermos observer checkpoint refresh

2018-03-21 Thread Reza Motamedi 


> On March 21, 2018, 5:36 p.m., Reza Motamedi wrote:
> > Ship It!

Can you explain a bit more about the exeperiement settings?


- Reza


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


On March 20, 2018, 10:41 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66139/
> ---
> 
> (Updated March 20, 2018, 10:41 p.m.)
> 
> 
> Review request for Aurora, Jordan Ly, Renan DelValle, and Reza Motamedi.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Profiling indicates that a significant part of the refresh time os spend in 
> `os.path.realpath`. 
> This was introduced in https://reviews.apache.org/r/35580/ to properly handle 
> the `latest`
> symlink in the Mesos folder layout. 
> 
> This patch takes a slightly different approach to solve this problem based on 
> `os.path.islink`.
> The latter is faster as it just needs to look at a single folder rather than 
> an entire path.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/path_detector.py 
> ed264d74ef5a5a7aa681a56b340f9b16504a88ad 
>   src/test/python/apache/aurora/executor/common/test_path_detector.py 
> 7b5ef0cf552d22d4cfbf3357071de036551026dc 
> 
> 
> Diff: https://reviews.apache.org/r/66139/diff/2/
> 
> 
> Testing
> ---
> 
> I have tested this build on a node with 55 running tasks and 2004 finished 
> ones.
> 
> Before this patch:
> 
> D0320 22:20:44.887248 25771 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.92s
> D0320 22:20:50.746316 25771 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.93s
> D0320 22:20:56.590157 25771 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.89s
>  
> With this patch:
> 
> D0320 22:18:53.545236 16250 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.48s
> D0320 22:18:59.031919 16250 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.49s
> D0320 22:19:04.512358 16250 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.48s
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 66103: Introduce mesos disk collector

2018-03-21 Thread Santhosh Kumar Shanmugham 

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




src/main/python/apache/aurora/tools/thermos_observer.py
Lines 89 (patched)


The agent's HTTP endpoints can have AuthN/AuthZ enabled. We can either add 
an option to specify the credentials file to be used while talking to the 
endpoint or we can call this out as limitation for now.


- Santhosh Kumar Shanmugham


On March 20, 2018, 10:20 a.m., Reza Motamedi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66103/
> ---
> 
> (Updated March 20, 2018, 10:20 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Daniel Knightly, Jordan Ly, 
> Santhosh Kumar Shanmugham, and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> When disk isolation is enabled in a Mesos agent it calculates the disk usage 
> for each container. 
> Thermos Observer also monitors disk usage using `twitter.common.dirutil`, 
> essentially repeating the work already done by the agent. In practice, we see 
> that disk monitoring is one of the most expensive resource monitoring tasks. 
> For instance, when there are deeply nested directories, the CPU utilization 
> of the observer process can easily reach 1.5 CPUs. It would be ideal if we 
> delegate the disk monitoring task to the agent and do it only once. With this 
> approach, when disk collection has improved in the agent (for instance by 
> implementing XFS isolation), we can simply benefit from it without any code 
> change. Some more information about the problem is provided in AURORA-1918.
> 
> This patch that introduces `MesosDiskCollector` which queries the agent's API 
> endpoint to lookup disk_used_bytes. Note that there is also resource 
> monitoring in thermos executor. Currently, I left the disk collector there to 
> use the `du` implementation. That can be changed in a later patch.
> 
> I modified some vagrant config files including `aurora-executor.service` and 
> `etc_mesos-slave/isolation` for testing. They can be left as is. I included 
> them in this patch to show how this would work e2e.
> 
> 
> Diffs
> -
> 
>   3rdparty/python/requirements.txt 4ac242cfa2c1c19cb7447816ab86e748839d3d11 
>   examples/jobs/hello_world.aurora 5401bfebe753b5e53abd08baeac501144ced9b5a 
>   examples/vagrant/mesos_config/etc_mesos-slave/isolation 
> 1a7028ffc70116b104ef3ad22b7388f637707a0f 
>   examples/vagrant/systemd/thermos.service 
> 01925bcd2ae44f100df511f3c3951c3f5a1a72aa 
>   src/main/python/apache/aurora/tools/thermos_observer.py 
> dd9f0c46ceac9e939b1b763073314161de0ea614 
>   src/main/python/apache/thermos/monitoring/BUILD 
> 65ba7088f65e7baa5d30744736ba456b46a55e86 
>   src/main/python/apache/thermos/monitoring/disk.py 
> 986d33a5000f8d5db15cb639c81f8b1d756ffa05 
>   src/main/python/apache/thermos/monitoring/resource.py 
> adcdc751c03460dc801a18278faa96d6bd64722b 
>   src/main/python/apache/thermos/observer/task_observer.py 
> a6870d48bddf2a2ccede7bb68195f2baae1d0e47 
>   
> src/test/python/apache/aurora/executor/common/test_resource_manager_integration.py
>  fe74bd1d3ecd89fca1b5b2251202cbbc0f24 
>   src/test/python/apache/thermos/monitoring/BUILD 
> 8f2b39336dce6c7b580e6ba0009f60afdcb89179 
>   src/test/python/apache/thermos/monitoring/test_disk.py 
> 362393bfd1facf3198e2d438d0596b16700b72b8 
>   src/test/python/apache/thermos/monitoring/test_resource.py 
> e577e552d4ee1807096a15401851bb9fd95fa426 
> 
> 
> Diff: https://reviews.apache.org/r/66103/diff/3/
> 
> 
> Testing
> ---
> 
> I added unit tests.
> Tested in vagrant and it works as intenced.
> I also built and deployed in our test enviroment. In order to measure 
> imporoved performance I created jobs with nested folders and noticed 
> reduction in CPU utilization of the Observer process, by at least 60%. (1.5 
> CPU cores to 0.4 CPU cores)
> 
> 
> Thanks,
> 
> Reza Motamedi
> 
>

Re: Review Request 66139: Speedup regular Thermos observer checkpoint refresh

2018-03-21 Thread Santhosh Kumar Shanmugham 

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


Ship it!




Ship It!

- Santhosh Kumar Shanmugham


On March 20, 2018, 3:41 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66139/
> ---
> 
> (Updated March 20, 2018, 3:41 p.m.)
> 
> 
> Review request for Aurora, Jordan Ly, Renan DelValle, and Reza Motamedi.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Profiling indicates that a significant part of the refresh time os spend in 
> `os.path.realpath`. 
> This was introduced in https://reviews.apache.org/r/35580/ to properly handle 
> the `latest`
> symlink in the Mesos folder layout. 
> 
> This patch takes a slightly different approach to solve this problem based on 
> `os.path.islink`.
> The latter is faster as it just needs to look at a single folder rather than 
> an entire path.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/path_detector.py 
> ed264d74ef5a5a7aa681a56b340f9b16504a88ad 
>   src/test/python/apache/aurora/executor/common/test_path_detector.py 
> 7b5ef0cf552d22d4cfbf3357071de036551026dc 
> 
> 
> Diff: https://reviews.apache.org/r/66139/diff/2/
> 
> 
> Testing
> ---
> 
> I have tested this build on a node with 55 running tasks and 2004 finished 
> ones.
> 
> Before this patch:
> 
> D0320 22:20:44.887248 25771 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.92s
> D0320 22:20:50.746316 25771 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.93s
> D0320 22:20:56.590157 25771 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.89s
>  
> With this patch:
> 
> D0320 22:18:53.545236 16250 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.48s
> D0320 22:18:59.031919 16250 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.49s
> D0320 22:19:04.512358 16250 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.48s
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 66139: Speedup regular Thermos observer checkpoint refresh

2018-03-21 Thread Santhosh Kumar Shanmugham 


> On March 21, 2018, 10:47 a.m., Santhosh Kumar Shanmugham wrote:
> > Ship It!

I am a little confused about the performance testing results that are posted 
here, since the pervious results indicated gains from 2secs to 0.2secs, while 
the current one is much lesser. Can you add a little bit of context regarding 
the results? Thanks.


- Santhosh Kumar


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


On March 20, 2018, 3:41 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66139/
> ---
> 
> (Updated March 20, 2018, 3:41 p.m.)
> 
> 
> Review request for Aurora, Jordan Ly, Renan DelValle, and Reza Motamedi.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Profiling indicates that a significant part of the refresh time os spend in 
> `os.path.realpath`. 
> This was introduced in https://reviews.apache.org/r/35580/ to properly handle 
> the `latest`
> symlink in the Mesos folder layout. 
> 
> This patch takes a slightly different approach to solve this problem based on 
> `os.path.islink`.
> The latter is faster as it just needs to look at a single folder rather than 
> an entire path.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/path_detector.py 
> ed264d74ef5a5a7aa681a56b340f9b16504a88ad 
>   src/test/python/apache/aurora/executor/common/test_path_detector.py 
> 7b5ef0cf552d22d4cfbf3357071de036551026dc 
> 
> 
> Diff: https://reviews.apache.org/r/66139/diff/2/
> 
> 
> Testing
> ---
> 
> I have tested this build on a node with 55 running tasks and 2004 finished 
> ones.
> 
> Before this patch:
> 
> D0320 22:20:44.887248 25771 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.92s
> D0320 22:20:50.746316 25771 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.93s
> D0320 22:20:56.590157 25771 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.89s
>  
> With this patch:
> 
> D0320 22:18:53.545236 16250 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.48s
> D0320 22:18:59.031919 16250 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.49s
> D0320 22:19:04.512358 16250 task_observer.py:142] TaskObserver: finished 
> checkpoint refresh in 0.48s
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 66190: Fix 'PreemptorSlotSearchBenchmark', remove 'isProduction' references in benchmark

2018-03-21 Thread Jordan Ly 

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

(Updated March 21, 2018, 8:23 p.m.)


Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, and 
Stephan Erb.


Changes
---

Change cluster allocation values to previous values.


Repository: aurora


Description
---

This benchmark was using the deprecated `production` flag when building the 
tasks for the cluster. `PendingTaskProcessor` depends on `tier` instead, so 
this benchmark ended up not testing the correct codepath.

Removed references to `production` and added `tier` instead. Additionally, 
removed some unused options.


Diffs (updated)
-

  src/jmh/java/org/apache/aurora/benchmark/BenchmarkSettings.java 
ddab2eccb22a93ecb67481f399707d2d82df5db2 
  src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
1f9a5764b502f939f0345ff99fb0fc2830b4c2f0 
  src/jmh/java/org/apache/aurora/benchmark/Tasks.java 
60c62bbf3061650a5dd8654045dc8189293d0190 
  src/main/java/org/apache/aurora/scheduler/http/State.java 
5b3b7c52fb3623c45518d27b242527ce0e775e06 
  src/main/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessor.java 
ef06471d007b1d36300eea30cdea059c1ba231b0 
  src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictim.java 
780689e1b8967a9f558a464271b7b60035dce1a7 
  
src/main/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilter.java 
569cfe6b04e6b7bf0dca7625b00698e9d8e47daf 
  src/main/java/org/apache/aurora/scheduler/preemptor/Preemptor.java 
293d106eee383dd5352a629780b897d58c9dd439 
  src/main/java/org/apache/aurora/scheduler/state/ClusterState.java 
527cfd6bd7d55ffb526919ea772a0947ea3e958f 
  src/main/java/org/apache/aurora/scheduler/state/ClusterStateImpl.java 
d804198f5b3c0f835997af6693360b8376ccd865 
  src/test/java/org/apache/aurora/scheduler/http/StateTest.java 
852c2f8c1cf23c6f448e78d476608a829b786751 
  src/test/java/org/apache/aurora/scheduler/preemptor/ClusterStateImplTest.java 
84987212be184edfe801278213a8b2bb0c49db9e 
  
src/test/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessorTest.java
 ba775f4688dc57504e2def0dc4b5dcd00da448e1 


Diff: https://reviews.apache.org/r/66190/diff/3/

Changes: https://reviews.apache.org/r/66190/diff/2-3/


Testing
---

Old:
```
# Run complete. Total time: 00:08:32

Benchmark   
 (numPendingTasks)   Mode  Cnt Score Error   
Units
SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark  
 1  thrpt   1057.670 ±  20.451   
ops/s
SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.alloc.rate   
 1  thrpt   10   595.374 ± 211.805  
MB/sec
SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.alloc.rate.norm
   1  thrpt   10  10830342.916 ± 380.919B/op
SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Eden_Space
   1  thrpt   10   593.530 ± 222.002  MB/sec
SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Eden_Space.norm
  1  thrpt   10  10717947.102 ± 1280229.296B/op
SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Survivor_Space
   1  thrpt   10 0.305 ±   1.264  MB/sec
SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Survivor_Space.norm
  1  thrpt   10 13552.434 ±   61403.918B/op
SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.count
 1  thrpt   1060.000
counts
SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.time 
 1  thrpt   10   202.000
ms
SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·stack   
 1  thrptNaN   
---
SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark  
10  thrpt   1052.161 ±   8.526   
ops/s
SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.alloc.rate   
10  thrpt   10   550.771 ±  89.939  
MB/sec
SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.alloc.rate.norm
  10  thrpt   10  11074211.352 ± 318.376B/op
SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Eden_Space
  10  thrpt   10   550.125 ± 107.470  MB/sec
SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.ch

Re: Review Request 66190: Fix 'PreemptorSlotSearchBenchmark', remove 'isProduction' references in benchmark

2018-03-21 Thread Jordan Ly 


> On March 21, 2018, 12:10 a.m., Jordan Ly wrote:
> > src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java
> > Lines 386-387 (original), 386-387 (patched)
> > 
> >
> > Changed these values to be a bit forward facing. I plan on adding a 
> > patch that reduces the number of hosts we look at when doing preemptions.
> 
> David McLaughlin wrote:
> One (non-blocking) observation: wouldn't it best to keep the old values 
> around, and add tests with the new settings too? That way you can measure 
> performance of different algorithms for different distributions of tasks.

That seems better. I reverted the values to the existing values for this patch, 
and will add tests for the new benchmarks later.


- Jordan


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


On March 21, 2018, 8:23 p.m., Jordan Ly wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66190/
> ---
> 
> (Updated March 21, 2018, 8:23 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, and 
> Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This benchmark was using the deprecated `production` flag when building the 
> tasks for the cluster. `PendingTaskProcessor` depends on `tier` instead, so 
> this benchmark ended up not testing the correct codepath.
> 
> Removed references to `production` and added `tier` instead. Additionally, 
> removed some unused options.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/BenchmarkSettings.java 
> ddab2eccb22a93ecb67481f399707d2d82df5db2 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
> 1f9a5764b502f939f0345ff99fb0fc2830b4c2f0 
>   src/jmh/java/org/apache/aurora/benchmark/Tasks.java 
> 60c62bbf3061650a5dd8654045dc8189293d0190 
> 
> 
> Diff: https://reviews.apache.org/r/66190/diff/4/
> 
> 
> Testing
> ---
> 
> Old:
> ```
> # Run complete. Total time: 00:08:32
> 
> Benchmark 
>(numPendingTasks)   Mode  Cnt Score Error  
>  Units
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark
>1  thrpt   1057.670 ±  20.451  
>  ops/s
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.alloc.rate 
>1  thrpt   10   595.374 ± 211.805  
> MB/sec
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.alloc.rate.norm
>1  thrpt   10  10830342.916 ± 380.919
> B/op
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Eden_Space
>1  thrpt   10   593.530 ± 222.002  MB/sec
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Eden_Space.norm
>   1  thrpt   10  10717947.102 ± 1280229.296B/op
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Survivor_Space
>1  thrpt   10 0.305 ±   1.264  MB/sec
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Survivor_Space.norm
>   1  thrpt   10 13552.434 ±   61403.918B/op
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.count  
>1  thrpt   1060.000
> counts
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.time   
>1  thrpt   10   202.000
> ms
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·stack 
>1  thrptNaN
>---
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark
>   10  thrpt   1052.161 ±   8.526  
>  ops/s
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.alloc.rate 
>   10  thrpt   10   550.771 ±  89.939  
> MB/sec
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.alloc.rate.norm
>   10  thrpt   10  11074211.352 ± 318.376
> B/op
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Eden_Space
>   10  thrpt   10   550.125 ± 107.470  MB/sec
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Eden_Space.norm
>

Re: Review Request 66190: Fix 'PreemptorSlotSearchBenchmark', remove 'isProduction' references in benchmark

2018-03-21 Thread Aurora ReviewBot 

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


Ship it!




Master (f32086d) is green with this patch.
  ./build-support/jenkins/build.sh

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

- Aurora ReviewBot


On March 21, 2018, 8:23 p.m., Jordan Ly wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66190/
> ---
> 
> (Updated March 21, 2018, 8:23 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, and 
> Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This benchmark was using the deprecated `production` flag when building the 
> tasks for the cluster. `PendingTaskProcessor` depends on `tier` instead, so 
> this benchmark ended up not testing the correct codepath.
> 
> Removed references to `production` and added `tier` instead. Additionally, 
> removed some unused options.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/BenchmarkSettings.java 
> ddab2eccb22a93ecb67481f399707d2d82df5db2 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
> 1f9a5764b502f939f0345ff99fb0fc2830b4c2f0 
>   src/jmh/java/org/apache/aurora/benchmark/Tasks.java 
> 60c62bbf3061650a5dd8654045dc8189293d0190 
> 
> 
> Diff: https://reviews.apache.org/r/66190/diff/4/
> 
> 
> Testing
> ---
> 
> Old:
> ```
> # Run complete. Total time: 00:08:32
> 
> Benchmark 
>(numPendingTasks)   Mode  Cnt Score Error  
>  Units
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark
>1  thrpt   1057.670 ±  20.451  
>  ops/s
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.alloc.rate 
>1  thrpt   10   595.374 ± 211.805  
> MB/sec
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.alloc.rate.norm
>1  thrpt   10  10830342.916 ± 380.919
> B/op
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Eden_Space
>1  thrpt   10   593.530 ± 222.002  MB/sec
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Eden_Space.norm
>   1  thrpt   10  10717947.102 ± 1280229.296B/op
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Survivor_Space
>1  thrpt   10 0.305 ±   1.264  MB/sec
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Survivor_Space.norm
>   1  thrpt   10 13552.434 ±   61403.918B/op
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.count  
>1  thrpt   1060.000
> counts
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.time   
>1  thrpt   10   202.000
> ms
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·stack 
>1  thrptNaN
>---
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark
>   10  thrpt   1052.161 ±   8.526  
>  ops/s
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.alloc.rate 
>   10  thrpt   10   550.771 ±  89.939  
> MB/sec
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.alloc.rate.norm
>   10  thrpt   10  11074211.352 ± 318.376
> B/op
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Eden_Space
>   10  thrpt   10   550.125 ± 107.470  MB/sec
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Eden_Space.norm
>  10  thrpt   10  11073792.311 ± 1621636.993B/op
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Survivor_Space
>   10  thrpt   10 0.038 ±   0.049  MB/sec
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.churn.PS_Survivor_Space.norm
>  10  thrpt   10   737.753 ± 919.460B/op
> SchedulingBenchmarks.PreemptorSlotSearchBenchmark.runBenchmark:·gc.count  
>   10  thrpt   1055.000
> counts
> SchedulingBenchmarks.PreemptorSlotSearch

Re: Review Request 66192: [WIP] Variable group size updates

2018-03-21 Thread Jordan Ly 

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



I am mostly concerned about the UX. Users will be able to specify both batch 
size and variable batch size and must know that variable batch sizing takes 
precedent over other strategies.

Is it worth it to make a larger investment into the Thrift interface and avoid 
ambiguity? Or refactor the current batching strategy to use the new variable 
codepath (a single batch size specified to the variable strategy should behave 
the same as the current implementation).

- Jordan Ly


On March 21, 2018, 2:10 a.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66192/
> ---
> 
> (Updated March 21, 2018, 2:10 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, and 
> Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding support for variable group sizes when executing an update.
> 
> Design doc for this change is here: 
> https://docs.google.com/document/d/1xGk4ueH8YlmJCk6hQJh85u4to4M1VQD0l630IOchvgY/edit#heading=h.lg3hty82f5cz
> 
> I opted for the path of least resistance with regards to the Thrift changes 
> as I didn't see any benefit in making the larger changes required to make the 
> interfaces a bit more flexible.
> 
> Requesting feedback on these changes and the approach from the community 
> before I proceed.
> 
> Tests will be added after the community approves of the direciton and 
> approach.
> 
> Note to reviewers: Changes made in ActiveLimitedStrategy.java were made to 
> move towards getting rid of FluentIterable. I figured since I was touching 
> that code, it wouldn't hurt to test the Java 8 equivalent of it. I can get 
> rid of the change here and make it in a separate patch if desired.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> ef754e32172e7490a47a13e7b526f243ffa3efeb 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> b79e2045ccda05d5058565f81988dfe33feea8f1 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> b25f3831cecc58c90375c90b16142421f8f09e38 
>   src/main/java/org/apache/aurora/scheduler/storage/durability/Loader.java 
> 10864f122eff5027c88d835baae6de483d960218 
>   
> src/main/java/org/apache/aurora/scheduler/storage/durability/WriteRecorder.java
>  8d70cae35289a9e36142bab288cf0c9398ebd2d4 
>   
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemJobUpdateStore.java 
> 9e86b9e276ea90a249284a824705b5bbf19dcbce 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  9fc0416086dd3eb2e2f4e8f659da59fcdea2b22b 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  f8be8058f3a80a18b999d2666e2adb33e1e55fef 
>   src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 
> 3992aa77fc305adc390a4aaeb1d3939d6241ddbd 
>   
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java
>  855ea9c20788b51695b7eff5ac0970f0d52a9546 
>   
> src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/66192/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Renan DelValle
> 
>

Review Request 66199: Remove unused LOST_LOCK_MESSAGE variable in JobUpdateControllerImpl

2018-03-21 Thread Jordan Ly 

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

Review request for Aurora, Renan DelValle and Stephan Erb.


Repository: aurora


Description
---

We no longer use locks for updates (context: 
https://reviews.apache.org/r/63130/). This was a legacy variable.


Diffs
-

  
src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 
f8be8058f3a80a18b999d2666e2adb33e1e55fef 


Diff: https://reviews.apache.org/r/66199/diff/1/


Testing
---

`./gradlew test`


Thanks,

Jordan Ly

Re: Review Request 66199: Remove unused LOST_LOCK_MESSAGE variable in JobUpdateControllerImpl

2018-03-21 Thread David McLaughlin 

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


Ship it!




Ship It!

- David McLaughlin


On March 21, 2018, 9:38 p.m., Jordan Ly wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66199/
> ---
> 
> (Updated March 21, 2018, 9:38 p.m.)
> 
> 
> Review request for Aurora, Renan DelValle and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> We no longer use locks for updates (context: 
> https://reviews.apache.org/r/63130/). This was a legacy variable.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  f8be8058f3a80a18b999d2666e2adb33e1e55fef 
> 
> 
> Diff: https://reviews.apache.org/r/66199/diff/1/
> 
> 
> Testing
> ---
> 
> `./gradlew test`
> 
> 
> Thanks,
> 
> Jordan Ly
> 
>

Re: Review Request 66199: Remove unused LOST_LOCK_MESSAGE variable in JobUpdateControllerImpl

2018-03-21 Thread Renan DelValle 

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


Ship it!




Ship It!

- Renan DelValle


On March 21, 2018, 2:38 p.m., Jordan Ly wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66199/
> ---
> 
> (Updated March 21, 2018, 2:38 p.m.)
> 
> 
> Review request for Aurora, Renan DelValle and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> We no longer use locks for updates (context: 
> https://reviews.apache.org/r/63130/). This was a legacy variable.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  f8be8058f3a80a18b999d2666e2adb33e1e55fef 
> 
> 
> Diff: https://reviews.apache.org/r/66199/diff/1/
> 
> 
> Testing
> ---
> 
> `./gradlew test`
> 
> 
> Thanks,
> 
> Jordan Ly
> 
>

Re: Review Request 66192: [WIP] Variable group size updates

2018-03-21 Thread Santhosh Kumar Shanmugham 


> On March 21, 2018, 2:33 p.m., Jordan Ly wrote:
> > I am mostly concerned about the UX. Users will be able to specify both 
> > batch size and variable batch size and must know that variable batch sizing 
> > takes precedent over other strategies.
> > 
> > Is it worth it to make a larger investment into the Thrift interface and 
> > avoid ambiguity? Or refactor the current batching strategy to use the new 
> > variable codepath (a single batch size specified to the variable strategy 
> > should behave the same as the current implementation).

+1 I think it should be an either-or. There should be logic in the API to 
clearly message this case.


- Santhosh Kumar


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


On March 20, 2018, 7:10 p.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66192/
> ---
> 
> (Updated March 20, 2018, 7:10 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, and 
> Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding support for variable group sizes when executing an update.
> 
> Design doc for this change is here: 
> https://docs.google.com/document/d/1xGk4ueH8YlmJCk6hQJh85u4to4M1VQD0l630IOchvgY/edit#heading=h.lg3hty82f5cz
> 
> I opted for the path of least resistance with regards to the Thrift changes 
> as I didn't see any benefit in making the larger changes required to make the 
> interfaces a bit more flexible.
> 
> Requesting feedback on these changes and the approach from the community 
> before I proceed.
> 
> Tests will be added after the community approves of the direciton and 
> approach.
> 
> Note to reviewers: Changes made in ActiveLimitedStrategy.java were made to 
> move towards getting rid of FluentIterable. I figured since I was touching 
> that code, it wouldn't hurt to test the Java 8 equivalent of it. I can get 
> rid of the change here and make it in a separate patch if desired.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> ef754e32172e7490a47a13e7b526f243ffa3efeb 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> b79e2045ccda05d5058565f81988dfe33feea8f1 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> b25f3831cecc58c90375c90b16142421f8f09e38 
>   src/main/java/org/apache/aurora/scheduler/storage/durability/Loader.java 
> 10864f122eff5027c88d835baae6de483d960218 
>   
> src/main/java/org/apache/aurora/scheduler/storage/durability/WriteRecorder.java
>  8d70cae35289a9e36142bab288cf0c9398ebd2d4 
>   
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemJobUpdateStore.java 
> 9e86b9e276ea90a249284a824705b5bbf19dcbce 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  9fc0416086dd3eb2e2f4e8f659da59fcdea2b22b 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  f8be8058f3a80a18b999d2666e2adb33e1e55fef 
>   src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 
> 3992aa77fc305adc390a4aaeb1d3939d6241ddbd 
>   
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java
>  855ea9c20788b51695b7eff5ac0970f0d52a9546 
>   
> src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/66192/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Renan DelValle
> 
>

Re: Review Request 66192: [WIP] Variable group size updates

2018-03-21 Thread Santhosh Kumar Shanmugham 


> On March 20, 2018, 9:29 p.m., David McLaughlin wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java
> > Lines 107-111 (patched)
> > 
> >
> > Is this idea of a step sate needed? Strategies writing to the database 
> > seems like a design smell. 
> > 
> > If I have 10 instances:
> > 
> > [0,1,2,3,4,5,6,7,8,9] 
> > 
> > And I pass a VariableBatchStrategy of: [1, 4, 5]
> > 
> > Then using the BatchStrategy it is *always* in the following order:
> > 
> > [[0], [1,2,3,4], [5,6,7,8,9]]
> > 
> > And I don't *think* I need to write the step to the database to figure 
> > out which step I'm in? I can use the number idle and active to figure out 
> > exactly how many have been processed.
> 
> Jordan Ly wrote:
> +1, removing the need for persistent state would greatly reduce the 
> surface area of this patch.

+1


- Santhosh Kumar


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


On March 20, 2018, 7:10 p.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66192/
> ---
> 
> (Updated March 20, 2018, 7:10 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, and 
> Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding support for variable group sizes when executing an update.
> 
> Design doc for this change is here: 
> https://docs.google.com/document/d/1xGk4ueH8YlmJCk6hQJh85u4to4M1VQD0l630IOchvgY/edit#heading=h.lg3hty82f5cz
> 
> I opted for the path of least resistance with regards to the Thrift changes 
> as I didn't see any benefit in making the larger changes required to make the 
> interfaces a bit more flexible.
> 
> Requesting feedback on these changes and the approach from the community 
> before I proceed.
> 
> Tests will be added after the community approves of the direciton and 
> approach.
> 
> Note to reviewers: Changes made in ActiveLimitedStrategy.java were made to 
> move towards getting rid of FluentIterable. I figured since I was touching 
> that code, it wouldn't hurt to test the Java 8 equivalent of it. I can get 
> rid of the change here and make it in a separate patch if desired.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> ef754e32172e7490a47a13e7b526f243ffa3efeb 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> b79e2045ccda05d5058565f81988dfe33feea8f1 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> b25f3831cecc58c90375c90b16142421f8f09e38 
>   src/main/java/org/apache/aurora/scheduler/storage/durability/Loader.java 
> 10864f122eff5027c88d835baae6de483d960218 
>   
> src/main/java/org/apache/aurora/scheduler/storage/durability/WriteRecorder.java
>  8d70cae35289a9e36142bab288cf0c9398ebd2d4 
>   
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemJobUpdateStore.java 
> 9e86b9e276ea90a249284a824705b5bbf19dcbce 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  9fc0416086dd3eb2e2f4e8f659da59fcdea2b22b 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  f8be8058f3a80a18b999d2666e2adb33e1e55fef 
>   src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 
> 3992aa77fc305adc390a4aaeb1d3939d6241ddbd 
>   
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java
>  855ea9c20788b51695b7eff5ac0970f0d52a9546 
>   
> src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/66192/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Renan DelValle
> 
>

Re: Review Request 66199: Remove unused LOST_LOCK_MESSAGE variable in JobUpdateControllerImpl

2018-03-21 Thread Aurora ReviewBot 

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



Master (f32086d) is green with this patch.
  ./build-support/jenkins/build.sh

However, it appears that it might lack test coverage.

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

- Aurora ReviewBot


On March 21, 2018, 2:38 p.m., Jordan Ly wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66199/
> ---
> 
> (Updated March 21, 2018, 2:38 p.m.)
> 
> 
> Review request for Aurora, Renan DelValle and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> We no longer use locks for updates (context: 
> https://reviews.apache.org/r/63130/). This was a legacy variable.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  f8be8058f3a80a18b999d2666e2adb33e1e55fef 
> 
> 
> Diff: https://reviews.apache.org/r/66199/diff/1/
> 
> 
> Testing
> ---
> 
> `./gradlew test`
> 
> 
> Thanks,
> 
> Jordan Ly
> 
>

Re: Review Request 66192: [WIP] Variable group size updates

2018-03-21 Thread Renan DelValle 


> On March 21, 2018, 2:33 p.m., Jordan Ly wrote:
> > I am mostly concerned about the UX. Users will be able to specify both 
> > batch size and variable batch size and must know that variable batch sizing 
> > takes precedent over other strategies.
> > 
> > Is it worth it to make a larger investment into the Thrift interface and 
> > avoid ambiguity? Or refactor the current batching strategy to use the new 
> > variable codepath (a single batch size specified to the variable strategy 
> > should behave the same as the current implementation).
> 
> Santhosh Kumar Shanmugham wrote:
> +1 I think it should be an either-or. There should be logic in the API to 
> clearly message this case.

I've thought about refactoring the batching strategy, and I'm willing to travel 
down that path as the current batch stategy is a specific case (single step, 
repeated over and over as Jordan pointed out) of the functionality that I'm 
trying to achieve with this patch but I'll have to do something like wrap 
around a single item list which might look funky. I'll give it a shot.

Changing the Thrift interface is always tricky because it almost always results 
in some yak shaving but if the consensus is that this is the better, more clear 
approach, then I'm game to implement it that way.

Appreciate all the feedback!


- Renan


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


On March 20, 2018, 7:10 p.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66192/
> ---
> 
> (Updated March 20, 2018, 7:10 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, and 
> Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding support for variable group sizes when executing an update.
> 
> Design doc for this change is here: 
> https://docs.google.com/document/d/1xGk4ueH8YlmJCk6hQJh85u4to4M1VQD0l630IOchvgY/edit#heading=h.lg3hty82f5cz
> 
> I opted for the path of least resistance with regards to the Thrift changes 
> as I didn't see any benefit in making the larger changes required to make the 
> interfaces a bit more flexible.
> 
> Requesting feedback on these changes and the approach from the community 
> before I proceed.
> 
> Tests will be added after the community approves of the direciton and 
> approach.
> 
> Note to reviewers: Changes made in ActiveLimitedStrategy.java were made to 
> move towards getting rid of FluentIterable. I figured since I was touching 
> that code, it wouldn't hurt to test the Java 8 equivalent of it. I can get 
> rid of the change here and make it in a separate patch if desired.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> ef754e32172e7490a47a13e7b526f243ffa3efeb 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> b79e2045ccda05d5058565f81988dfe33feea8f1 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> b25f3831cecc58c90375c90b16142421f8f09e38 
>   src/main/java/org/apache/aurora/scheduler/storage/durability/Loader.java 
> 10864f122eff5027c88d835baae6de483d960218 
>   
> src/main/java/org/apache/aurora/scheduler/storage/durability/WriteRecorder.java
>  8d70cae35289a9e36142bab288cf0c9398ebd2d4 
>   
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemJobUpdateStore.java 
> 9e86b9e276ea90a249284a824705b5bbf19dcbce 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  9fc0416086dd3eb2e2f4e8f659da59fcdea2b22b 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  f8be8058f3a80a18b999d2666e2adb33e1e55fef 
>   src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 
> 3992aa77fc305adc390a4aaeb1d3939d6241ddbd 
>   
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java
>  855ea9c20788b51695b7eff5ac0970f0d52a9546 
>   
> src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/66192/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Renan DelValle
> 
>

Re: Review Request 66103: Introduce mesos disk collector

2018-03-21 Thread Reza Motamedi 


> On March 21, 2018, 5:39 p.m., Santhosh Kumar Shanmugham wrote:
> > src/main/python/apache/aurora/tools/thermos_observer.py
> > Lines 89 (patched)
> > 
> >
> > The agent's HTTP endpoints can have AuthN/AuthZ enabled. We can either 
> > add an option to specify the credentials file to be used while talking to 
> > the endpoint or we can call this out as limitation for now.

Good point. I updated the help message for `--enable_mesos_disk_collector` arg 
to make this incompatibility clear.


- Reza


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


On March 20, 2018, 5:20 p.m., Reza Motamedi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66103/
> ---
> 
> (Updated March 20, 2018, 5:20 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Daniel Knightly, Jordan Ly, 
> Santhosh Kumar Shanmugham, and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> When disk isolation is enabled in a Mesos agent it calculates the disk usage 
> for each container. 
> Thermos Observer also monitors disk usage using `twitter.common.dirutil`, 
> essentially repeating the work already done by the agent. In practice, we see 
> that disk monitoring is one of the most expensive resource monitoring tasks. 
> For instance, when there are deeply nested directories, the CPU utilization 
> of the observer process can easily reach 1.5 CPUs. It would be ideal if we 
> delegate the disk monitoring task to the agent and do it only once. With this 
> approach, when disk collection has improved in the agent (for instance by 
> implementing XFS isolation), we can simply benefit from it without any code 
> change. Some more information about the problem is provided in AURORA-1918.
> 
> This patch that introduces `MesosDiskCollector` which queries the agent's API 
> endpoint to lookup disk_used_bytes. Note that there is also resource 
> monitoring in thermos executor. Currently, I left the disk collector there to 
> use the `du` implementation. That can be changed in a later patch.
> 
> I modified some vagrant config files including `aurora-executor.service` and 
> `etc_mesos-slave/isolation` for testing. They can be left as is. I included 
> them in this patch to show how this would work e2e.
> 
> 
> Diffs
> -
> 
>   3rdparty/python/requirements.txt 4ac242cfa2c1c19cb7447816ab86e748839d3d11 
>   examples/jobs/hello_world.aurora 5401bfebe753b5e53abd08baeac501144ced9b5a 
>   examples/vagrant/mesos_config/etc_mesos-slave/isolation 
> 1a7028ffc70116b104ef3ad22b7388f637707a0f 
>   examples/vagrant/systemd/thermos.service 
> 01925bcd2ae44f100df511f3c3951c3f5a1a72aa 
>   src/main/python/apache/aurora/tools/thermos_observer.py 
> dd9f0c46ceac9e939b1b763073314161de0ea614 
>   src/main/python/apache/thermos/monitoring/BUILD 
> 65ba7088f65e7baa5d30744736ba456b46a55e86 
>   src/main/python/apache/thermos/monitoring/disk.py 
> 986d33a5000f8d5db15cb639c81f8b1d756ffa05 
>   src/main/python/apache/thermos/monitoring/resource.py 
> adcdc751c03460dc801a18278faa96d6bd64722b 
>   src/main/python/apache/thermos/observer/task_observer.py 
> a6870d48bddf2a2ccede7bb68195f2baae1d0e47 
>   
> src/test/python/apache/aurora/executor/common/test_resource_manager_integration.py
>  fe74bd1d3ecd89fca1b5b2251202cbbc0f24 
>   src/test/python/apache/thermos/monitoring/BUILD 
> 8f2b39336dce6c7b580e6ba0009f60afdcb89179 
>   src/test/python/apache/thermos/monitoring/test_disk.py 
> 362393bfd1facf3198e2d438d0596b16700b72b8 
>   src/test/python/apache/thermos/monitoring/test_resource.py 
> e577e552d4ee1807096a15401851bb9fd95fa426 
> 
> 
> Diff: https://reviews.apache.org/r/66103/diff/3/
> 
> 
> Testing
> ---
> 
> I added unit tests.
> Tested in vagrant and it works as intenced.
> I also built and deployed in our test enviroment. In order to measure 
> imporoved performance I created jobs with nested folders and noticed 
> reduction in CPU utilization of the Observer process, by at least 60%. (1.5 
> CPU cores to 0.4 CPU cores)
> 
> 
> Thanks,
> 
> Reza Motamedi
> 
>

Re: Review Request 66103: Introduce mesos disk collector

2018-03-21 Thread Reza Motamedi 

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

(Updated March 21, 2018, 11:44 p.m.)


Review request for Aurora, David McLaughlin, Daniel Knightly, Jordan Ly, 
Santhosh Kumar Shanmugham, and Stephan Erb.


Repository: aurora


Description
---

When disk isolation is enabled in a Mesos agent it calculates the disk usage 
for each container. 
Thermos Observer also monitors disk usage using `twitter.common.dirutil`, 
essentially repeating the work already done by the agent. In practice, we see 
that disk monitoring is one of the most expensive resource monitoring tasks. 
For instance, when there are deeply nested directories, the CPU utilization of 
the observer process can easily reach 1.5 CPUs. It would be ideal if we 
delegate the disk monitoring task to the agent and do it only once. With this 
approach, when disk collection has improved in the agent (for instance by 
implementing XFS isolation), we can simply benefit from it without any code 
change. Some more information about the problem is provided in AURORA-1918.

This patch that introduces `MesosDiskCollector` which queries the agent's API 
endpoint to lookup disk_used_bytes. Note that there is also resource monitoring 
in thermos executor. Currently, I left the disk collector there to use the `du` 
implementation. That can be changed in a later patch.

I modified some vagrant config files including `aurora-executor.service` and 
`etc_mesos-slave/isolation` for testing. They can be left as is. I included 
them in this patch to show how this would work e2e.


Diffs (updated)
-

  3rdparty/python/requirements.txt 4ac242cfa2c1c19cb7447816ab86e748839d3d11 
  examples/jobs/hello_world.aurora 5401bfebe753b5e53abd08baeac501144ced9b5a 
  examples/vagrant/mesos_config/etc_mesos-slave/isolation 
1a7028ffc70116b104ef3ad22b7388f637707a0f 
  examples/vagrant/systemd/thermos.service 
01925bcd2ae44f100df511f3c3951c3f5a1a72aa 
  src/main/python/apache/aurora/tools/thermos_observer.py 
dd9f0c46ceac9e939b1b763073314161de0ea614 
  src/main/python/apache/thermos/monitoring/BUILD 
65ba7088f65e7baa5d30744736ba456b46a55e86 
  src/main/python/apache/thermos/monitoring/disk.py 
986d33a5000f8d5db15cb639c81f8b1d756ffa05 
  src/main/python/apache/thermos/monitoring/resource.py 
adcdc751c03460dc801a18278faa96d6bd64722b 
  src/main/python/apache/thermos/observer/task_observer.py 
a6870d48bddf2a2ccede7bb68195f2baae1d0e47 
  
src/test/python/apache/aurora/executor/common/test_resource_manager_integration.py
 fe74bd1d3ecd89fca1b5b2251202cbbc0f24 
  src/test/python/apache/thermos/monitoring/BUILD 
8f2b39336dce6c7b580e6ba0009f60afdcb89179 
  src/test/python/apache/thermos/monitoring/test_disk.py 
362393bfd1facf3198e2d438d0596b16700b72b8 
  src/test/python/apache/thermos/monitoring/test_resource.py 
e577e552d4ee1807096a15401851bb9fd95fa426 


Diff: https://reviews.apache.org/r/66103/diff/4/

Changes: https://reviews.apache.org/r/66103/diff/3-4/


Testing
---

I added unit tests.
Tested in vagrant and it works as intenced.
I also built and deployed in our test enviroment. In order to measure imporoved 
performance I created jobs with nested folders and noticed reduction in CPU 
utilization of the Observer process, by at least 60%. (1.5 CPU cores to 0.4 CPU 
cores)


Thanks,

Reza Motamedi

Re: Review Request 66192: [WIP] Variable group size updates

2018-03-21 Thread Renan DelValle 


> On March 20, 2018, 9:29 p.m., David McLaughlin wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java
> > Lines 107-111 (patched)
> > 
> >
> > Is this idea of a step sate needed? Strategies writing to the database 
> > seems like a design smell. 
> > 
> > If I have 10 instances:
> > 
> > [0,1,2,3,4,5,6,7,8,9] 
> > 
> > And I pass a VariableBatchStrategy of: [1, 4, 5]
> > 
> > Then using the BatchStrategy it is *always* in the following order:
> > 
> > [[0], [1,2,3,4], [5,6,7,8,9]]
> > 
> > And I don't *think* I need to write the step to the database to figure 
> > out which step I'm in? I can use the number idle and active to figure out 
> > exactly how many have been processed.
> 
> Jordan Ly wrote:
> +1, removing the need for persistent state would greatly reduce the 
> surface area of this patch.
> 
> Santhosh Kumar Shanmugham wrote:
> +1

Thanks for the feedback! 

I thought about doing it this way originally, but I wasn't totally convinced it 
would work well given the only info I had to go on inside of the strategy was 
how many tasks were waiting to be updated and how many were in the progress of 
being updated. 

As David pointed out, the order will remain the same (unless of course changes 
are made to 
https://github.com/apache/aurora/blob/2e1ca42887bc8ea1e8c6cddebe9d1cf29268c714/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java#L190
 ) so the update order should be deterministic (at least within the same 
version of Aurora).


The key points for this feature are: how this would survive leader 
elections/scheduler restarts, being able to roll back in a predictable manner 
at any given point of failure, and manainting the order in which tasks(shards) 
would get updated. 

I think I should be able to tackle all of these without writing to the database 
to persist with this new approach.


- Renan


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


On March 20, 2018, 7:10 p.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66192/
> ---
> 
> (Updated March 20, 2018, 7:10 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Santhosh Kumar Shanmugham, and 
> Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding support for variable group sizes when executing an update.
> 
> Design doc for this change is here: 
> https://docs.google.com/document/d/1xGk4ueH8YlmJCk6hQJh85u4to4M1VQD0l630IOchvgY/edit#heading=h.lg3hty82f5cz
> 
> I opted for the path of least resistance with regards to the Thrift changes 
> as I didn't see any benefit in making the larger changes required to make the 
> interfaces a bit more flexible.
> 
> Requesting feedback on these changes and the approach from the community 
> before I proceed.
> 
> Tests will be added after the community approves of the direciton and 
> approach.
> 
> Note to reviewers: Changes made in ActiveLimitedStrategy.java were made to 
> move towards getting rid of FluentIterable. I figured since I was touching 
> that code, it wouldn't hurt to test the Java 8 equivalent of it. I can get 
> rid of the change here and make it in a separate patch if desired.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> ef754e32172e7490a47a13e7b526f243ffa3efeb 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> b79e2045ccda05d5058565f81988dfe33feea8f1 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> b25f3831cecc58c90375c90b16142421f8f09e38 
>   src/main/java/org/apache/aurora/scheduler/storage/durability/Loader.java 
> 10864f122eff5027c88d835baae6de483d960218 
>   
> src/main/java/org/apache/aurora/scheduler/storage/durability/WriteRecorder.java
>  8d70cae35289a9e36142bab288cf0c9398ebd2d4 
>   
> src/main/java/org/apache/aurora/scheduler/storage/mem/MemJobUpdateStore.java 
> 9e86b9e276ea90a249284a824705b5bbf19dcbce 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  9fc0416086dd3eb2e2f4e8f659da59fcdea2b22b 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  f8be8058f3a80a18b999d2666e2adb33e1e55fef 
>   src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 
> 3992aa77fc305adc390a4aaeb1d3939d6241ddbd 
>   
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java
>  855ea9c20788b51695b7eff5ac0970f0d52a9546 
>   
> src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrateg

Re: Review Request 66103: Introduce mesos disk collector

2018-03-21 Thread Santhosh Kumar Shanmugham 

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



Update RELEASE_NOTES.


src/main/python/apache/thermos/monitoring/disk.py
Lines 132-135 (patched)


What happens if `GET` returns 5xx or 4xx? Will this crash the Observer? We 
should be adding more logging and fail gracefully. Particularly this can happen 
if someone has HTTP Auth enabled for the Mesos endpoints and deploying this has 
the potential to crash all the Observers.


- Santhosh Kumar Shanmugham


On March 21, 2018, 4:44 p.m., Reza Motamedi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66103/
> ---
> 
> (Updated March 21, 2018, 4:44 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Daniel Knightly, Jordan Ly, 
> Santhosh Kumar Shanmugham, and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> When disk isolation is enabled in a Mesos agent it calculates the disk usage 
> for each container. 
> Thermos Observer also monitors disk usage using `twitter.common.dirutil`, 
> essentially repeating the work already done by the agent. In practice, we see 
> that disk monitoring is one of the most expensive resource monitoring tasks. 
> For instance, when there are deeply nested directories, the CPU utilization 
> of the observer process can easily reach 1.5 CPUs. It would be ideal if we 
> delegate the disk monitoring task to the agent and do it only once. With this 
> approach, when disk collection has improved in the agent (for instance by 
> implementing XFS isolation), we can simply benefit from it without any code 
> change. Some more information about the problem is provided in AURORA-1918.
> 
> This patch that introduces `MesosDiskCollector` which queries the agent's API 
> endpoint to lookup disk_used_bytes. Note that there is also resource 
> monitoring in thermos executor. Currently, I left the disk collector there to 
> use the `du` implementation. That can be changed in a later patch.
> 
> I modified some vagrant config files including `aurora-executor.service` and 
> `etc_mesos-slave/isolation` for testing. They can be left as is. I included 
> them in this patch to show how this would work e2e.
> 
> 
> Diffs
> -
> 
>   3rdparty/python/requirements.txt 4ac242cfa2c1c19cb7447816ab86e748839d3d11 
>   examples/jobs/hello_world.aurora 5401bfebe753b5e53abd08baeac501144ced9b5a 
>   examples/vagrant/mesos_config/etc_mesos-slave/isolation 
> 1a7028ffc70116b104ef3ad22b7388f637707a0f 
>   examples/vagrant/systemd/thermos.service 
> 01925bcd2ae44f100df511f3c3951c3f5a1a72aa 
>   src/main/python/apache/aurora/tools/thermos_observer.py 
> dd9f0c46ceac9e939b1b763073314161de0ea614 
>   src/main/python/apache/thermos/monitoring/BUILD 
> 65ba7088f65e7baa5d30744736ba456b46a55e86 
>   src/main/python/apache/thermos/monitoring/disk.py 
> 986d33a5000f8d5db15cb639c81f8b1d756ffa05 
>   src/main/python/apache/thermos/monitoring/resource.py 
> adcdc751c03460dc801a18278faa96d6bd64722b 
>   src/main/python/apache/thermos/observer/task_observer.py 
> a6870d48bddf2a2ccede7bb68195f2baae1d0e47 
>   
> src/test/python/apache/aurora/executor/common/test_resource_manager_integration.py
>  fe74bd1d3ecd89fca1b5b2251202cbbc0f24 
>   src/test/python/apache/thermos/monitoring/BUILD 
> 8f2b39336dce6c7b580e6ba0009f60afdcb89179 
>   src/test/python/apache/thermos/monitoring/test_disk.py 
> 362393bfd1facf3198e2d438d0596b16700b72b8 
>   src/test/python/apache/thermos/monitoring/test_resource.py 
> e577e552d4ee1807096a15401851bb9fd95fa426 
> 
> 
> Diff: https://reviews.apache.org/r/66103/diff/4/
> 
> 
> Testing
> ---
> 
> I added unit tests.
> Tested in vagrant and it works as intenced.
> I also built and deployed in our test enviroment. In order to measure 
> imporoved performance I created jobs with nested folders and noticed 
> reduction in CPU utilization of the Observer process, by at least 60%. (1.5 
> CPU cores to 0.4 CPU cores)
> 
> 
> Thanks,
> 
> Reza Motamedi
> 
>

Re: Review Request 66103: Introduce mesos disk collector

2018-03-21 Thread Aurora ReviewBot 

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


Ship it!




Master (f32086d) is green with this patch.
  ./build-support/jenkins/build.sh

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

- Aurora ReviewBot


On March 21, 2018, 11:44 p.m., Reza Motamedi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66103/
> ---
> 
> (Updated March 21, 2018, 11:44 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Daniel Knightly, Jordan Ly, 
> Santhosh Kumar Shanmugham, and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> When disk isolation is enabled in a Mesos agent it calculates the disk usage 
> for each container. 
> Thermos Observer also monitors disk usage using `twitter.common.dirutil`, 
> essentially repeating the work already done by the agent. In practice, we see 
> that disk monitoring is one of the most expensive resource monitoring tasks. 
> For instance, when there are deeply nested directories, the CPU utilization 
> of the observer process can easily reach 1.5 CPUs. It would be ideal if we 
> delegate the disk monitoring task to the agent and do it only once. With this 
> approach, when disk collection has improved in the agent (for instance by 
> implementing XFS isolation), we can simply benefit from it without any code 
> change. Some more information about the problem is provided in AURORA-1918.
> 
> This patch that introduces `MesosDiskCollector` which queries the agent's API 
> endpoint to lookup disk_used_bytes. Note that there is also resource 
> monitoring in thermos executor. Currently, I left the disk collector there to 
> use the `du` implementation. That can be changed in a later patch.
> 
> I modified some vagrant config files including `aurora-executor.service` and 
> `etc_mesos-slave/isolation` for testing. They can be left as is. I included 
> them in this patch to show how this would work e2e.
> 
> 
> Diffs
> -
> 
>   3rdparty/python/requirements.txt 4ac242cfa2c1c19cb7447816ab86e748839d3d11 
>   examples/jobs/hello_world.aurora 5401bfebe753b5e53abd08baeac501144ced9b5a 
>   examples/vagrant/mesos_config/etc_mesos-slave/isolation 
> 1a7028ffc70116b104ef3ad22b7388f637707a0f 
>   examples/vagrant/systemd/thermos.service 
> 01925bcd2ae44f100df511f3c3951c3f5a1a72aa 
>   src/main/python/apache/aurora/tools/thermos_observer.py 
> dd9f0c46ceac9e939b1b763073314161de0ea614 
>   src/main/python/apache/thermos/monitoring/BUILD 
> 65ba7088f65e7baa5d30744736ba456b46a55e86 
>   src/main/python/apache/thermos/monitoring/disk.py 
> 986d33a5000f8d5db15cb639c81f8b1d756ffa05 
>   src/main/python/apache/thermos/monitoring/resource.py 
> adcdc751c03460dc801a18278faa96d6bd64722b 
>   src/main/python/apache/thermos/observer/task_observer.py 
> a6870d48bddf2a2ccede7bb68195f2baae1d0e47 
>   
> src/test/python/apache/aurora/executor/common/test_resource_manager_integration.py
>  fe74bd1d3ecd89fca1b5b2251202cbbc0f24 
>   src/test/python/apache/thermos/monitoring/BUILD 
> 8f2b39336dce6c7b580e6ba0009f60afdcb89179 
>   src/test/python/apache/thermos/monitoring/test_disk.py 
> 362393bfd1facf3198e2d438d0596b16700b72b8 
>   src/test/python/apache/thermos/monitoring/test_resource.py 
> e577e552d4ee1807096a15401851bb9fd95fa426 
> 
> 
> Diff: https://reviews.apache.org/r/66103/diff/4/
> 
> 
> Testing
> ---
> 
> I added unit tests.
> Tested in vagrant and it works as intenced.
> I also built and deployed in our test enviroment. In order to measure 
> imporoved performance I created jobs with nested folders and noticed 
> reduction in CPU utilization of the Observer process, by at least 60%. (1.5 
> CPU cores to 0.4 CPU cores)
> 
> 
> Thanks,
> 
> Reza Motamedi
> 
>

Re: Review Request 66103: Introduce mesos disk collector

2018-03-21 Thread Reza Motamedi 

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

(Updated March 22, 2018, 5:29 a.m.)


Review request for Aurora, David McLaughlin, Daniel Knightly, Jordan Ly, 
Santhosh Kumar Shanmugham, and Stephan Erb.


Changes
---

- Address the comment about Auth enabled HTTP APIs on mesos agent and added 
additional tests.
- updated release notes and config notes.


Repository: aurora


Description
---

When disk isolation is enabled in a Mesos agent it calculates the disk usage 
for each container. 
Thermos Observer also monitors disk usage using `twitter.common.dirutil`, 
essentially repeating the work already done by the agent. In practice, we see 
that disk monitoring is one of the most expensive resource monitoring tasks. 
For instance, when there are deeply nested directories, the CPU utilization of 
the observer process can easily reach 1.5 CPUs. It would be ideal if we 
delegate the disk monitoring task to the agent and do it only once. With this 
approach, when disk collection has improved in the agent (for instance by 
implementing XFS isolation), we can simply benefit from it without any code 
change. Some more information about the problem is provided in AURORA-1918.

This patch that introduces `MesosDiskCollector` which queries the agent's API 
endpoint to lookup disk_used_bytes. Note that there is also resource monitoring 
in thermos executor. Currently, I left the disk collector there to use the `du` 
implementation. That can be changed in a later patch.

I modified some vagrant config files including `aurora-executor.service` and 
`etc_mesos-slave/isolation` for testing. They can be left as is. I included 
them in this patch to show how this would work e2e.


Diffs (updated)
-

  3rdparty/python/requirements.txt 4ac242cfa2c1c19cb7447816ab86e748839d3d11 
  RELEASE-NOTES.md 51ab6c724694244bf616b29e9beace4a4a3f5252 
  docs/reference/observer-configuration.md 
8a443c94f7f37f9454989781f722101a97c99f15 
  examples/jobs/hello_world.aurora 5401bfebe753b5e53abd08baeac501144ced9b5a 
  examples/vagrant/mesos_config/etc_mesos-slave/isolation 
1a7028ffc70116b104ef3ad22b7388f637707a0f 
  examples/vagrant/systemd/thermos.service 
01925bcd2ae44f100df511f3c3951c3f5a1a72aa 
  src/main/python/apache/aurora/tools/thermos_observer.py 
dd9f0c46ceac9e939b1b763073314161de0ea614 
  src/main/python/apache/thermos/monitoring/BUILD 
65ba7088f65e7baa5d30744736ba456b46a55e86 
  src/main/python/apache/thermos/monitoring/disk.py 
986d33a5000f8d5db15cb639c81f8b1d756ffa05 
  src/main/python/apache/thermos/monitoring/resource.py 
adcdc751c03460dc801a18278faa96d6bd64722b 
  src/main/python/apache/thermos/observer/task_observer.py 
a6870d48bddf2a2ccede7bb68195f2baae1d0e47 
  
src/test/python/apache/aurora/executor/common/test_resource_manager_integration.py
 fe74bd1d3ecd89fca1b5b2251202cbbc0f24 
  src/test/python/apache/thermos/monitoring/BUILD 
8f2b39336dce6c7b580e6ba0009f60afdcb89179 
  src/test/python/apache/thermos/monitoring/test_disk.py 
362393bfd1facf3198e2d438d0596b16700b72b8 
  src/test/python/apache/thermos/monitoring/test_resource.py 
e577e552d4ee1807096a15401851bb9fd95fa426 


Diff: https://reviews.apache.org/r/66103/diff/5/

Changes: https://reviews.apache.org/r/66103/diff/4-5/


Testing (updated)
---

- I added unit tests.
- Tested in vagrant and it works as intenced.
- I also built and deployed in our test enviroment. In order to measure 
imporoved performance I created jobs with nested folders and noticed reduction 
in CPU utilization of the Observer process, by at least 60%. (1.5 CPU cores to 
0.4 CPU cores)

Here is one specific test setup: On two hosts I created a two tasks. Each task 
creates identical nested directory structures and files in them. The overall 
size is 30GB. test_host_1 runs the current version of observer and test_host_2 
runs Observer with this patch and also has mesos_disk_collection enabled. The 
results are as follows:

```
rezam[7]TEST_HOST_1 ~ $ while true; do echo `date`; curl localhost:1338/vars -s 
| grep cpu; sleep 10; done
Thu Mar 22 04:36:17 UTC 2018
observer.observer_cpu 108.9
Thu Mar 22 04:36:27 UTC 2018
observer.observer_cpu 123.2
Thu Mar 22 04:36:38 UTC 2018
observer.observer_cpu 123.2
Thu Mar 22 04:36:48 UTC 2018
observer.observer_cpu 123.2
Thu Mar 22 04:36:58 UTC 2018
observer.observer_cpu 111.0
Thu Mar 22 04:37:08 UTC 2018
observer.observer_cpu 111.0
Thu Mar 22 04:37:18 UTC 2018
observer.observer_cpu 111.0


rezam[7]TEST_HOST_2 ~ $ while true; do echo `date`; curl localhost:1338/vars -s 
| grep cpu; sleep 10; done
Thu Mar 22 04:36:20 UTC 2018
observer.observer_cpu 1.3
Thu Mar 22 04:36:30 UTC 2018
observer.observer_cpu 1.3
Thu Mar 22 04:36:40 UTC 2018
observer.observer_cpu 1.3
Thu Mar 22 04:36:50 UTC 2018
observer.observer_cpu 1.2
Thu Mar 22 04:37:00 UTC 2018
observer.observer_cpu 1.2
Thu Mar 22

Re: Review Request 66103: Introduce mesos disk collector

2018-03-21 Thread Aurora ReviewBot 

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


Ship it!




Master (f32086d) is green with this patch.
  ./build-support/jenkins/build.sh

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

- Aurora ReviewBot


On March 21, 2018, 10:29 p.m., Reza Motamedi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66103/
> ---
> 
> (Updated March 21, 2018, 10:29 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Daniel Knightly, Jordan Ly, 
> Santhosh Kumar Shanmugham, and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> When disk isolation is enabled in a Mesos agent it calculates the disk usage 
> for each container. 
> Thermos Observer also monitors disk usage using `twitter.common.dirutil`, 
> essentially repeating the work already done by the agent. In practice, we see 
> that disk monitoring is one of the most expensive resource monitoring tasks. 
> For instance, when there are deeply nested directories, the CPU utilization 
> of the observer process can easily reach 1.5 CPUs. It would be ideal if we 
> delegate the disk monitoring task to the agent and do it only once. With this 
> approach, when disk collection has improved in the agent (for instance by 
> implementing XFS isolation), we can simply benefit from it without any code 
> change. Some more information about the problem is provided in AURORA-1918.
> 
> This patch that introduces `MesosDiskCollector` which queries the agent's API 
> endpoint to lookup disk_used_bytes. Note that there is also resource 
> monitoring in thermos executor. Currently, I left the disk collector there to 
> use the `du` implementation. That can be changed in a later patch.
> 
> I modified some vagrant config files including `aurora-executor.service` and 
> `etc_mesos-slave/isolation` for testing. They can be left as is. I included 
> them in this patch to show how this would work e2e.
> 
> 
> Diffs
> -
> 
>   3rdparty/python/requirements.txt 4ac242cfa2c1c19cb7447816ab86e748839d3d11 
>   RELEASE-NOTES.md 51ab6c724694244bf616b29e9beace4a4a3f5252 
>   docs/reference/observer-configuration.md 
> 8a443c94f7f37f9454989781f722101a97c99f15 
>   examples/jobs/hello_world.aurora 5401bfebe753b5e53abd08baeac501144ced9b5a 
>   examples/vagrant/mesos_config/etc_mesos-slave/isolation 
> 1a7028ffc70116b104ef3ad22b7388f637707a0f 
>   examples/vagrant/systemd/thermos.service 
> 01925bcd2ae44f100df511f3c3951c3f5a1a72aa 
>   src/main/python/apache/aurora/tools/thermos_observer.py 
> dd9f0c46ceac9e939b1b763073314161de0ea614 
>   src/main/python/apache/thermos/monitoring/BUILD 
> 65ba7088f65e7baa5d30744736ba456b46a55e86 
>   src/main/python/apache/thermos/monitoring/disk.py 
> 986d33a5000f8d5db15cb639c81f8b1d756ffa05 
>   src/main/python/apache/thermos/monitoring/resource.py 
> adcdc751c03460dc801a18278faa96d6bd64722b 
>   src/main/python/apache/thermos/observer/task_observer.py 
> a6870d48bddf2a2ccede7bb68195f2baae1d0e47 
>   
> src/test/python/apache/aurora/executor/common/test_resource_manager_integration.py
>  fe74bd1d3ecd89fca1b5b2251202cbbc0f24 
>   src/test/python/apache/thermos/monitoring/BUILD 
> 8f2b39336dce6c7b580e6ba0009f60afdcb89179 
>   src/test/python/apache/thermos/monitoring/test_disk.py 
> 362393bfd1facf3198e2d438d0596b16700b72b8 
>   src/test/python/apache/thermos/monitoring/test_resource.py 
> e577e552d4ee1807096a15401851bb9fd95fa426 
> 
> 
> Diff: https://reviews.apache.org/r/66103/diff/5/
> 
> 
> Testing
> ---
> 
> - I added unit tests.
> - Tested in vagrant and it works as intenced.
> - I also built and deployed in our test enviroment. In order to measure 
> imporoved performance I created jobs with nested folders and noticed 
> reduction in CPU utilization of the Observer process, by at least 60%. (1.5 
> CPU cores to 0.4 CPU cores)
> 
> Here is one specific test setup: On two hosts I created a two tasks. Each 
> task creates identical nested directory structures and files in them. The 
> overall size is 30GB. test_host_1 runs the current version of observer and 
> test_host_2 runs Observer with this patch and also has mesos_disk_collection 
> enabled. The results are as follows:
> 
> ```
> rezam[7]TEST_HOST_1 ~ $ while true; do echo `date`; curl localhost:1338/vars 
> -s | grep cpu; sleep 10; done
> Thu Mar 22 04:36:17 UTC 2018
> observer.observer_cpu 108.9
> Thu Mar 22 04:36:27 UTC 2018
> observer.observer_cpu 123.2
> Thu Mar 22 04:36:38 UTC 2018
> observer.observer_cpu 123.2
> Thu Mar 22 04:36:48 UTC 2018
> observer.observer_cpu 123.2
> Thu Mar 22 04:36:58 UTC 2018
> observer.observer_cpu 111.0
> Thu Mar 22 04:37:08 UTC 2018
> observer.obs