Re: Review Request 51662: AURORA-1602 Aurora admin commands for reconcilation

[Aurora ReviewBot]

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



This patch does not apply cleanly against master (8fca745), do you need to 
rebase?

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

- Aurora ReviewBot


On Sept. 8, 2016, 6:48 a.m., Karthik Anantha Padmanabhan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51662/
> ---
> 
> (Updated Sept. 8, 2016, 6:48 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> AURORA-1602 Aurora admin commands for reconcilation
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md d79aaadc197697d09a71c83494a01765d6a983d4 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   
> src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java 
> 3275d72a0a74909e635bce615e2036ec72aa38ee 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   src/main/python/apache/aurora/admin/admin.py 
> 76009b9c1c7a130c25abad544a176dc590dafb12 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 9149c3018ae58d405f284fcbd4076d251ccc8192 
>   
> src/test/java/org/apache/aurora/scheduler/reconciliation/TaskReconcilerTest.java
>  b9317dc20456f90057ec2bf4d10619a5ae986187 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  779dc302602ae8842084807ca868a491ea99b676 
>   src/test/python/apache/aurora/admin/test_admin.py 
> eb193c4a33f5275f05a995338446bec0e19bc1cf 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afac2500551af2fce406bb906aa4e33f353e90a1 
> 
> Diff: https://reviews.apache.org/r/51662/diff/
> 
> 
> Testing
> ---
> 
> * Manually tested on my local vagrant installation
> * ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Karthik Anantha Padmanabhan
> 
>

Re: Review Request 51712: Extend getJobUpdateDetails to accept JobUpdateQuery

[Stephan Erb]

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



LGTM (+-1 the current test failure)


api/src/main/thrift/org/apache/aurora/gen/api.thrift (lines 899 - 900)


If this is deprecated, shouldn't you call this out in the release notes ad 
well?


- Stephan Erb


On Sept. 8, 2016, 2:03 a.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51712/
> ---
> 
> (Updated Sept. 8, 2016, 2:03 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1764
> https://issues.apache.org/jira/browse/AURORA-1764
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This extends getJobUpdateDetails to return a list of details instead of being 
> scoped to a single update.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md d79aaadc197697d09a71c83494a01765d6a983d4 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 4202f0eec59ef16668fca6b6ebb925335ad5dea1 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  a7d1f74acdfe5f58eb822a4d826e920cad53dced 
> 
> Diff: https://reviews.apache.org/r/51712/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 51664: Document the Mesos containerizer

[Joshua Cohen]

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


Ship it!




lgtm modulo the below.


docs/operations/configuration.md (line 32)


s/pulbic/public



docs/operations/configuration.md (line 137)


"Both the Mesos and Docker containerizers"



docs/operations/configuration.md (lines 158 - 159)


s/to pass//


- Joshua Cohen


On Sept. 6, 2016, 7:18 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51664/
> ---
> 
> (Updated Sept. 6, 2016, 7:18 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Bugs: AURORA-1640
> https://issues.apache.org/jira/browse/AURORA-1640
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Included changes:
> 
> * enabled Docker support in our vagrant box
> * consistent example jobs for both containerizers
> * short enduser and operator  documentation
> * shuffled the reference documentation so that it is clear certain 
> limitations apply only to the Docker containerizer
> 
> 
> Diffs
> -
> 
>   docs/features/containers.md 6b9f717d672ef88be61341268f9c06923d217087 
>   docs/operations/configuration.md 85787b0815e9f0cb37c21efc04923b0e0bd10bf9 
>   docs/reference/configuration.md ff40262f1fb4eff780fbef17abcaecc457dc68d3 
>   examples/jobs/docker/hello_docker.aurora 
> d5611e6b78cb613c04e2fc3df54397ee4401a62e 
>   examples/jobs/hello_docker_image.aurora PRE-CREATION 
>   examples/vagrant/aurorabuild.sh 9daca067acc05c89ccc183e56bf30c787baf97dd 
>   examples/vagrant/mesos_config/etc_mesos-slave/image_providers 
> 7168c0ca4a81e0a1ce2320534015c6692b804b54 
>   examples/vagrant/mesos_config/etc_mesos-slave/isolation 
> c36223028513ace26040620b813022accaf0edf3 
> 
> Diff: https://reviews.apache.org/r/51664/diff/
> 
> 
> Testing
> ---
> 
> Rendered version available at 
> https://github.com/StephanErb/aurora/tree/containerizer-docs/docs
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 51712: Extend getJobUpdateDetails to accept JobUpdateQuery

[Maxim Khutornenko]

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



This will most certainly break the UI. The client needs to be updated as well 
to pass both (old and new) arguments. Please, test in vagrant with different 
combinations of scheduler and client versions.

- Maxim Khutornenko


On Sept. 8, 2016, 12:03 a.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51712/
> ---
> 
> (Updated Sept. 8, 2016, 12:03 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1764
> https://issues.apache.org/jira/browse/AURORA-1764
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This extends getJobUpdateDetails to return a list of details instead of being 
> scoped to a single update.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md d79aaadc197697d09a71c83494a01765d6a983d4 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 4202f0eec59ef16668fca6b6ebb925335ad5dea1 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  a7d1f74acdfe5f58eb822a4d826e920cad53dced 
> 
> Diff: https://reviews.apache.org/r/51712/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 51662: AURORA-1602 Aurora admin commands for reconcilation

[Karthik Anantha Padmanabhan]

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

(Updated Sept. 8, 2016, 5:27 p.m.)


Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.


Changes
---

Resolve merge conflict


Repository: aurora


Description
---

AURORA-1602 Aurora admin commands for reconcilation


Diffs (updated)
-

  RELEASE-NOTES.md bbf7198dc7ce53bb02a1bc59f75a661e23472913 
  api/src/main/thrift/org/apache/aurora/gen/api.thrift 
c5765b70501c101f0535b4eed94e9948c36808f9 
  src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java 
3275d72a0a74909e635bce615e2036ec72aa38ee 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
929d021a336c6a3438613c9340c84a1096dc9069 
  src/main/python/apache/aurora/admin/admin.py 
76009b9c1c7a130c25abad544a176dc590dafb12 
  src/main/python/apache/aurora/client/api/__init__.py 
9149c3018ae58d405f284fcbd4076d251ccc8192 
  
src/test/java/org/apache/aurora/scheduler/reconciliation/TaskReconcilerTest.java
 b9317dc20456f90057ec2bf4d10619a5ae986187 
  
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 779dc302602ae8842084807ca868a491ea99b676 
  src/test/python/apache/aurora/admin/test_admin.py 
eb193c4a33f5275f05a995338446bec0e19bc1cf 
  src/test/python/apache/aurora/client/api/test_scheduler_client.py 
afac2500551af2fce406bb906aa4e33f353e90a1 

Diff: https://reviews.apache.org/r/51662/diff/


Testing
---

* Manually tested on my local vagrant installation
* ./build-support/jenkins/build.sh


Thanks,

Karthik Anantha Padmanabhan

Re: Review Request 51662: AURORA-1602 Aurora admin commands for reconcilation

[Aurora ReviewBot]

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


Ship it!




Master (8fca745) 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 Sept. 8, 2016, 5:27 p.m., Karthik Anantha Padmanabhan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51662/
> ---
> 
> (Updated Sept. 8, 2016, 5:27 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> AURORA-1602 Aurora admin commands for reconcilation
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md bbf7198dc7ce53bb02a1bc59f75a661e23472913 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   
> src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java 
> 3275d72a0a74909e635bce615e2036ec72aa38ee 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   src/main/python/apache/aurora/admin/admin.py 
> 76009b9c1c7a130c25abad544a176dc590dafb12 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 9149c3018ae58d405f284fcbd4076d251ccc8192 
>   
> src/test/java/org/apache/aurora/scheduler/reconciliation/TaskReconcilerTest.java
>  b9317dc20456f90057ec2bf4d10619a5ae986187 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  779dc302602ae8842084807ca868a491ea99b676 
>   src/test/python/apache/aurora/admin/test_admin.py 
> eb193c4a33f5275f05a995338446bec0e19bc1cf 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afac2500551af2fce406bb906aa4e33f353e90a1 
> 
> Diff: https://reviews.apache.org/r/51662/diff/
> 
> 
> Testing
> ---
> 
> * Manually tested on my local vagrant installation
> * ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Karthik Anantha Padmanabhan
> 
>

Re: Review Request 51662: AURORA-1602 Aurora admin commands for reconcilation

[Maxim Khutornenko]

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




api/src/main/thrift/org/apache/aurora/gen/api.thrift (line 1215)


Typos in `reconciliation`. Here and in other places.

Also, please use `task` in the RPC name, e.g.: 
`triggerExplicitTaskReconciliation`.



src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java 
(line 123)


Prefer using immutable `IExplicit...` wrapper generated for you by the 
build.



src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java 
(lines 124 - 125)


Should be formatted as:
```
int trueBatchSize = reconcilationSettings.isSetBatchSize()
? reconcilationSettings.getBatchSize()
: settings.explicitBatchSize;
```



src/main/python/apache/aurora/admin/admin.py (line 345)


Please be explicit in the name, e.g.: `reconcile_tasks`



src/main/python/apache/aurora/admin/admin.py (line 351)


Needs clarifcation that `reconciliation_explicit_batch_size` is a scheduler 
setting.


- Maxim Khutornenko


On Sept. 8, 2016, 5:27 p.m., Karthik Anantha Padmanabhan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51662/
> ---
> 
> (Updated Sept. 8, 2016, 5:27 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> AURORA-1602 Aurora admin commands for reconcilation
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md bbf7198dc7ce53bb02a1bc59f75a661e23472913 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   
> src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java 
> 3275d72a0a74909e635bce615e2036ec72aa38ee 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   src/main/python/apache/aurora/admin/admin.py 
> 76009b9c1c7a130c25abad544a176dc590dafb12 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 9149c3018ae58d405f284fcbd4076d251ccc8192 
>   
> src/test/java/org/apache/aurora/scheduler/reconciliation/TaskReconcilerTest.java
>  b9317dc20456f90057ec2bf4d10619a5ae986187 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  779dc302602ae8842084807ca868a491ea99b676 
>   src/test/python/apache/aurora/admin/test_admin.py 
> eb193c4a33f5275f05a995338446bec0e19bc1cf 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afac2500551af2fce406bb906aa4e33f353e90a1 
> 
> Diff: https://reviews.apache.org/r/51662/diff/
> 
> 
> Testing
> ---
> 
> * Manually tested on my local vagrant installation
> * ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Karthik Anantha Padmanabhan
> 
>

Re: Review Request 51662: AURORA-1602 Aurora admin commands for reconcilation

[Santhosh Kumar Shanmugham]

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




src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 (line 167)


Don't we need tests specific to 'reconciliation' here too?



src/test/python/apache/aurora/admin/test_admin.py (line 228)


s/get_scheduler/reconcile



src/test/python/apache/aurora/admin/test_admin.py (line 248)


s/get_scheduler/reconcile



src/test/python/apache/aurora/admin/test_admin.py (line 265)


Should this also be assert_called_once_with(None) ?



src/test/python/apache/aurora/admin/test_admin.py (line 268)


s/get_scheduler/reconcile



src/test/python/apache/aurora/admin/test_admin.py (line 285)


Should this also be assert_called_once_with(None) ?


- Santhosh Kumar Shanmugham


On Sept. 8, 2016, 10:27 a.m., Karthik Anantha Padmanabhan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51662/
> ---
> 
> (Updated Sept. 8, 2016, 10:27 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> AURORA-1602 Aurora admin commands for reconcilation
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md bbf7198dc7ce53bb02a1bc59f75a661e23472913 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   
> src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java 
> 3275d72a0a74909e635bce615e2036ec72aa38ee 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   src/main/python/apache/aurora/admin/admin.py 
> 76009b9c1c7a130c25abad544a176dc590dafb12 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 9149c3018ae58d405f284fcbd4076d251ccc8192 
>   
> src/test/java/org/apache/aurora/scheduler/reconciliation/TaskReconcilerTest.java
>  b9317dc20456f90057ec2bf4d10619a5ae986187 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  779dc302602ae8842084807ca868a491ea99b676 
>   src/test/python/apache/aurora/admin/test_admin.py 
> eb193c4a33f5275f05a995338446bec0e19bc1cf 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afac2500551af2fce406bb906aa4e33f353e90a1 
> 
> Diff: https://reviews.apache.org/r/51662/diff/
> 
> 
> Testing
> ---
> 
> * Manually tested on my local vagrant installation
> * ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Karthik Anantha Padmanabhan
> 
>

Re: Review Request 51662: AURORA-1602 Aurora admin commands for reconcilation

[Karthik Anantha Padmanabhan]

> On Sept. 8, 2016, 6:18 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java,
> >  line 123
> > 
> >
> > Prefer using immutable `IExplicit...` wrapper generated for you by the 
> > build.

With the `IExplicit...` version I do not have the ability to check if the field 
is set or not using `isSet...`. I use that in order to provide the default 
values for the batch size.


> On Sept. 8, 2016, 6:18 p.m., Maxim Khutornenko wrote:
> > src/main/python/apache/aurora/admin/admin.py, line 351
> > 
> >
> > Needs clarifcation that `reconciliation_explicit_batch_size` is a 
> > scheduler setting.

Good question. It does not show up in the docs. However it is a command line 
argument in the `ReconciliationModule`. So should this be a scheduler setting ?


- Karthik


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


On Sept. 8, 2016, 5:27 p.m., Karthik Anantha Padmanabhan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51662/
> ---
> 
> (Updated Sept. 8, 2016, 5:27 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> AURORA-1602 Aurora admin commands for reconcilation
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md bbf7198dc7ce53bb02a1bc59f75a661e23472913 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   
> src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java 
> 3275d72a0a74909e635bce615e2036ec72aa38ee 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   src/main/python/apache/aurora/admin/admin.py 
> 76009b9c1c7a130c25abad544a176dc590dafb12 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 9149c3018ae58d405f284fcbd4076d251ccc8192 
>   
> src/test/java/org/apache/aurora/scheduler/reconciliation/TaskReconcilerTest.java
>  b9317dc20456f90057ec2bf4d10619a5ae986187 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  779dc302602ae8842084807ca868a491ea99b676 
>   src/test/python/apache/aurora/admin/test_admin.py 
> eb193c4a33f5275f05a995338446bec0e19bc1cf 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afac2500551af2fce406bb906aa4e33f353e90a1 
> 
> Diff: https://reviews.apache.org/r/51662/diff/
> 
> 
> Testing
> ---
> 
> * Manually tested on my local vagrant installation
> * ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Karthik Anantha Padmanabhan
> 
>

Re: Review Request 51662: AURORA-1602 Aurora admin commands for reconcilation

[Karthik Anantha Padmanabhan]

> On Sept. 8, 2016, 6:43 p.m., Santhosh Kumar Shanmugham wrote:
> > src/test/python/apache/aurora/admin/test_admin.py, line 285
> > 
> >
> > Should this also be assert_called_once_with(None) ?

No - I set the option as 500 in the test and I want to test that we call with 
the provided option


- Karthik


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


On Sept. 8, 2016, 5:27 p.m., Karthik Anantha Padmanabhan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51662/
> ---
> 
> (Updated Sept. 8, 2016, 5:27 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> AURORA-1602 Aurora admin commands for reconcilation
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md bbf7198dc7ce53bb02a1bc59f75a661e23472913 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   
> src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java 
> 3275d72a0a74909e635bce615e2036ec72aa38ee 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   src/main/python/apache/aurora/admin/admin.py 
> 76009b9c1c7a130c25abad544a176dc590dafb12 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 9149c3018ae58d405f284fcbd4076d251ccc8192 
>   
> src/test/java/org/apache/aurora/scheduler/reconciliation/TaskReconcilerTest.java
>  b9317dc20456f90057ec2bf4d10619a5ae986187 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  779dc302602ae8842084807ca868a491ea99b676 
>   src/test/python/apache/aurora/admin/test_admin.py 
> eb193c4a33f5275f05a995338446bec0e19bc1cf 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afac2500551af2fce406bb906aa4e33f353e90a1 
> 
> Diff: https://reviews.apache.org/r/51662/diff/
> 
> 
> Testing
> ---
> 
> * Manually tested on my local vagrant installation
> * ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Karthik Anantha Padmanabhan
> 
>

Review Request 51742: Modify the callback function passed to StatusManager

[Kai Huang]

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

Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.


Bugs: https://issues.apache.org/jira/browse/AURORA-1225

https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/AURORA-1225


Repository: aurora


Description
---

Modify the callback function passed to StatusManager to handle more 
StatusResult.

Currently, Aurora executor passes a callback to a status manager. The status 
manager periodically polls the status of HealthChecker. When a non-empty status 
is deteted, the callback function will shut down the status manager. 

In health check driven updates, the health checker will change its status to 
"TASK_RUNNING" when a successful required number of health checks is reached. 
Therefore, we need to modify the callback function, so that it won't shutdown 
the status manager if the task is in non-exit state like "TASK_RUNNING".

Please see this epic: https://issues.apache.org/jira/browse/AURORA-1225
and the design doc: 
https://docs.google.com/document/d/1ZdgW8S4xMhvKW7iQUX99xZm10NXSxEWR0a-21FP5d94/edit#
 for more details and background.


Diffs
-

  src/main/python/apache/aurora/executor/aurora_executor.py 
ce5ef680f01831cd89fced8969ae3246c7f60cfd 
  src/main/python/apache/aurora/executor/status_manager.py 
228a99a05f339e21cd7e769a42b9b2276e7bc3fc 
  src/test/python/apache/aurora/executor/test_status_manager.py 
ce4679ba1aa7b42cf0115c943d84663030182d23 

Diff: https://reviews.apache.org/r/51742/diff/


Testing
---

./gradlew build

./build-support/jenkins/build.sh


Thanks,

Kai Huang

Re: Review Request 51742: Modify the callback function passed to StatusManager

[Kai Huang]

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

(Updated Sept. 8, 2016, 8:07 p.m.)


Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.


Changes
---

Updated bugs related to this review request.


Bugs: AURORA-1225
https://issues.apache.org/jira/browse/AURORA-1225


Repository: aurora


Description
---

Modify the callback function passed to StatusManager to handle more 
StatusResult.

Currently, Aurora executor passes a callback to a status manager. The status 
manager periodically polls the status of HealthChecker. When a non-empty status 
is deteted, the callback function will shut down the status manager. 

In health check driven updates, the health checker will change its status to 
"TASK_RUNNING" when a successful required number of health checks is reached. 
Therefore, we need to modify the callback function, so that it won't shutdown 
the status manager if the task is in non-exit state like "TASK_RUNNING".

Please see this epic: https://issues.apache.org/jira/browse/AURORA-1225
and the design doc: 
https://docs.google.com/document/d/1ZdgW8S4xMhvKW7iQUX99xZm10NXSxEWR0a-21FP5d94/edit#
 for more details and background.


Diffs
-

  src/main/python/apache/aurora/executor/aurora_executor.py 
ce5ef680f01831cd89fced8969ae3246c7f60cfd 
  src/main/python/apache/aurora/executor/status_manager.py 
228a99a05f339e21cd7e769a42b9b2276e7bc3fc 
  src/test/python/apache/aurora/executor/test_status_manager.py 
ce4679ba1aa7b42cf0115c943d84663030182d23 

Diff: https://reviews.apache.org/r/51742/diff/


Testing
---

./gradlew build

./build-support/jenkins/build.sh


Thanks,

Kai Huang

Re: Review Request 51662: AURORA-1602 Aurora admin commands for reconcilation

[Maxim Khutornenko]

> On Sept. 8, 2016, 6:43 p.m., Santhosh Kumar Shanmugham wrote:
> > src/test/python/apache/aurora/admin/test_admin.py, line 285
> > 
> >
> > Should this also be assert_called_once_with(None) ?
> 
> Karthik Anantha Padmanabhan wrote:
> No - I set the option as 500 in the test and I want to test that we call 
> with the provided option

Don't use `assert_called_with`. Use `mock_calls` instead as a generally more 
robust way to assert the exact number of calls. There are plenty of examples 
around.


- Maxim


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


On Sept. 8, 2016, 5:27 p.m., Karthik Anantha Padmanabhan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51662/
> ---
> 
> (Updated Sept. 8, 2016, 5:27 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> AURORA-1602 Aurora admin commands for reconcilation
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md bbf7198dc7ce53bb02a1bc59f75a661e23472913 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   
> src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java 
> 3275d72a0a74909e635bce615e2036ec72aa38ee 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   src/main/python/apache/aurora/admin/admin.py 
> 76009b9c1c7a130c25abad544a176dc590dafb12 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 9149c3018ae58d405f284fcbd4076d251ccc8192 
>   
> src/test/java/org/apache/aurora/scheduler/reconciliation/TaskReconcilerTest.java
>  b9317dc20456f90057ec2bf4d10619a5ae986187 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  779dc302602ae8842084807ca868a491ea99b676 
>   src/test/python/apache/aurora/admin/test_admin.py 
> eb193c4a33f5275f05a995338446bec0e19bc1cf 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afac2500551af2fce406bb906aa4e33f353e90a1 
> 
> Diff: https://reviews.apache.org/r/51662/diff/
> 
> 
> Testing
> ---
> 
> * Manually tested on my local vagrant installation
> * ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Karthik Anantha Padmanabhan
> 
>

Re: Review Request 51662: AURORA-1602 Aurora admin commands for reconcilation

[Maxim Khutornenko]

> On Sept. 8, 2016, 6:18 p.m., Maxim Khutornenko wrote:
> > src/main/python/apache/aurora/admin/admin.py, line 351
> > 
> >
> > Needs clarifcation that `reconciliation_explicit_batch_size` is a 
> > scheduler setting.
> 
> Karthik Anantha Padmanabhan wrote:
> Good question. It does not show up in the docs. However it is a command 
> line argument in the `ReconciliationModule`. So should this be a scheduler 
> setting ?

By 'setting' I meant the command line argument. Without any explanation it's 
hard to grok what that means.


> On Sept. 8, 2016, 6:18 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java,
> >  line 123
> > 
> >
> > Prefer using immutable `IExplicit...` wrapper generated for you by the 
> > build.
> 
> Karthik Anantha Padmanabhan wrote:
> With the `IExplicit...` version I do not have the ability to check if the 
> field is set or not using `isSet...`. I use that in order to provide the 
> default values for the batch size.

What if it's set to 0 or negative value instead? Don't you want to avoid that 
scenario?


- Maxim


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


On Sept. 8, 2016, 5:27 p.m., Karthik Anantha Padmanabhan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51662/
> ---
> 
> (Updated Sept. 8, 2016, 5:27 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> AURORA-1602 Aurora admin commands for reconcilation
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md bbf7198dc7ce53bb02a1bc59f75a661e23472913 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   
> src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java 
> 3275d72a0a74909e635bce615e2036ec72aa38ee 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   src/main/python/apache/aurora/admin/admin.py 
> 76009b9c1c7a130c25abad544a176dc590dafb12 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 9149c3018ae58d405f284fcbd4076d251ccc8192 
>   
> src/test/java/org/apache/aurora/scheduler/reconciliation/TaskReconcilerTest.java
>  b9317dc20456f90057ec2bf4d10619a5ae986187 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  779dc302602ae8842084807ca868a491ea99b676 
>   src/test/python/apache/aurora/admin/test_admin.py 
> eb193c4a33f5275f05a995338446bec0e19bc1cf 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afac2500551af2fce406bb906aa4e33f353e90a1 
> 
> Diff: https://reviews.apache.org/r/51662/diff/
> 
> 
> Testing
> ---
> 
> * Manually tested on my local vagrant installation
> * ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Karthik Anantha Padmanabhan
> 
>

Re: Review Request 51742: Modify the callback function passed to StatusManager

[Aurora ReviewBot]

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


Ship it!




Master (8fca745) 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 Sept. 8, 2016, 8:07 p.m., Kai Huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51742/
> ---
> 
> (Updated Sept. 8, 2016, 8:07 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Bugs: AURORA-1225
> https://issues.apache.org/jira/browse/AURORA-1225
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Modify the callback function passed to StatusManager to handle more 
> StatusResult.
> 
> Currently, Aurora executor passes a callback to a status manager. The status 
> manager periodically polls the status of HealthChecker. When a non-empty 
> status is deteted, the callback function will shut down the status manager. 
> 
> In health check driven updates, the health checker will change its status to 
> "TASK_RUNNING" when a successful required number of health checks is reached. 
> Therefore, we need to modify the callback function, so that it won't shutdown 
> the status manager if the task is in non-exit state like "TASK_RUNNING".
> 
> Please see this epic: https://issues.apache.org/jira/browse/AURORA-1225
> and the design doc: 
> https://docs.google.com/document/d/1ZdgW8S4xMhvKW7iQUX99xZm10NXSxEWR0a-21FP5d94/edit#
>  for more details and background.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/aurora_executor.py 
> ce5ef680f01831cd89fced8969ae3246c7f60cfd 
>   src/main/python/apache/aurora/executor/status_manager.py 
> 228a99a05f339e21cd7e769a42b9b2276e7bc3fc 
>   src/test/python/apache/aurora/executor/test_status_manager.py 
> ce4679ba1aa7b42cf0115c943d84663030182d23 
> 
> Diff: https://reviews.apache.org/r/51742/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Kai Huang
> 
>

Review Request 51746: Update e2e tests to verify running a docker image via the unified containerizer.

[Joshua Cohen]

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

Review request for Aurora, Stephan Erb and Zhitao Li.


Repository: aurora


Description
---

Update e2e tests to verify running a docker image via the unified containerizer.


Diffs
-

  examples/vagrant/mesos_config/etc_mesos-slave/docker_registry PRE-CREATION 
  examples/vagrant/mesos_config/etc_mesos-slave/image_providers 
7168c0ca4a81e0a1ce2320534015c6692b804b54 
  examples/vagrant/mesos_config/etc_mesos-slave/isolation 
c36223028513ace26040620b813022accaf0edf3 
  src/test/sh/org/apache/aurora/e2e/http/http_example.aurora 
042424d408a6b14be05c6e1c967215a53836d20e 
  src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
f80f2b1fe8accfa575b2c3e626feb8d926455f68 
  src/test/sh/org/apache/aurora/e2e/http/http_example_updated.aurora 
4a95bc3a3d1ce50ea143266884efc4d0a7c6e767 
  src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
d81abe1f974bfb13b54a60b2018700aaf28a8ee6 

Diff: https://reviews.apache.org/r/51746/diff/


Testing
---


Thanks,

Joshua Cohen

Re: Review Request 51742: Modify the callback function passed to StatusManager

[Joshua Cohen]

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




src/main/python/apache/aurora/executor/aurora_executor.py (line 20)


we already import `mesos_pb2` above, so instead of importing `TaskState` 
here, just refer to it below as `mesos_pb2.TaskState`?



src/main/python/apache/aurora/executor/aurora_executor.py (lines 204 - 211)


I see a couple of problems with this method:

1. The `status_result` argument passed in by `StatusManager#run` is not a 
`TaskState`, it's a 
[StatusResult](https://github.com/apache/aurora/blob/master/src/main/python/apache/aurora/executor/common/status_checker.py#L23-L50)
 which has a `status` property whose value is a `TaskState`. So I believe your 
first check will always fail in a real world scenario?
2. If we receive multiple invocations of this callback for healthy tasks, 
the second callback will result in the healthy task being killed (since 
`self.task_running` will be `True`, the initial condition will evaluate to 
`False` and we'll land in the shutdown branch).

These two issues combined make me wary of shipping these changes without 
the corresponding changes further down the stack, as it makes it hard to 
evaluate their actual effect. I appreciate trying to break the review up into 
manageable chunks, but in this case I think it's having the opposite effect of 
causing reviewers to have to envision the contents of future reviews to 
understand the impact of the changes in this review. On top of that, the 
doubling up of sending the `TASK_RUNNING` update while the code is in the 
in-between state also makes me nervous.

We also definitely need test coverage for this callback.



src/main/python/apache/aurora/executor/aurora_executor.py (line 206)


We should still send the 'Task is healthy' message with this update?



src/main/python/apache/aurora/executor/status_manager.py (line 34)


Please add a pydoc comment explaining the expected interface of the 
callback (i.e. it should return `True` if the handled status is terminal).


- Joshua Cohen


On Sept. 8, 2016, 8:07 p.m., Kai Huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51742/
> ---
> 
> (Updated Sept. 8, 2016, 8:07 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Bugs: AURORA-1225
> https://issues.apache.org/jira/browse/AURORA-1225
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Modify the callback function passed to StatusManager to handle more 
> StatusResult.
> 
> Currently, Aurora executor passes a callback to a status manager. The status 
> manager periodically polls the status of HealthChecker. When a non-empty 
> status is deteted, the callback function will shut down the status manager. 
> 
> In health check driven updates, the health checker will change its status to 
> "TASK_RUNNING" when a successful required number of health checks is reached. 
> Therefore, we need to modify the callback function, so that it won't shutdown 
> the status manager if the task is in non-exit state like "TASK_RUNNING".
> 
> Please see this epic: https://issues.apache.org/jira/browse/AURORA-1225
> and the design doc: 
> https://docs.google.com/document/d/1ZdgW8S4xMhvKW7iQUX99xZm10NXSxEWR0a-21FP5d94/edit#
>  for more details and background.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/aurora_executor.py 
> ce5ef680f01831cd89fced8969ae3246c7f60cfd 
>   src/main/python/apache/aurora/executor/status_manager.py 
> 228a99a05f339e21cd7e769a42b9b2276e7bc3fc 
>   src/test/python/apache/aurora/executor/test_status_manager.py 
> ce4679ba1aa7b42cf0115c943d84663030182d23 
> 
> Diff: https://reviews.apache.org/r/51742/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Kai Huang
> 
>

Re: Review Request 51746: Update e2e tests to verify running a docker image via the unified containerizer.

[Zhitao Li]

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


Ship it!




Ship It!

- Zhitao Li


On Sept. 8, 2016, 8:56 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51746/
> ---
> 
> (Updated Sept. 8, 2016, 8:56 p.m.)
> 
> 
> Review request for Aurora, Stephan Erb and Zhitao Li.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Update e2e tests to verify running a docker image via the unified 
> containerizer.
> 
> 
> Diffs
> -
> 
>   examples/vagrant/mesos_config/etc_mesos-slave/docker_registry PRE-CREATION 
>   examples/vagrant/mesos_config/etc_mesos-slave/image_providers 
> 7168c0ca4a81e0a1ce2320534015c6692b804b54 
>   examples/vagrant/mesos_config/etc_mesos-slave/isolation 
> c36223028513ace26040620b813022accaf0edf3 
>   src/test/sh/org/apache/aurora/e2e/http/http_example.aurora 
> 042424d408a6b14be05c6e1c967215a53836d20e 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> f80f2b1fe8accfa575b2c3e626feb8d926455f68 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_updated.aurora 
> 4a95bc3a3d1ce50ea143266884efc4d0a7c6e767 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
> d81abe1f974bfb13b54a60b2018700aaf28a8ee6 
> 
> Diff: https://reviews.apache.org/r/51746/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 51662: AURORA-1602 Aurora admin commands for reconcilation

[Santhosh Kumar Shanmugham]

> On Sept. 8, 2016, 11:43 a.m., Santhosh Kumar Shanmugham wrote:
> > src/test/python/apache/aurora/admin/test_admin.py, line 285
> > 
> >
> > Should this also be assert_called_once_with(None) ?
> 
> Karthik Anantha Padmanabhan wrote:
> No - I set the option as 500 in the test and I want to test that we call 
> with the provided option
> 
> Maxim Khutornenko wrote:
> Don't use `assert_called_with`. Use `mock_calls` instead as a generally 
> more robust way to assert the exact number of calls. There are plenty of 
> examples around.

Typo - I meant assert_called_once_with(500), but +1 for using mock_calls.


- Santhosh Kumar


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


On Sept. 8, 2016, 10:27 a.m., Karthik Anantha Padmanabhan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51662/
> ---
> 
> (Updated Sept. 8, 2016, 10:27 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> AURORA-1602 Aurora admin commands for reconcilation
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md bbf7198dc7ce53bb02a1bc59f75a661e23472913 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   
> src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java 
> 3275d72a0a74909e635bce615e2036ec72aa38ee 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   src/main/python/apache/aurora/admin/admin.py 
> 76009b9c1c7a130c25abad544a176dc590dafb12 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 9149c3018ae58d405f284fcbd4076d251ccc8192 
>   
> src/test/java/org/apache/aurora/scheduler/reconciliation/TaskReconcilerTest.java
>  b9317dc20456f90057ec2bf4d10619a5ae986187 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  779dc302602ae8842084807ca868a491ea99b676 
>   src/test/python/apache/aurora/admin/test_admin.py 
> eb193c4a33f5275f05a995338446bec0e19bc1cf 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afac2500551af2fce406bb906aa4e33f353e90a1 
> 
> Diff: https://reviews.apache.org/r/51662/diff/
> 
> 
> Testing
> ---
> 
> * Manually tested on my local vagrant installation
> * ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Karthik Anantha Padmanabhan
> 
>

Re: Review Request 51746: Update e2e tests to verify running a docker image via the unified containerizer.

[Aurora ReviewBot]

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


Ship it!




Master (8fca745) 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 Sept. 8, 2016, 8:56 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51746/
> ---
> 
> (Updated Sept. 8, 2016, 8:56 p.m.)
> 
> 
> Review request for Aurora, Stephan Erb and Zhitao Li.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Update e2e tests to verify running a docker image via the unified 
> containerizer.
> 
> 
> Diffs
> -
> 
>   examples/vagrant/mesos_config/etc_mesos-slave/docker_registry PRE-CREATION 
>   examples/vagrant/mesos_config/etc_mesos-slave/image_providers 
> 7168c0ca4a81e0a1ce2320534015c6692b804b54 
>   examples/vagrant/mesos_config/etc_mesos-slave/isolation 
> c36223028513ace26040620b813022accaf0edf3 
>   src/test/sh/org/apache/aurora/e2e/http/http_example.aurora 
> 042424d408a6b14be05c6e1c967215a53836d20e 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> f80f2b1fe8accfa575b2c3e626feb8d926455f68 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_updated.aurora 
> 4a95bc3a3d1ce50ea143266884efc4d0a7c6e767 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
> d81abe1f974bfb13b54a60b2018700aaf28a8ee6 
> 
> Diff: https://reviews.apache.org/r/51746/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 51742: Modify the callback function passed to StatusManager

[Kai Huang]

> On Sept. 8, 2016, 9:13 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/executor/aurora_executor.py, lines 204-211
> > 
> >
> > I see a couple of problems with this method:
> > 
> > 1. The `status_result` argument passed in by `StatusManager#run` is not 
> > a `TaskState`, it's a 
> > [StatusResult](https://github.com/apache/aurora/blob/master/src/main/python/apache/aurora/executor/common/status_checker.py#L23-L50)
> >  which has a `status` property whose value is a `TaskState`. So I believe 
> > your first check will always fail in a real world scenario?
> > 2. If we receive multiple invocations of this callback for healthy 
> > tasks, the second callback will result in the healthy task being killed 
> > (since `self.task_running` will be `True`, the initial condition will 
> > evaluate to `False` and we'll land in the shutdown branch).
> > 
> > These two issues combined make me wary of shipping these changes 
> > without the corresponding changes further down the stack, as it makes it 
> > hard to evaluate their actual effect. I appreciate trying to break the 
> > review up into manageable chunks, but in this case I think it's having the 
> > opposite effect of causing reviewers to have to envision the contents of 
> > future reviews to understand the impact of the changes in this review. On 
> > top of that, the doubling up of sending the `TASK_RUNNING` update while the 
> > code is in the in-between state also makes me nervous.
> > 
> > We also definitely need test coverage for this callback.

Thanks for the valid feedback. I'll work on them.


- Kai


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


On Sept. 8, 2016, 8:07 p.m., Kai Huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51742/
> ---
> 
> (Updated Sept. 8, 2016, 8:07 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Bugs: AURORA-1225
> https://issues.apache.org/jira/browse/AURORA-1225
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Modify the callback function passed to StatusManager to handle more 
> StatusResult.
> 
> Currently, Aurora executor passes a callback to a status manager. The status 
> manager periodically polls the status of HealthChecker. When a non-empty 
> status is deteted, the callback function will shut down the status manager. 
> 
> In health check driven updates, the health checker will change its status to 
> "TASK_RUNNING" when a successful required number of health checks is reached. 
> Therefore, we need to modify the callback function, so that it won't shutdown 
> the status manager if the task is in non-exit state like "TASK_RUNNING".
> 
> Please see this epic: https://issues.apache.org/jira/browse/AURORA-1225
> and the design doc: 
> https://docs.google.com/document/d/1ZdgW8S4xMhvKW7iQUX99xZm10NXSxEWR0a-21FP5d94/edit#
>  for more details and background.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/aurora_executor.py 
> ce5ef680f01831cd89fced8969ae3246c7f60cfd 
>   src/main/python/apache/aurora/executor/status_manager.py 
> 228a99a05f339e21cd7e769a42b9b2276e7bc3fc 
>   src/test/python/apache/aurora/executor/test_status_manager.py 
> ce4679ba1aa7b42cf0115c943d84663030182d23 
> 
> Diff: https://reviews.apache.org/r/51742/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Kai Huang
> 
>

Re: Review Request 51746: Update e2e tests to verify running a docker image via the unified containerizer.

[Stephan Erb]

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


Ship it!




Ship It!

- Stephan Erb


On Sept. 8, 2016, 10:56 p.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51746/
> ---
> 
> (Updated Sept. 8, 2016, 10:56 p.m.)
> 
> 
> Review request for Aurora, Stephan Erb and Zhitao Li.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Update e2e tests to verify running a docker image via the unified 
> containerizer.
> 
> 
> Diffs
> -
> 
>   examples/vagrant/mesos_config/etc_mesos-slave/docker_registry PRE-CREATION 
>   examples/vagrant/mesos_config/etc_mesos-slave/image_providers 
> 7168c0ca4a81e0a1ce2320534015c6692b804b54 
>   examples/vagrant/mesos_config/etc_mesos-slave/isolation 
> c36223028513ace26040620b813022accaf0edf3 
>   src/test/sh/org/apache/aurora/e2e/http/http_example.aurora 
> 042424d408a6b14be05c6e1c967215a53836d20e 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> f80f2b1fe8accfa575b2c3e626feb8d926455f68 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_updated.aurora 
> 4a95bc3a3d1ce50ea143266884efc4d0a7c6e767 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
> d81abe1f974bfb13b54a60b2018700aaf28a8ee6 
> 
> Diff: https://reviews.apache.org/r/51746/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 51664: Document the Mesos containerizer

[Joshua Cohen]

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



@ReviewBot retry

- Joshua Cohen


On Sept. 6, 2016, 7:18 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51664/
> ---
> 
> (Updated Sept. 6, 2016, 7:18 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Bugs: AURORA-1640
> https://issues.apache.org/jira/browse/AURORA-1640
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Included changes:
> 
> * enabled Docker support in our vagrant box
> * consistent example jobs for both containerizers
> * short enduser and operator  documentation
> * shuffled the reference documentation so that it is clear certain 
> limitations apply only to the Docker containerizer
> 
> 
> Diffs
> -
> 
>   docs/features/containers.md 6b9f717d672ef88be61341268f9c06923d217087 
>   docs/operations/configuration.md 85787b0815e9f0cb37c21efc04923b0e0bd10bf9 
>   docs/reference/configuration.md ff40262f1fb4eff780fbef17abcaecc457dc68d3 
>   examples/jobs/docker/hello_docker.aurora 
> d5611e6b78cb613c04e2fc3df54397ee4401a62e 
>   examples/jobs/hello_docker_image.aurora PRE-CREATION 
>   examples/vagrant/aurorabuild.sh 9daca067acc05c89ccc183e56bf30c787baf97dd 
>   examples/vagrant/mesos_config/etc_mesos-slave/image_providers 
> 7168c0ca4a81e0a1ce2320534015c6692b804b54 
>   examples/vagrant/mesos_config/etc_mesos-slave/isolation 
> c36223028513ace26040620b813022accaf0edf3 
> 
> Diff: https://reviews.apache.org/r/51664/diff/
> 
> 
> Testing
> ---
> 
> Rendered version available at 
> https://github.com/StephanErb/aurora/tree/containerizer-docs/docs
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 51742: Modify the callback function passed to StatusManager

[Kai Huang]

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




src/test/python/apache/aurora/executor/test_status_manager.py (line 43)


I think the param result in the callback fucntion here should be 
StatusResult as well, not TaskState.Value('TASK_KILLED')? This test callback 
function is a little bit inconsistent with the callback in status_mananger.py, 
but I guess it's OK.


- Kai Huang


On Sept. 8, 2016, 8:07 p.m., Kai Huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51742/
> ---
> 
> (Updated Sept. 8, 2016, 8:07 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Bugs: AURORA-1225
> https://issues.apache.org/jira/browse/AURORA-1225
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Modify the callback function passed to StatusManager to handle more 
> StatusResult.
> 
> Currently, Aurora executor passes a callback to a status manager. The status 
> manager periodically polls the status of HealthChecker. When a non-empty 
> status is deteted, the callback function will shut down the status manager. 
> 
> In health check driven updates, the health checker will change its status to 
> "TASK_RUNNING" when a successful required number of health checks is reached. 
> Therefore, we need to modify the callback function, so that it won't shutdown 
> the status manager if the task is in non-exit state like "TASK_RUNNING".
> 
> Please see this epic: https://issues.apache.org/jira/browse/AURORA-1225
> and the design doc: 
> https://docs.google.com/document/d/1ZdgW8S4xMhvKW7iQUX99xZm10NXSxEWR0a-21FP5d94/edit#
>  for more details and background.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/aurora_executor.py 
> ce5ef680f01831cd89fced8969ae3246c7f60cfd 
>   src/main/python/apache/aurora/executor/status_manager.py 
> 228a99a05f339e21cd7e769a42b9b2276e7bc3fc 
>   src/test/python/apache/aurora/executor/test_status_manager.py 
> ce4679ba1aa7b42cf0115c943d84663030182d23 
> 
> Diff: https://reviews.apache.org/r/51742/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Kai Huang
> 
>

Re: Review Request 51664: Document the Mesos containerizer

[Aurora ReviewBot]

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


Ship it!




Master (87ae968) 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 Sept. 6, 2016, 7:18 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51664/
> ---
> 
> (Updated Sept. 6, 2016, 7:18 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Bugs: AURORA-1640
> https://issues.apache.org/jira/browse/AURORA-1640
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Included changes:
> 
> * enabled Docker support in our vagrant box
> * consistent example jobs for both containerizers
> * short enduser and operator  documentation
> * shuffled the reference documentation so that it is clear certain 
> limitations apply only to the Docker containerizer
> 
> 
> Diffs
> -
> 
>   docs/features/containers.md 6b9f717d672ef88be61341268f9c06923d217087 
>   docs/operations/configuration.md 85787b0815e9f0cb37c21efc04923b0e0bd10bf9 
>   docs/reference/configuration.md ff40262f1fb4eff780fbef17abcaecc457dc68d3 
>   examples/jobs/docker/hello_docker.aurora 
> d5611e6b78cb613c04e2fc3df54397ee4401a62e 
>   examples/jobs/hello_docker_image.aurora PRE-CREATION 
>   examples/vagrant/aurorabuild.sh 9daca067acc05c89ccc183e56bf30c787baf97dd 
>   examples/vagrant/mesos_config/etc_mesos-slave/image_providers 
> 7168c0ca4a81e0a1ce2320534015c6692b804b54 
>   examples/vagrant/mesos_config/etc_mesos-slave/isolation 
> c36223028513ace26040620b813022accaf0edf3 
> 
> Diff: https://reviews.apache.org/r/51664/diff/
> 
> 
> Testing
> ---
> 
> Rendered version available at 
> https://github.com/StephanErb/aurora/tree/containerizer-docs/docs
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 51742: Modify the callback function passed to StatusManager

[Kai Huang]

> On Sept. 8, 2016, 9:13 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/executor/aurora_executor.py, lines 204-211
> > 
> >
> > I see a couple of problems with this method:
> > 
> > 1. The `status_result` argument passed in by `StatusManager#run` is not 
> > a `TaskState`, it's a 
> > [StatusResult](https://github.com/apache/aurora/blob/master/src/main/python/apache/aurora/executor/common/status_checker.py#L23-L50)
> >  which has a `status` property whose value is a `TaskState`. So I believe 
> > your first check will always fail in a real world scenario?
> > 2. If we receive multiple invocations of this callback for healthy 
> > tasks, the second callback will result in the healthy task being killed 
> > (since `self.task_running` will be `True`, the initial condition will 
> > evaluate to `False` and we'll land in the shutdown branch).
> > 
> > These two issues combined make me wary of shipping these changes 
> > without the corresponding changes further down the stack, as it makes it 
> > hard to evaluate their actual effect. I appreciate trying to break the 
> > review up into manageable chunks, but in this case I think it's having the 
> > opposite effect of causing reviewers to have to envision the contents of 
> > future reviews to understand the impact of the changes in this review. On 
> > top of that, the doubling up of sending the `TASK_RUNNING` update while the 
> > code is in the in-between state also makes me nervous.
> > 
> > We also definitely need test coverage for this callback.
> 
> Kai Huang wrote:
> Thanks for the valid feedback. I'll work on them.

Given that _status_result_handler is an instance method of Aurora executor, the 
unit test of _status_result_handler is kind of interwined in the following 
tests in test_thermos_executor.py:
test_shutdown
test_task_health_check_ok
test_task_health_failed, etc.

The complexity results from the attribute "task_running" in Aurora Executor, 
which makes it hard to unit test this function. I'll think about if there is 
other way than relying on the "task_running" field in aurora_executor.


- Kai


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


On Sept. 8, 2016, 8:07 p.m., Kai Huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51742/
> ---
> 
> (Updated Sept. 8, 2016, 8:07 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Bugs: AURORA-1225
> https://issues.apache.org/jira/browse/AURORA-1225
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Modify the callback function passed to StatusManager to handle more 
> StatusResult.
> 
> Currently, Aurora executor passes a callback to a status manager. The status 
> manager periodically polls the status of HealthChecker. When a non-empty 
> status is deteted, the callback function will shut down the status manager. 
> 
> In health check driven updates, the health checker will change its status to 
> "TASK_RUNNING" when a successful required number of health checks is reached. 
> Therefore, we need to modify the callback function, so that it won't shutdown 
> the status manager if the task is in non-exit state like "TASK_RUNNING".
> 
> Please see this epic: https://issues.apache.org/jira/browse/AURORA-1225
> and the design doc: 
> https://docs.google.com/document/d/1ZdgW8S4xMhvKW7iQUX99xZm10NXSxEWR0a-21FP5d94/edit#
>  for more details and background.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/aurora_executor.py 
> ce5ef680f01831cd89fced8969ae3246c7f60cfd 
>   src/main/python/apache/aurora/executor/status_manager.py 
> 228a99a05f339e21cd7e769a42b9b2276e7bc3fc 
>   src/test/python/apache/aurora/executor/test_status_manager.py 
> ce4679ba1aa7b42cf0115c943d84663030182d23 
> 
> Diff: https://reviews.apache.org/r/51742/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Kai Huang
> 
>

Re: Review Request 51712: Extend getJobUpdateDetails to accept JobUpdateQuery

[Zameer Manji]

> On Sept. 8, 2016, 9:37 a.m., Maxim Khutornenko wrote:
> > This will most certainly break the UI. The client needs to be updated as 
> > well to pass both (old and new) arguments. Please, test in vagrant with 
> > different combinations of scheduler and client versions.

I have validated that an old client can call the new scheduler. The deprecation 
message in the scheduler is printed out:

vagrant@aurora:~$ aurora update info devcluster/www-data/prod/hello 
85fc8af6-2242-43e3-80d3-1e7d9c6b6e42
 INFO] The key argument is deprecated, use the query argument instead
Job: devcluster/www-data/prod/hello, UpdateID: 
85fc8af6-2242-43e3-80d3-1e7d9c6b6e42
Started 2016-09-08T23:22:32, last activity: 2016-09-08T23:23:19
Current status: ROLLED_FORWARD
Update events:
  Status: ROLLING_FORWARD at 2016-09-08T23:22:32
  Status: ROLLED_FORWARD at 2016-09-08T23:23:19
Instance events:
  Instance 0 at 2016-09-08T23:22:32: INSTANCE_UPDATING
  Instance 0 at 2016-09-08T23:23:19: INSTANCE_UPDATED


I have not yet updated the JS code or the client code to use the query 
parameter to prevent deployment ordering issues.


- Zameer


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


On Sept. 7, 2016, 5:03 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51712/
> ---
> 
> (Updated Sept. 7, 2016, 5:03 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1764
> https://issues.apache.org/jira/browse/AURORA-1764
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This extends getJobUpdateDetails to return a list of details instead of being 
> scoped to a single update.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md d79aaadc197697d09a71c83494a01765d6a983d4 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 4202f0eec59ef16668fca6b6ebb925335ad5dea1 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  a7d1f74acdfe5f58eb822a4d826e920cad53dced 
> 
> Diff: https://reviews.apache.org/r/51712/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 51712: Extend getJobUpdateDetails to accept JobUpdateQuery

[Zameer Manji]

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

(Updated Sept. 8, 2016, 4:49 p.m.)


Review request for Aurora, Joshua Cohen and Maxim Khutornenko.


Bugs: AURORA-1764
https://issues.apache.org/jira/browse/AURORA-1764


Repository: aurora


Description
---

This extends getJobUpdateDetails to return a list of details instead of being 
scoped to a single update.


Diffs (updated)
-

  RELEASE-NOTES.md bbf7198dc7ce53bb02a1bc59f75a661e23472913 
  api/src/main/thrift/org/apache/aurora/gen/api.thrift 
c5765b70501c101f0535b4eed94e9948c36808f9 
  src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
4202f0eec59ef16668fca6b6ebb925335ad5dea1 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
929d021a336c6a3438613c9340c84a1096dc9069 
  src/main/python/apache/aurora/client/api/__init__.py 
9149c3018ae58d405f284fcbd4076d251ccc8192 
  src/main/resources/scheduler/assets/js/services.js 
315fc17894c2261333715fbb9c45fc02505f2942 
  
src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java 
a7d1f74acdfe5f58eb822a4d826e920cad53dced 
  src/test/python/apache/aurora/client/api/test_api.py 
7a0797d5402e931006d4f215e2d9c0dbbd113257 

Diff: https://reviews.apache.org/r/51712/diff/


Testing
---


Thanks,

Zameer Manji

Re: Review Request 51712: Extend getJobUpdateDetails to accept JobUpdateQuery

[Zameer Manji]

> On Sept. 8, 2016, 1:19 a.m., Stephan Erb wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift, lines 899-900
> > 
> >
> > If this is deprecated, shouldn't you call this out in the release notes 
> > ad well?

Done.


- Zameer


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


On Sept. 7, 2016, 5:03 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51712/
> ---
> 
> (Updated Sept. 7, 2016, 5:03 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1764
> https://issues.apache.org/jira/browse/AURORA-1764
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This extends getJobUpdateDetails to return a list of details instead of being 
> scoped to a single update.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md d79aaadc197697d09a71c83494a01765d6a983d4 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 4202f0eec59ef16668fca6b6ebb925335ad5dea1 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  a7d1f74acdfe5f58eb822a4d826e920cad53dced 
> 
> Diff: https://reviews.apache.org/r/51712/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 51712: Extend getJobUpdateDetails to accept JobUpdateQuery

[Aurora ReviewBot]

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



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

:commons:classes
:commons:jar
:compileJava/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java:74:
 Note: Wrote forwarder 
org.apache.aurora.scheduler.storage.log.WriteAheadStorageForwarder
@Forward({
^
Note: Writing 
file:/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/classes/main/org/apache/aurora/common/args/apt/cmdline.arg.info.txt.2
Note: Writing 
file:/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/classes/main/META-INF/compiler/resource-mappings/org.apache.aurora.common.args.apt.CmdLineProcessor

:generateBuildProperties
:processResources
:classes
:jar
:startScripts
:distTar
:distZip
:assemble
:compileJmhJavaNote: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java
 uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.

:processJmhResources UP-TO-DATE
:jmhClasses
:checkstyleJmh
:jsHint
Line is too long. 
(/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/resources/scheduler/assets/js/services.js:170:106)
> auroraClient.getSchedulerClient().getJobUpdateDetails(updateKey, null, 
> function (response) {

:jsHint FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':jsHint'.
> Process 'command '/usr/local/asfpackages/java/jdk1.8.0_102/bin/java'' 
> finished with non-zero exit value 2

* 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: 1 mins 17.48 secs


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

- Aurora ReviewBot


On Sept. 8, 2016, 11:49 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51712/
> ---
> 
> (Updated Sept. 8, 2016, 11:49 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1764
> https://issues.apache.org/jira/browse/AURORA-1764
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This extends getJobUpdateDetails to return a list of details instead of being 
> scoped to a single update.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md bbf7198dc7ce53bb02a1bc59f75a661e23472913 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 4202f0eec59ef16668fca6b6ebb925335ad5dea1 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 9149c3018ae58d405f284fcbd4076d251ccc8192 
>   src/main/resources/scheduler/assets/js/services.js 
> 315fc17894c2261333715fbb9c45fc02505f2942 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  a7d1f74acdfe5f58eb822a4d826e920cad53dced 
>   src/test/python/apache/aurora/client/api/test_api.py 
> 7a0797d5402e931006d4f215e2d9c0dbbd113257 
> 
> Diff: https://reviews.apache.org/r/51712/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 51662: AURORA-1602 Aurora admin commands for reconcilation

[Karthik Anantha Padmanabhan]

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

(Updated Sept. 9, 2016, 12:01 a.m.)


Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.


Changes
---

More code review changes.


Repository: aurora


Description
---

AURORA-1602 Aurora admin commands for reconcilation


Diffs (updated)
-

  RELEASE-NOTES.md bbf7198dc7ce53bb02a1bc59f75a661e23472913 
  api/src/main/thrift/org/apache/aurora/gen/api.thrift 
c5765b70501c101f0535b4eed94e9948c36808f9 
  src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java 
3275d72a0a74909e635bce615e2036ec72aa38ee 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
929d021a336c6a3438613c9340c84a1096dc9069 
  src/main/python/apache/aurora/admin/admin.py 
76009b9c1c7a130c25abad544a176dc590dafb12 
  src/main/python/apache/aurora/client/api/__init__.py 
9149c3018ae58d405f284fcbd4076d251ccc8192 
  
src/test/java/org/apache/aurora/scheduler/reconciliation/TaskReconcilerTest.java
 b9317dc20456f90057ec2bf4d10619a5ae986187 
  
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 779dc302602ae8842084807ca868a491ea99b676 
  src/test/python/apache/aurora/admin/test_admin.py 
eb193c4a33f5275f05a995338446bec0e19bc1cf 
  src/test/python/apache/aurora/client/api/test_scheduler_client.py 
afac2500551af2fce406bb906aa4e33f353e90a1 

Diff: https://reviews.apache.org/r/51662/diff/


Testing
---

* Manually tested on my local vagrant installation
* ./build-support/jenkins/build.sh


Thanks,

Karthik Anantha Padmanabhan

Re: Review Request 51742: Modify the callback function passed to StatusManager

[Kai Huang]

> On Sept. 8, 2016, 9:13 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/executor/aurora_executor.py, lines 204-211
> > 
> >
> > I see a couple of problems with this method:
> > 
> > 1. The `status_result` argument passed in by `StatusManager#run` is not 
> > a `TaskState`, it's a 
> > [StatusResult](https://github.com/apache/aurora/blob/master/src/main/python/apache/aurora/executor/common/status_checker.py#L23-L50)
> >  which has a `status` property whose value is a `TaskState`. So I believe 
> > your first check will always fail in a real world scenario?
> > 2. If we receive multiple invocations of this callback for healthy 
> > tasks, the second callback will result in the healthy task being killed 
> > (since `self.task_running` will be `True`, the initial condition will 
> > evaluate to `False` and we'll land in the shutdown branch).
> > 
> > These two issues combined make me wary of shipping these changes 
> > without the corresponding changes further down the stack, as it makes it 
> > hard to evaluate their actual effect. I appreciate trying to break the 
> > review up into manageable chunks, but in this case I think it's having the 
> > opposite effect of causing reviewers to have to envision the contents of 
> > future reviews to understand the impact of the changes in this review. On 
> > top of that, the doubling up of sending the `TASK_RUNNING` update while the 
> > code is in the in-between state also makes me nervous.
> > 
> > We also definitely need test coverage for this callback.
> 
> Kai Huang wrote:
> Thanks for the valid feedback. I'll work on them.
> 
> Kai Huang wrote:
> Given that _status_result_handler is an instance method of Aurora 
> executor, the unit test of _status_result_handler is kind of interwined in 
> the following tests in test_thermos_executor.py:
> test_shutdown
> test_task_health_check_ok
> test_task_health_failed, etc.
> 
> The complexity results from the attribute "task_running" in Aurora 
> Executor, which makes it hard to unit test this function. I'll think about if 
> there is other way than relying on the "task_running" field in 
> aurora_executor.

Since the major blocker here is that we don't want to trigger the callback to 
handle TASK_RUNNING status more than once, I suggest delegating the "check 
once" logic from Aurora Executor into status manager, because that will make 
the unit tests much easier.


- Kai


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


On Sept. 8, 2016, 8:07 p.m., Kai Huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51742/
> ---
> 
> (Updated Sept. 8, 2016, 8:07 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Bugs: AURORA-1225
> https://issues.apache.org/jira/browse/AURORA-1225
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Modify the callback function passed to StatusManager to handle more 
> StatusResult.
> 
> Currently, Aurora executor passes a callback to a status manager. The status 
> manager periodically polls the status of HealthChecker. When a non-empty 
> status is deteted, the callback function will shut down the status manager. 
> 
> In health check driven updates, the health checker will change its status to 
> "TASK_RUNNING" when a successful required number of health checks is reached. 
> Therefore, we need to modify the callback function, so that it won't shutdown 
> the status manager if the task is in non-exit state like "TASK_RUNNING".
> 
> Please see this epic: https://issues.apache.org/jira/browse/AURORA-1225
> and the design doc: 
> https://docs.google.com/document/d/1ZdgW8S4xMhvKW7iQUX99xZm10NXSxEWR0a-21FP5d94/edit#
>  for more details and background.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/aurora_executor.py 
> ce5ef680f01831cd89fced8969ae3246c7f60cfd 
>   src/main/python/apache/aurora/executor/status_manager.py 
> 228a99a05f339e21cd7e769a42b9b2276e7bc3fc 
>   src/test/python/apache/aurora/executor/test_status_manager.py 
> ce4679ba1aa7b42cf0115c943d84663030182d23 
> 
> Diff: https://reviews.apache.org/r/51742/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Kai Huang
> 
>

Re: Review Request 51662: AURORA-1602 Aurora admin commands for reconcilation

[Aurora ReviewBot]

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


Ship it!




Master (87ae968) 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 Sept. 9, 2016, 12:01 a.m., Karthik Anantha Padmanabhan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51662/
> ---
> 
> (Updated Sept. 9, 2016, 12:01 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> AURORA-1602 Aurora admin commands for reconcilation
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md bbf7198dc7ce53bb02a1bc59f75a661e23472913 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   
> src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java 
> 3275d72a0a74909e635bce615e2036ec72aa38ee 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   src/main/python/apache/aurora/admin/admin.py 
> 76009b9c1c7a130c25abad544a176dc590dafb12 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 9149c3018ae58d405f284fcbd4076d251ccc8192 
>   
> src/test/java/org/apache/aurora/scheduler/reconciliation/TaskReconcilerTest.java
>  b9317dc20456f90057ec2bf4d10619a5ae986187 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  779dc302602ae8842084807ca868a491ea99b676 
>   src/test/python/apache/aurora/admin/test_admin.py 
> eb193c4a33f5275f05a995338446bec0e19bc1cf 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afac2500551af2fce406bb906aa4e33f353e90a1 
> 
> Diff: https://reviews.apache.org/r/51662/diff/
> 
> 
> Testing
> ---
> 
> * Manually tested on my local vagrant installation
> * ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Karthik Anantha Padmanabhan
> 
>

Re: Review Request 51662: AURORA-1602 Aurora admin commands for reconcilation

[Zameer Manji]

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



Overall LGTM, just some missing tests and missing error handling.


src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java 
(line 125)


This smells to me and seems awkward. I guess this stemms from the fact that 
the `I*` wrappers don't have Optional fields for integers. Instead of this 
here, why not have the signature be 
`triggerExplicitReconciliation(Optional batchSize)`?

You can then do `int trueBatchSize = 
batchSize.orElse(settings.explicitBatchSize)`.

You can then push validation of the value to the RPC/API layer.



src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
(line 639)


As mentioned above, we should do validation of the arguments here so we can 
return a usefull error message if it is invalid. This includes `0` and negative 
values.



src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 (line 1258)


Please add tests for cases where batch size is unset and when it is 0 and 
when it is negative.


- Zameer Manji


On Sept. 8, 2016, 5:01 p.m., Karthik Anantha Padmanabhan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51662/
> ---
> 
> (Updated Sept. 8, 2016, 5:01 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> AURORA-1602 Aurora admin commands for reconcilation
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md bbf7198dc7ce53bb02a1bc59f75a661e23472913 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   
> src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java 
> 3275d72a0a74909e635bce615e2036ec72aa38ee 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   src/main/python/apache/aurora/admin/admin.py 
> 76009b9c1c7a130c25abad544a176dc590dafb12 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 9149c3018ae58d405f284fcbd4076d251ccc8192 
>   
> src/test/java/org/apache/aurora/scheduler/reconciliation/TaskReconcilerTest.java
>  b9317dc20456f90057ec2bf4d10619a5ae986187 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  779dc302602ae8842084807ca868a491ea99b676 
>   src/test/python/apache/aurora/admin/test_admin.py 
> eb193c4a33f5275f05a995338446bec0e19bc1cf 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afac2500551af2fce406bb906aa4e33f353e90a1 
> 
> Diff: https://reviews.apache.org/r/51662/diff/
> 
> 
> Testing
> ---
> 
> * Manually tested on my local vagrant installation
> * ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Karthik Anantha Padmanabhan
> 
>

Re: Review Request 51712: Extend getJobUpdateDetails to accept JobUpdateQuery

[Zameer Manji]

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

(Updated Sept. 8, 2016, 5:30 p.m.)


Review request for Aurora, Joshua Cohen and Maxim Khutornenko.


Changes
---

Fix JS style issue.


Bugs: AURORA-1764
https://issues.apache.org/jira/browse/AURORA-1764


Repository: aurora


Description
---

This extends getJobUpdateDetails to return a list of details instead of being 
scoped to a single update.


Diffs (updated)
-

  RELEASE-NOTES.md bbf7198dc7ce53bb02a1bc59f75a661e23472913 
  api/src/main/thrift/org/apache/aurora/gen/api.thrift 
c5765b70501c101f0535b4eed94e9948c36808f9 
  src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
4202f0eec59ef16668fca6b6ebb925335ad5dea1 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
929d021a336c6a3438613c9340c84a1096dc9069 
  src/main/python/apache/aurora/client/api/__init__.py 
9149c3018ae58d405f284fcbd4076d251ccc8192 
  src/main/resources/scheduler/assets/js/services.js 
315fc17894c2261333715fbb9c45fc02505f2942 
  
src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java 
a7d1f74acdfe5f58eb822a4d826e920cad53dced 
  src/test/python/apache/aurora/client/api/test_api.py 
7a0797d5402e931006d4f215e2d9c0dbbd113257 

Diff: https://reviews.apache.org/r/51712/diff/


Testing
---


Thanks,

Zameer Manji

Re: Review Request 51712: Extend getJobUpdateDetails to accept JobUpdateQuery

[Aurora ReviewBot]

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


Ship it!




Master (87ae968) 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 Sept. 9, 2016, 12:30 a.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51712/
> ---
> 
> (Updated Sept. 9, 2016, 12:30 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1764
> https://issues.apache.org/jira/browse/AURORA-1764
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This extends getJobUpdateDetails to return a list of details instead of being 
> scoped to a single update.
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md bbf7198dc7ce53bb02a1bc59f75a661e23472913 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> 4202f0eec59ef16668fca6b6ebb925335ad5dea1 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 9149c3018ae58d405f284fcbd4076d251ccc8192 
>   src/main/resources/scheduler/assets/js/services.js 
> 315fc17894c2261333715fbb9c45fc02505f2942 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  a7d1f74acdfe5f58eb822a4d826e920cad53dced 
>   src/test/python/apache/aurora/client/api/test_api.py 
> 7a0797d5402e931006d4f215e2d9c0dbbd113257 
> 
> Diff: https://reviews.apache.org/r/51712/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>