Re: Review Request 33740: Handle UpdateConfigError when starting a job update.

[Zameer Manji]

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

Ship it!


Ship It!

- Zameer Manji


On April 30, 2015, 5:11 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33740/
> ---
> 
> (Updated April 30, 2015, 5:11 p.m.)
> 
> 
> Review request for Aurora and Zameer Manji.
> 
> 
> Bugs: AURORA-1299
> https://issues.apache.org/jira/browse/AURORA-1299
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Handle UpdateConfigError when starting a job update.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/cli/update.py 
> 9f190a62e9bc14f20dbcf7e49917eeb412d2e68a 
>   src/test/python/apache/aurora/client/cli/test_supdate.py 
> ccbcae64e894c2ae5fd6a97b1e3dadbb54fbb7d6 
> 
> Diff: https://reviews.apache.org/r/33740/diff/
> 
> 
> Testing
> ---
> 
> Test suite, also verified in vagrant:
> ```
> $ aurora update start devcluster/vagrant/test/http_example 
> /vagrant/src/test/sh/org/apache/aurora/e2e/http/http_example.aurora
> Error executing command: Pulse interval seconds must be at least 60 seconds.
> ```
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 33740: Handle UpdateConfigError when starting a job update.

[Aurora ReviewBot]

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

Ship it!


Master (f9b60c6) 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 May 1, 2015, 12:11 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33740/
> ---
> 
> (Updated May 1, 2015, 12:11 a.m.)
> 
> 
> Review request for Aurora and Zameer Manji.
> 
> 
> Bugs: AURORA-1299
> https://issues.apache.org/jira/browse/AURORA-1299
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Handle UpdateConfigError when starting a job update.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/cli/update.py 
> 9f190a62e9bc14f20dbcf7e49917eeb412d2e68a 
>   src/test/python/apache/aurora/client/cli/test_supdate.py 
> ccbcae64e894c2ae5fd6a97b1e3dadbb54fbb7d6 
> 
> Diff: https://reviews.apache.org/r/33740/diff/
> 
> 
> Testing
> ---
> 
> Test suite, also verified in vagrant:
> ```
> $ aurora update start devcluster/vagrant/test/http_example 
> /vagrant/src/test/sh/org/apache/aurora/e2e/http/http_example.aurora
> Error executing command: Pulse interval seconds must be at least 60 seconds.
> ```
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 33739: Enable GC executor to gc STARTING tasks which don't exist on the host

[Aurora ReviewBot]

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

Ship it!


Master (f9b60c6) 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 May 1, 2015, 12:06 a.m., Zeke Harris wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33739/
> ---
> 
> (Updated May 1, 2015, 12:06 a.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Enable GC executor to cause the scheduler to kill tasks that it thinks should 
> be STARTING but which don't exist on the host.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/gc_executor.py 
> a7776b599f5fc028ec2ce7712856e080381e84a6 
>   src/test/python/apache/aurora/executor/test_gc_executor.py 
> 856483d9ef7e89a03bb0780d81510bf60928219a 
> 
> Diff: https://reviews.apache.org/r/33739/diff/
> 
> 
> Testing
> ---
> 
> $ ./pants test src/test/python/apache/aurora/executor:gc_executor
> ...
> SUCCESS
> 
> 
> Thanks,
> 
> Zeke Harris
> 
>

Review Request 33740: Handle UpdateConfigError when starting a job update.

[Bill Farner]

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

Review request for Aurora and Zameer Manji.


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


Repository: aurora


Description
---

Handle UpdateConfigError when starting a job update.


Diffs
-

  src/main/python/apache/aurora/client/cli/update.py 
9f190a62e9bc14f20dbcf7e49917eeb412d2e68a 
  src/test/python/apache/aurora/client/cli/test_supdate.py 
ccbcae64e894c2ae5fd6a97b1e3dadbb54fbb7d6 

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


Testing
---

Test suite, also verified in vagrant:
```
$ aurora update start devcluster/vagrant/test/http_example 
/vagrant/src/test/sh/org/apache/aurora/e2e/http/http_example.aurora
Error executing command: Pulse interval seconds must be at least 60 seconds.
```


Thanks,

Bill Farner

Review Request 33739: Enable GC executor to gc STARTING tasks which don't exist on the host

[Zeke Harris]

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

Review request for Aurora.


Repository: aurora


Description
---

Enable GC executor to cause the scheduler to kill tasks that it thinks should 
be STARTING but which don't exist on the host.


Diffs
-

  src/main/python/apache/aurora/executor/gc_executor.py 
a7776b599f5fc028ec2ce7712856e080381e84a6 
  src/test/python/apache/aurora/executor/test_gc_executor.py 
856483d9ef7e89a03bb0780d81510bf60928219a 

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


Testing
---

$ ./pants test src/test/python/apache/aurora/executor:gc_executor
...
SUCCESS


Thanks,

Zeke Harris

Re: Review Request 33738: Fix test fixture issue causing failing tests to report as passing.

[Aurora ReviewBot]

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

Ship it!


Master (975318f) 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 April 30, 2015, 11:50 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33738/
> ---
> 
> (Updated April 30, 2015, 11:50 p.m.)
> 
> 
> Review request for Aurora and Zameer Manji.
> 
> 
> Bugs: AURORA-1301
> https://issues.apache.org/jira/browse/AURORA-1301
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fix test fixture issue causing failing tests to report as passing.
> 
> 
> Diffs
> -
> 
>   src/test/python/apache/aurora/client/cli/test_diff.py 
> f6ce00e779d2372b2e3aede5f27dde145d052075 
>   src/test/python/apache/aurora/client/cli/test_task.py 
> abf62529c70db3e7ce6d1df0f95aaabab900b5a3 
>   src/test/python/apache/aurora/client/cli/util.py 
> 5903101cb4af24a11c9276f42cc712b8ee6c9da5 
> 
> Diff: https://reviews.apache.org/r/33738/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 33738: Fix test fixture issue causing failing tests to report as passing.

[Zameer Manji]

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

Ship it!



src/test/python/apache/aurora/client/cli/util.py


I can't beleive this happened.


- Zameer Manji


On April 30, 2015, 4:50 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33738/
> ---
> 
> (Updated April 30, 2015, 4:50 p.m.)
> 
> 
> Review request for Aurora and Zameer Manji.
> 
> 
> Bugs: AURORA-1301
> https://issues.apache.org/jira/browse/AURORA-1301
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fix test fixture issue causing failing tests to report as passing.
> 
> 
> Diffs
> -
> 
>   src/test/python/apache/aurora/client/cli/test_diff.py 
> f6ce00e779d2372b2e3aede5f27dde145d052075 
>   src/test/python/apache/aurora/client/cli/test_task.py 
> abf62529c70db3e7ce6d1df0f95aaabab900b5a3 
>   src/test/python/apache/aurora/client/cli/util.py 
> 5903101cb4af24a11c9276f42cc712b8ee6c9da5 
> 
> Diff: https://reviews.apache.org/r/33738/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Review Request 33738: Fix test fixture issue causing failing tests to report as passing.

[Bill Farner]

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

Review request for Aurora and Zameer Manji.


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


Repository: aurora


Description
---

Fix test fixture issue causing failing tests to report as passing.


Diffs
-

  src/test/python/apache/aurora/client/cli/test_diff.py 
f6ce00e779d2372b2e3aede5f27dde145d052075 
  src/test/python/apache/aurora/client/cli/test_task.py 
abf62529c70db3e7ce6d1df0f95aaabab900b5a3 
  src/test/python/apache/aurora/client/cli/util.py 
5903101cb4af24a11c9276f42cc712b8ee6c9da5 

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


Testing
---


Thanks,

Bill Farner

Re: Review Request 33705: Don't retry API requests that fail with auth errors.

[Joshua Cohen]

> On April 30, 2015, 7:15 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/common/transport.py, line 64
> > 
> >
> > Is this meant to indicate that it should be a function that returns a 
> > Session? Is this a standard that's documented somewhere?
> 
> Bill Farner wrote:
> Yes, that's what it indicates.  I made this change to address warnings 
> that PyCharm was showing, since the type hint was incorrect.
> 
> I couldn't find details on whether this is more broad than PyCharm, but 
> docs are here: 
> https://www.jetbrains.com/pycharm/help/type-hinting-in-pycharm.html#d269326e143

Thanks, good to know.


- Joshua


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


On April 30, 2015, 12:12 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33705/
> ---
> 
> (Updated April 30, 2015, 12:12 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Kevin Sweeney.
> 
> 
> Bugs: AURORA-1248
> https://issues.apache.org/jira/browse/AURORA-1248
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The only thing i really don't care for in this patch is that 
> `add_auth_error_handler` alters a 'private' field, but i was unable to come 
> up with an alternative that wouldn't seem to spiral into a refactor.  
> Advice/opinions solicited.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/scheduler_client.py 
> 3f9c691056ee50103a7857bea43160c75388f2aa 
>   src/main/python/apache/aurora/client/cli/__init__.py 
> 10d943215282a83ebe0cadac1592848d797c3e79 
>   src/main/python/apache/aurora/client/cli/context.py 
> adbffc4aa07c242691e87e0fcfc9f5b840999d1d 
>   src/main/python/apache/aurora/common/transport.py 
> eefe8f7b50e0e0040e28ff47b226e91316f1911e 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> 9319728d3a95b158cb734f4cbc6be97187d43def 
>   src/test/python/apache/aurora/client/cli/test_context.py 
> d63f060ea6cf747792f27499da901dd7cecae596 
>   src/test/python/apache/aurora/common/test_transport.py 
> e4f93ef9dd6bf0d54166bdf6afb247fad871ea10 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
> cc7cdee42b7c03dd1f121963129d94e67cb9e2a2 
> 
> Diff: https://reviews.apache.org/r/33705/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 33705: Don't retry API requests that fail with auth errors.

[Bill Farner]

> On April 30, 2015, 7:15 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/common/transport.py, line 64
> > 
> >
> > Is this meant to indicate that it should be a function that returns a 
> > Session? Is this a standard that's documented somewhere?

Yes, that's what it indicates.  I made this change to address warnings that 
PyCharm was showing, since the type hint was incorrect.

I couldn't find details on whether this is more broad than PyCharm, but docs 
are here: 
https://www.jetbrains.com/pycharm/help/type-hinting-in-pycharm.html#d269326e143


- Bill


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


On April 30, 2015, 12:12 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33705/
> ---
> 
> (Updated April 30, 2015, 12:12 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Kevin Sweeney.
> 
> 
> Bugs: AURORA-1248
> https://issues.apache.org/jira/browse/AURORA-1248
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The only thing i really don't care for in this patch is that 
> `add_auth_error_handler` alters a 'private' field, but i was unable to come 
> up with an alternative that wouldn't seem to spiral into a refactor.  
> Advice/opinions solicited.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/scheduler_client.py 
> 3f9c691056ee50103a7857bea43160c75388f2aa 
>   src/main/python/apache/aurora/client/cli/__init__.py 
> 10d943215282a83ebe0cadac1592848d797c3e79 
>   src/main/python/apache/aurora/client/cli/context.py 
> adbffc4aa07c242691e87e0fcfc9f5b840999d1d 
>   src/main/python/apache/aurora/common/transport.py 
> eefe8f7b50e0e0040e28ff47b226e91316f1911e 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> 9319728d3a95b158cb734f4cbc6be97187d43def 
>   src/test/python/apache/aurora/client/cli/test_context.py 
> d63f060ea6cf747792f27499da901dd7cecae596 
>   src/test/python/apache/aurora/common/test_transport.py 
> e4f93ef9dd6bf0d54166bdf6afb247fad871ea10 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
> cc7cdee42b7c03dd1f121963129d94e67cb9e2a2 
> 
> Diff: https://reviews.apache.org/r/33705/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 33728: Fix query for active job updates when serving /updates.

[David McLaughlin]

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

Ship it!


Ship It!

- David McLaughlin


On April 30, 2015, 6:59 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33728/
> ---
> 
> (Updated April 30, 2015, 6:59 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Bugs: AURORA-1231
> https://issues.apache.org/jira/browse/AURORA-1231
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fix query for active job updates when serving /updates.
> 
> 
> Diffs
> -
> 
>   src/main/resources/scheduler/assets/js/services.js 
> 47820743484da2e097651a829386a4abc72744eb 
> 
> Diff: https://reviews.apache.org/r/33728/diff/
> 
> 
> Testing
> ---
> 
> Manually in vagrant :-/
> 
> 
> File Attachments
> 
> 
> /updates
>   
> https://reviews.apache.org/media/uploaded/files/2015/04/30/c966cca3-9fa4-4e15-89fb-5aae44d5302e__Screenshot_from_2015-04-30_113250.png
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 33728: Fix query for active job updates when serving /updates.

[Joshua Cohen]

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

Ship it!


Ship It!

- Joshua Cohen


On April 30, 2015, 6:59 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33728/
> ---
> 
> (Updated April 30, 2015, 6:59 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Bugs: AURORA-1231
> https://issues.apache.org/jira/browse/AURORA-1231
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fix query for active job updates when serving /updates.
> 
> 
> Diffs
> -
> 
>   src/main/resources/scheduler/assets/js/services.js 
> 47820743484da2e097651a829386a4abc72744eb 
> 
> Diff: https://reviews.apache.org/r/33728/diff/
> 
> 
> Testing
> ---
> 
> Manually in vagrant :-/
> 
> 
> File Attachments
> 
> 
> /updates
>   
> https://reviews.apache.org/media/uploaded/files/2015/04/30/c966cca3-9fa4-4e15-89fb-5aae44d5302e__Screenshot_from_2015-04-30_113250.png
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 33705: Don't retry API requests that fail with auth errors.

[Joshua Cohen]

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

Ship it!


I'm not *too* concerned about accessing the `_scheduler_proxy` property 
directly, but if you're really worried you could expose wrapping the proxy via 
a method on the api object (not sure if the use case warrants such a broad 
mechanism though?).


src/main/python/apache/aurora/common/transport.py


Is this meant to indicate that it should be a function that returns a 
Session? Is this a standard that's documented somewhere?


- Joshua Cohen


On April 30, 2015, 12:12 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33705/
> ---
> 
> (Updated April 30, 2015, 12:12 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Kevin Sweeney.
> 
> 
> Bugs: AURORA-1248
> https://issues.apache.org/jira/browse/AURORA-1248
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The only thing i really don't care for in this patch is that 
> `add_auth_error_handler` alters a 'private' field, but i was unable to come 
> up with an alternative that wouldn't seem to spiral into a refactor.  
> Advice/opinions solicited.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/scheduler_client.py 
> 3f9c691056ee50103a7857bea43160c75388f2aa 
>   src/main/python/apache/aurora/client/cli/__init__.py 
> 10d943215282a83ebe0cadac1592848d797c3e79 
>   src/main/python/apache/aurora/client/cli/context.py 
> adbffc4aa07c242691e87e0fcfc9f5b840999d1d 
>   src/main/python/apache/aurora/common/transport.py 
> eefe8f7b50e0e0040e28ff47b226e91316f1911e 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> 9319728d3a95b158cb734f4cbc6be97187d43def 
>   src/test/python/apache/aurora/client/cli/test_context.py 
> d63f060ea6cf747792f27499da901dd7cecae596 
>   src/test/python/apache/aurora/common/test_transport.py 
> e4f93ef9dd6bf0d54166bdf6afb247fad871ea10 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
> cc7cdee42b7c03dd1f121963129d94e67cb9e2a2 
> 
> Diff: https://reviews.apache.org/r/33705/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 33728: Fix query for active job updates when serving /updates.

[Aurora ReviewBot]

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


Master (f77daf7) 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 April 30, 2015, 6:59 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33728/
> ---
> 
> (Updated April 30, 2015, 6:59 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Bugs: AURORA-1231
> https://issues.apache.org/jira/browse/AURORA-1231
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fix query for active job updates when serving /updates.
> 
> 
> Diffs
> -
> 
>   src/main/resources/scheduler/assets/js/services.js 
> 47820743484da2e097651a829386a4abc72744eb 
> 
> Diff: https://reviews.apache.org/r/33728/diff/
> 
> 
> Testing
> ---
> 
> Manually in vagrant :-/
> 
> 
> File Attachments
> 
> 
> /updates
>   
> https://reviews.apache.org/media/uploaded/files/2015/04/30/c966cca3-9fa4-4e15-89fb-5aae44d5302e__Screenshot_from_2015-04-30_113250.png
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 33728: Fix query for active job updates when serving /updates.

[Bill Farner]

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

(Updated April 30, 2015, 6:59 p.m.)


Review request for Aurora and Joshua Cohen.


Changes
---

Fix jshint error.


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


Repository: aurora


Description
---

Fix query for active job updates when serving /updates.


Diffs (updated)
-

  src/main/resources/scheduler/assets/js/services.js 
47820743484da2e097651a829386a4abc72744eb 

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


Testing
---

Manually in vagrant :-/


File Attachments


/updates
  
https://reviews.apache.org/media/uploaded/files/2015/04/30/c966cca3-9fa4-4e15-89fb-5aae44d5302e__Screenshot_from_2015-04-30_113250.png


Thanks,

Bill Farner

Re: Review Request 33728: Fix query for active job updates when serving /updates.

[Aurora ReviewBot]

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


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


:api:checkPython
:api:generateThriftEntitiesJava
:api:classesThriftEntities
:api:compileJava UP-TO-DATE
:api:generateThriftResources
:api:processResources UP-TO-DATE
:api:classes
:api:jar
:compileJavaNote: Writing 
file:/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/classes/main/com/twitter/common/args/apt/cmdline.arg.info.txt.2

:processResources
:classes
:jar
:startScripts
:distTar
:distZip
:assemble
:compileJmhJava
:processJmhResources UP-TO-DATE
:jmhClasses
:checkstyleJmh
:jsHint
'' is defined but never used. 
(/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/resources/scheduler/assets/js/services.js:28:5)
> */

:jsHint FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':jsHint'.
> Process 'command '/home/jenkins/tools/java/jdk1.7.0_25-32/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 46.699 secs


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

- Aurora ReviewBot


On April 30, 2015, 6:33 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33728/
> ---
> 
> (Updated April 30, 2015, 6:33 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Bugs: AURORA-1231
> https://issues.apache.org/jira/browse/AURORA-1231
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Fix query for active job updates when serving /updates.
> 
> 
> Diffs
> -
> 
>   src/main/resources/scheduler/assets/js/services.js 
> 47820743484da2e097651a829386a4abc72744eb 
> 
> Diff: https://reviews.apache.org/r/33728/diff/
> 
> 
> Testing
> ---
> 
> Manually in vagrant :-/
> 
> 
> File Attachments
> 
> 
> /updates
>   
> https://reviews.apache.org/media/uploaded/files/2015/04/30/c966cca3-9fa4-4e15-89fb-5aae44d5302e__Screenshot_from_2015-04-30_113250.png
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Review Request 33728: Fix query for active job updates when serving /updates.

[Bill Farner]

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

Review request for Aurora and Joshua Cohen.


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


Repository: aurora


Description
---

Fix query for active job updates when serving /updates.


Diffs
-

  src/main/resources/scheduler/assets/js/services.js 
47820743484da2e097651a829386a4abc72744eb 

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


Testing
---

Manually in vagrant :-/


File Attachments


/updates
  
https://reviews.apache.org/media/uploaded/files/2015/04/30/c966cca3-9fa4-4e15-89fb-5aae44d5302e__Screenshot_from_2015-04-30_113250.png


Thanks,

Bill Farner

Re: Review Request 33612: Add a task store implementation that uses a relational database.

[Bill Farner]

> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, line 
> > 154
> > 
> >
> > One perf improvement here could be avoiding deleting TaskConfigs as 
> > they are going to be re-inserted right away by the following loop (a very 
> > likely scenario for something like 'killall'). In fact, I don't see how 
> > `saveTasks` could require deleting any configs at all. Perhaps leave a TODO 
> > to carve out a `deleteTasksWithoutConfigs` method?
> 
> Bill Farner wrote:
> This would break behavior alignment with MemTaskStore, which i would 
> really prefer to avoid at this phase (`saveTasks()` can technically be used 
> as an upsert).
> 
> Maxim Khutornenko wrote:
> It's still not clear to me how upsert of a scheduled task can ever 
> require deleting any task configs. It's an additive operation by definition 
> that cannot possiblty render any TaskConfigs irrelevant.

It's effectively an upsert in practice because MemTaskStore just does a 
`Map.putAll`, possibly replacing values.


- Bill


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


On April 28, 2015, 8:11 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33612/
> ---
> 
> (Updated April 28, 2015, 8:11 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-556
> https://issues.apache.org/jira/browse/AURORA-556
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add a task store implementation that uses a relational database.
> 
> The meat of this review is in `DbTaskStore`, `TaskConfigManager`, and the 
> mapper xml files.  Some supporting actors include files under views/, which 
> serve DB business objects.  I suggest reviewers start by skimming 
> `DbTaskStore` and digesting the comments in there.
> 
> There are some known loose ends here, most notably being continued 
> performance enhancements and cleanup of relations in different tables.  I've 
> included several relevant TODOs, ~all of which must be addressed before this 
> becomes the default task store.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 0182ecb3942cfa2d9dd21138779815f4500339be 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 82ad42fd0a626672dca80a5362fc07d804b3e412 
>   src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java 
> ed1171d52655fef643330f34913c256f77300fa2 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> 3d19831ea0eb85032172b096ac87e126701aa218 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 
> 94ce5c3499ced1b63abf19787acc21b2cd4d0c75 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> c8df88b77b9a2017c48bdc0c9a0477927ba0b179 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java 
> 1a6c3f21d4fcb476539f588facecd8ef37332837 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigManager.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/TaskMapper.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/shims/ContainerShim.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/shims/TaskConstraintShim.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/ScheduleStatusTypeHandler.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/typehandlers/TypeHandlers.java
>  4d0c10d60037a3310226a6fd8936b84ae4135e7e 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/AssignedPort.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/ScheduledTaskWrapper.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/views/TaskConfigRow.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/TaskLink.java 
> PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/storage/mem/InMemStoresModule.java 
> 21f7d4db821930d2c5b52648e1996aa1ef12a85c 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
>  f76f9a988669dab598e9d19f849018c6f55ce03e 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/TaskConfigMapper.xml
>  PRE-CREATION 
>   src/main/resource

Re: Review Request 33608: Added a status update throughput benchmark.

[Bill Farner]

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



src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java


I should have asked the first time around - what's the thought process 
behind including this?  Given that this is a benchmark, it seems only to place 
a ceiling on throughput.


- Bill Farner


On April 30, 2015, 12:35 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33608/
> ---
> 
> (Updated April 30, 2015, 12:35 a.m.)
> 
> 
> Review request for Aurora and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-1283
> https://issues.apache.org/jira/browse/AURORA-1283
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> In order to justify doing asynchronous batch acknowledgements and to better 
> understand status update throughput, this introduces a benchmark.
> 
> Note that this assumes that status update processing is not synchronous, so 
> that the benchmark doesn't need to be updated for AURORA-1228.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/StatusUpdateBenchmark.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 
> PRE-CREATION 
>   src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java 
> PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 
> c54619f7cd617b48069363173dcf63b6254b4095 
>   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverModule.java 
> d7d659bb13f085ff06291ef0033572f8bdf29874 
> 
> Diff: https://reviews.apache.org/r/33608/diff/
> 
> 
> Testing
> ---
> 
> Ran the benchmarks against the existing code and some pending code I have for 
> AURORA-1228 to demonstrate the improvement.
> 
> The end to end tests are broken, appears to be unrelated to my change from 
> what I can tell.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 33612: Add a task store implementation that uses a relational database.

[Maxim Khutornenko]

> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java, line 131
> > 
> >
> > This feels like a potential for flakiness. Why 5? Any reason against 
> > setting it to -1 to let instances die with the tests?
> 
> Bill Farner wrote:
> -1 would cause the DBs to stick around until the JVM exits, and we'd 
> accrue one for every test **case**.  I share your concern of flakiness, 
> though.  An alternative would be to push `NonVolatileStorage.stop()` up to 
> `VolatileStorage`, and establish the pattern of invoking that during tear 
> down.  Does that sound better to you?

An explicit teardown seems more predictable and easy to reason. I recognize the 
ovehead it may bring to unit tests though, so I wonder what others think.


> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DbTaskStore.java, line 
> > 154
> > 
> >
> > One perf improvement here could be avoiding deleting TaskConfigs as 
> > they are going to be re-inserted right away by the following loop (a very 
> > likely scenario for something like 'killall'). In fact, I don't see how 
> > `saveTasks` could require deleting any configs at all. Perhaps leave a TODO 
> > to carve out a `deleteTasksWithoutConfigs` method?
> 
> Bill Farner wrote:
> This would break behavior alignment with MemTaskStore, which i would 
> really prefer to avoid at this phase (`saveTasks()` can technically be used 
> as an upsert).

It's still not clear to me how upsert of a scheduled task can ever require 
deleting any task configs. It's an additive operation by definition that cannot 
possiblty render any TaskConfigs irrelevant.


> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml,
> >  line 302
> > 
> >
> > This seems unrelated to this diff.
> 
> Bill Farner wrote:
> Would you prefer i open a new review?  I used this pattern in another 
> mapper in the diff, and wanted to align on style.

Up to you. I was expecting to see some job update relevant changes when I spot 
this file though.


> On April 28, 2015, 11:48 p.m., Maxim Khutornenko wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/db/DbTaskStoreTest.java, 
> > line 27
> > 
> >
> > Can you add tests (or TODOs) for all other TaskStore methods?
> 
> Bill Farner wrote:
> You mean in addition to those provided in `AbstractTaskStoreTest`?

Aha, makes sense, I did not think we had these.


- Maxim


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


On April 28, 2015, 8:11 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33612/
> ---
> 
> (Updated April 28, 2015, 8:11 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Kevin Sweeney, and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-556
> https://issues.apache.org/jira/browse/AURORA-556
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add a task store implementation that uses a relational database.
> 
> The meat of this review is in `DbTaskStore`, `TaskConfigManager`, and the 
> mapper xml files.  Some supporting actors include files under views/, which 
> serve DB business objects.  I suggest reviewers start by skimming 
> `DbTaskStore` and digesting the comments in there.
> 
> There are some known loose ends here, most notably being continued 
> performance enhancements and cleanup of relations in different tables.  I've 
> included several relevant TODOs, ~all of which must be addressed before this 
> becomes the default task store.
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 0182ecb3942cfa2d9dd21138779815f4500339be 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 82ad42fd0a626672dca80a5362fc07d804b3e412 
>   src/jmh/java/org/apache/aurora/benchmark/ThriftApiBenchmarks.java 
> ed1171d52655fef643330f34913c256f77300fa2 
>   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
> 3d19831ea0eb85032172b096ac87e126701aa218 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 
> 94ce5c3499ced1b63abf19787acc21b2cd4d0c75 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbModule.java 
> c8df88b77b9a2017c48bdc0c9a0477927ba0b179 
>   src/main/java/org/apache