Re: Review Request 32171: Change "update list" subcommand to accept a hierarchy.

[Maxim Khutornenko]

> On March 18, 2015, 12:04 a.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/cli/test_supdate.py, lines 404-406
> > 
> >
> > How would you feel about including all of the currently available data 
> > with a header row?
> > 
> > Also instead of raw epoch time how about ISO 8601 - using the spaceless 
> > 'T' form (`2015-03-15T13:27:36+00:00`)

> How would you feel about including all of the currently available data with a 
> header row?

-1 on shoving all available data into a single header row. I'd rather see a 
clean output here and rely on `aurora update info` for drill downs.


- Maxim


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


On March 17, 2015, 11:54 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32171/
> ---
> 
> (Updated March 17, 2015, 11:54 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Zameer Manji.
> 
> 
> Bugs: AURORA-1168
> https://issues.apache.org/jira/browse/AURORA-1168
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> NOTE: I also am proposing with this change that the output format of `aurora 
> update list` be line-oriented and more concise (see test changes for 
> examples).  The idea is to follow up ~immediately with AURORA-1206.  The 
> proposal is for this command to let you retrieve update identifiers, and 
> `update status` (whose name may change) will allow you to drill into updates.
> 
> I'm open to input on this.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/cli/update.py 
> f025d46d50592156e2455313890e981722ab63a5 
>   src/test/python/apache/aurora/client/cli/test_supdate.py 
> cb66439a778349fc5add4985a7395655c9e1328a 
> 
> Diff: https://reviews.apache.org/r/32171/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 32171: Change "update list" subcommand to accept a hierarchy.

[Aurora ReviewBot]

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


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

 src.test.python.apache.aurora.client.cli.plugins   
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.quota 
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.sla   
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.supdate   
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.task  
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.update
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.version   
 .   SUCCESS
 src.test.python.apache.aurora.client.config
 .   SUCCESS
 src.test.python.apache.aurora.client.factory   
 .   SUCCESS
 src.test.python.apache.aurora.client.hooks.hooked_api  
 .   SUCCESS
 src.test.python.apache.aurora.client.hooks.non_hooked_api  
 .   SUCCESS
 src.test.python.apache.aurora.common.test_aurora_job_key   
 .   SUCCESS
 src.test.python.apache.aurora.common.test_cluster  
 .   SUCCESS
 src.test.python.apache.aurora.common.test_cluster_option   
 .   SUCCESS
 src.test.python.apache.aurora.common.test_clusters 
 .   SUCCESS
 src.test.python.apache.aurora.common.test_http_signaler
 .   SUCCESS
 src.test.python.apache.aurora.common.test_pex_version  
 .   SUCCESS
 src.test.python.apache.aurora.common.test_shellify 
 .   SUCCESS
 src.test.python.apache.aurora.common.test_transport
 .   SUCCESS
 src.test.python.apache.aurora.config.test_base 
 .   SUCCESS
 
src.test.python.apache.aurora.config.test_constraint_parsing
.   SUCCESS
 src.test.python.apache.aurora.config.test_loader   
 .   SUCCESS
 src.test.python.apache.aurora.config.test_thrift   
 .   SUCCESS
 
src.test.python.apache.aurora.executor.common.path_detector 
.   SUCCESS
 src.test.python.apache.aurora.executor.common.task_info
 .   SUCCESS
 src.test.python.apache.aurora.executor.executor_base   
 .   SUCCESS
 src.test.python.apache.aurora.executor.executor_vars   
 .   SUCCESS
 src.test.python.apache.aurora.executor.status_manager  
 .   SUCCESS
 src.test.python.apache.aurora.executor.thermos_task_runner 
 .   FAILURE
 src.test.python.apache.thermos.cli.commands.commands   
 .   SUCCESS
 src.test.python.apache.thermos.cli.common  
 .   SUCCESS
 src.test.python.apache.thermos.cli.main
 .   SUCCESS
 src.test.python.apache.thermos.common.test_pathspec
 .   SUCCESS
 
src.test.python.apache.thermos.core.test_runner_integration 
.   SUCCESS
 src.test.python.apache.thermos.monitoring.test_disk
 .   SUCCESS
 
FAILURE


   FAILURE


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

- Aurora ReviewBot


On March 17, 2015, 11:54 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32171/
> ---
> 
> (Updated March 17, 2015, 11:54 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Zameer Man

Re: Review Request 32171: Change "update list" subcommand to accept a hierarchy.

[Kevin Sweeney]

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



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


How would you feel about including all of the currently available data with 
a header row?

Also instead of raw epoch time how about ISO 8601 - using the spaceless 'T' 
form (`2015-03-15T13:27:36+00:00`)


- Kevin Sweeney


On March 17, 2015, 4:54 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32171/
> ---
> 
> (Updated March 17, 2015, 4:54 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Zameer Manji.
> 
> 
> Bugs: AURORA-1168
> https://issues.apache.org/jira/browse/AURORA-1168
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> NOTE: I also am proposing with this change that the output format of `aurora 
> update list` be line-oriented and more concise (see test changes for 
> examples).  The idea is to follow up ~immediately with AURORA-1206.  The 
> proposal is for this command to let you retrieve update identifiers, and 
> `update status` (whose name may change) will allow you to drill into updates.
> 
> I'm open to input on this.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/cli/update.py 
> f025d46d50592156e2455313890e981722ab63a5 
>   src/test/python/apache/aurora/client/cli/test_supdate.py 
> cb66439a778349fc5add4985a7395655c9e1328a 
> 
> Diff: https://reviews.apache.org/r/32171/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 32171: Change "update list" subcommand to accept a hierarchy.

[David McLaughlin]

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



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


This is more of a general concern, but it would be nice to avoid adding new 
tests in this style - and instead test the Command instance directly. I can't 
find the actual back and forth that led me to testing like this, but it was 
introduced here: https://reviews.apache.org/r/27848/diff/#


- David McLaughlin


On March 17, 2015, 11:54 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32171/
> ---
> 
> (Updated March 17, 2015, 11:54 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Zameer Manji.
> 
> 
> Bugs: AURORA-1168
> https://issues.apache.org/jira/browse/AURORA-1168
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> NOTE: I also am proposing with this change that the output format of `aurora 
> update list` be line-oriented and more concise (see test changes for 
> examples).  The idea is to follow up ~immediately with AURORA-1206.  The 
> proposal is for this command to let you retrieve update identifiers, and 
> `update status` (whose name may change) will allow you to drill into updates.
> 
> I'm open to input on this.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/cli/update.py 
> f025d46d50592156e2455313890e981722ab63a5 
>   src/test/python/apache/aurora/client/cli/test_supdate.py 
> cb66439a778349fc5add4985a7395655c9e1328a 
> 
> Diff: https://reviews.apache.org/r/32171/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 32171: Change "update list" subcommand to accept a hierarchy.

[Bill Farner]

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

(Updated March 17, 2015, 11:54 p.m.)


Review request for Aurora, David McLaughlin and Zameer Manji.


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


Repository: aurora


Description
---

NOTE: I also am proposing with this change that the output format of `aurora 
update list` be line-oriented and more concise (see test changes for examples). 
 The idea is to follow up ~immediately with AURORA-1206.  The proposal is for 
this command to let you retrieve update identifiers, and `update status` (whose 
name may change) will allow you to drill into updates.

I'm open to input on this.


Diffs (updated)
-

  src/main/python/apache/aurora/client/cli/update.py 
f025d46d50592156e2455313890e981722ab63a5 
  src/test/python/apache/aurora/client/cli/test_supdate.py 
cb66439a778349fc5add4985a7395655c9e1328a 

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


Testing
---


Thanks,

Bill Farner

Re: Review Request 32181: Only warn about insufficient GC resources when actually needed

[Bill Farner]

> On March 17, 2015, 11:12 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/async/GcExecutorLauncher.java, 
> > line 208
> > 
> >
> > This reordering will result in pulsing the hostname even when an offer 
> > is insufficient for GC thus leading to missed collections. I'd much rather 
> > suppress the warning (e.g. change it to Level.FINE) than risk a missed 
> > collection.
> 
> Stephan Erb wrote:
> What about extracting the pulsing from `isTimeToCollect` and performing 
> it directly in `willUse`?
> 
> Maxim Khutornenko wrote:
> Given this feature is going away soon, I'd rather not invest any time in 
> improving it unless absolutely necessary. Are there any other concerns 
> besides warnings in scheduler log? If not, perhaps lowering the logging level 
> is the way to go.

I'd be okay with LOG.fine FWIW.  General +1 to removing the noise.


- Bill


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


On March 17, 2015, 10:50 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32181/
> ---
> 
> (Updated March 17, 2015, 10:50 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Only warn about insufficient GC resources when actually needed
> 
> Whenever a garbage collection run finishes, the launcher is offered the same 
> resources again. Probably due to rounding errors, these are not considered 
> sufficient for another run. By changing the order of time check and resource 
> check we prevent unnecessary warnings about these small offers.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/async/GcExecutorLauncher.java 
> 31aa2bbaab3d97875493ad75c4d2c7c82ac7fa58 
>   src/test/java/org/apache/aurora/scheduler/async/GcExecutorLauncherTest.java 
> 422d5a9a42310979752eb7282658316c2b772419 
> 
> Diff: https://reviews.apache.org/r/32181/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 32181: Only warn about insufficient GC resources when actually needed

[Maxim Khutornenko]

> On March 17, 2015, 11:12 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/async/GcExecutorLauncher.java, 
> > line 208
> > 
> >
> > This reordering will result in pulsing the hostname even when an offer 
> > is insufficient for GC thus leading to missed collections. I'd much rather 
> > suppress the warning (e.g. change it to Level.FINE) than risk a missed 
> > collection.
> 
> Stephan Erb wrote:
> What about extracting the pulsing from `isTimeToCollect` and performing 
> it directly in `willUse`?

Given this feature is going away soon, I'd rather not invest any time in 
improving it unless absolutely necessary. Are there any other concerns besides 
warnings in scheduler log? If not, perhaps lowering the logging level is the 
way to go.


- Maxim


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


On March 17, 2015, 10:50 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32181/
> ---
> 
> (Updated March 17, 2015, 10:50 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Only warn about insufficient GC resources when actually needed
> 
> Whenever a garbage collection run finishes, the launcher is offered the same 
> resources again. Probably due to rounding errors, these are not considered 
> sufficient for another run. By changing the order of time check and resource 
> check we prevent unnecessary warnings about these small offers.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/async/GcExecutorLauncher.java 
> 31aa2bbaab3d97875493ad75c4d2c7c82ac7fa58 
>   src/test/java/org/apache/aurora/scheduler/async/GcExecutorLauncherTest.java 
> 422d5a9a42310979752eb7282658316c2b772419 
> 
> Diff: https://reviews.apache.org/r/32181/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 32171: Change "update list" subcommand to accept a hierarchy.

[Bill Farner]

> On March 17, 2015, 11:36 p.m., Aurora ReviewBot wrote:
> > Master (a419863) is red with this patch.
> >   ./build-support/jenkins/build.sh
> > 
> > Installing 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter.common.collections-0.3.0-py2.7-nspkg.pth
> >   Running setup.py install for twitter.common.util
> > Skipping installation of 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/__init__.py
> >  (namespace package)
> > Skipping installation of 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/common/__init__.py
> >  (namespace package)
> > Installing 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter.common.util-0.3.0-py2.7-nspkg.pth
> >   Running setup.py install for twitter.common.log
> > Skipping installation of 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/__init__.py
> >  (namespace package)
> > Skipping installation of 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/common/__init__.py
> >  (namespace package)
> > Installing 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter.common.log-0.3.0-py2.7-nspkg.pth
> >   Running setup.py install for twitter.common.process
> > Skipping installation of 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/__init__.py
> >  (namespace package)
> > Skipping installation of 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/common/__init__.py
> >  (namespace package)
> > Installing 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter.common.process-0.3.0-py2.7-nspkg.pth
> >   Running setup.py install for gitdb
> > building 'gitdb._perf' extension
> > x86_64-linux-gnu-gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv 
> > -O2 -Wall -Wstrict-prototypes -fPIC -Igitdb -I/usr/include/python2.7 -c 
> > gitdb/_fun.c -o build/temp.linux-x86_64-2.7/gitdb/_fun.o
> > x86_64-linux-gnu-gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv 
> > -O2 -Wall -Wstrict-prototypes -fPIC -Igitdb -I/usr/include/python2.7 -c 
> > gitdb/_delta_apply.c -o build/temp.linux-x86_64-2.7/gitdb/_delta_apply.o
> > x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions 
> > -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g 
> > -fwrapv -O2 -Wall -Wstrict-prototypes -D_FORTIFY_SOURCE=2 -g 
> > -fstack-protector --param=ssp-buffer-size=4 -Wformat 
> > -Werror=format-security build/temp.linux-x86_64-2.7/gitdb/_fun.o 
> > build/temp.linux-x86_64-2.7/gitdb/_delta_apply.o -o 
> > build/lib.linux-x86_64-2.7/gitdb/_perf.so
> >   Running setup.py install for twitter.common.app
> > Skipping installation of 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/__init__.py
> >  (namespace package)
> > Skipping installation of 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/common/__init__.py
> >  (namespace package)
> > Installing 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter.common.app-0.3.0-py2.7-nspkg.pth
> >   Running setup.py install for GitPython
> > 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/local/lib/python2.7/site-packages/setuptools/dist.py:292:
> >  UserWarning: The version specified ('0.3.2 RC1') is an invalid version, 
> > this may not work as expected with newer versions of setuptools, pip, and 
> > PyPI. Please see PEP 440 for more details.
> >   "details." % self.metadata.version
> >   Running setup.py install for pep8
> > Installing pep8 script to 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/bin
> >   Running setup.py install for pyflakes
> > Installing pyflakes script to 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/bin
> >   Running setup.py install for twitter.checkstyle
> > Skipping installation of 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/__init__.py
> >  (namespace package)
> > Installing 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkst

Re: Review Request 32181: Only warn about insufficient GC resources when actually needed

[Stephan Erb]

> On March 18, 2015, 12:12 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/async/GcExecutorLauncher.java, 
> > line 208
> > 
> >
> > This reordering will result in pulsing the hostname even when an offer 
> > is insufficient for GC thus leading to missed collections. I'd much rather 
> > suppress the warning (e.g. change it to Level.FINE) than risk a missed 
> > collection.

What about extracting the pulsing from `isTimeToCollect` and performing it 
directly in `willUse`?


- Stephan


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


On March 17, 2015, 11:50 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32181/
> ---
> 
> (Updated March 17, 2015, 11:50 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Only warn about insufficient GC resources when actually needed
> 
> Whenever a garbage collection run finishes, the launcher is offered the same 
> resources again. Probably due to rounding errors, these are not considered 
> sufficient for another run. By changing the order of time check and resource 
> check we prevent unnecessary warnings about these small offers.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/async/GcExecutorLauncher.java 
> 31aa2bbaab3d97875493ad75c4d2c7c82ac7fa58 
>   src/test/java/org/apache/aurora/scheduler/async/GcExecutorLauncherTest.java 
> 422d5a9a42310979752eb7282658316c2b772419 
> 
> Diff: https://reviews.apache.org/r/32181/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 32171: Change "update list" subcommand to accept a hierarchy.

[Aurora ReviewBot]

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


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

Installing 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter.common.collections-0.3.0-py2.7-nspkg.pth
  Running setup.py install for twitter.common.util
Skipping installation of 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/__init__.py
 (namespace package)
Skipping installation of 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/common/__init__.py
 (namespace package)
Installing 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter.common.util-0.3.0-py2.7-nspkg.pth
  Running setup.py install for twitter.common.log
Skipping installation of 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/__init__.py
 (namespace package)
Skipping installation of 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/common/__init__.py
 (namespace package)
Installing 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter.common.log-0.3.0-py2.7-nspkg.pth
  Running setup.py install for twitter.common.process
Skipping installation of 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/__init__.py
 (namespace package)
Skipping installation of 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/common/__init__.py
 (namespace package)
Installing 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter.common.process-0.3.0-py2.7-nspkg.pth
  Running setup.py install for gitdb
building 'gitdb._perf' extension
x86_64-linux-gnu-gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 
-Wall -Wstrict-prototypes -fPIC -Igitdb -I/usr/include/python2.7 -c 
gitdb/_fun.c -o build/temp.linux-x86_64-2.7/gitdb/_fun.o
x86_64-linux-gnu-gcc -pthread -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 
-Wall -Wstrict-prototypes -fPIC -Igitdb -I/usr/include/python2.7 -c 
gitdb/_delta_apply.c -o build/temp.linux-x86_64-2.7/gitdb/_delta_apply.o
x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions 
-Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv 
-O2 -Wall -Wstrict-prototypes -D_FORTIFY_SOURCE=2 -g -fstack-protector 
--param=ssp-buffer-size=4 -Wformat -Werror=format-security 
build/temp.linux-x86_64-2.7/gitdb/_fun.o 
build/temp.linux-x86_64-2.7/gitdb/_delta_apply.o -o 
build/lib.linux-x86_64-2.7/gitdb/_perf.so
  Running setup.py install for twitter.common.app
Skipping installation of 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/__init__.py
 (namespace package)
Skipping installation of 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/common/__init__.py
 (namespace package)
Installing 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter.common.app-0.3.0-py2.7-nspkg.pth
  Running setup.py install for GitPython

/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/local/lib/python2.7/site-packages/setuptools/dist.py:292:
 UserWarning: The version specified ('0.3.2 RC1') is an invalid version, this 
may not work as expected with newer versions of setuptools, pip, and PyPI. 
Please see PEP 440 for more details.
  "details." % self.metadata.version
  Running setup.py install for pep8
Installing pep8 script to 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/bin
  Running setup.py install for pyflakes
Installing pyflakes script to 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/bin
  Running setup.py install for twitter.checkstyle
Skipping installation of 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter/__init__.py
 (namespace package)
Installing 
/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/python/checkstyle.venv/lib/python2.7/site-packages/twitter.checkstyle-0.1.0-py2.7-nspkg.pth
Installing twitterstyle script to 
/home/jenki

Re: Review Request 32171: Change "update list" subcommand to accept a hierarchy.

[Bill Farner]

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

(Updated March 17, 2015, 11:30 p.m.)


Review request for Aurora, David McLaughlin and Zameer Manji.


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


Repository: aurora


Description (updated)
---

NOTE: I also am proposing with this change that the output format of `aurora 
update list` be line-oriented and more concise (see test changes for examples). 
 The idea is to follow up ~immediately with AURORA-1206.  The proposal is for 
this command to let you retrieve update identifiers, and `update status` (whose 
name may change) will allow you to drill into updates.

I'm open to input on this.


Diffs (updated)
-

  src/main/python/apache/aurora/client/cli/update.py 
f025d46d50592156e2455313890e981722ab63a5 
  src/test/python/apache/aurora/client/cli/test_supdate.py 
cb66439a778349fc5add4985a7395655c9e1328a 

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


Testing
---


Thanks,

Bill Farner

Re: Review Request 32171: Change "update list" subcommand to accept a hierarchy.

[Zameer Manji]

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

Ship it!


Ship It!

- Zameer Manji


On March 17, 2015, 1:50 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32171/
> ---
> 
> (Updated March 17, 2015, 1:50 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Zameer Manji.
> 
> 
> Bugs: AURORA-1168
> https://issues.apache.org/jira/browse/AURORA-1168
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Change "update list" subcommand to accept a hierarchy.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/cli/update.py 
> f025d46d50592156e2455313890e981722ab63a5 
>   src/test/python/apache/aurora/client/cli/test_supdate.py 
> cb66439a778349fc5add4985a7395655c9e1328a 
> 
> Diff: https://reviews.apache.org/r/32171/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 32171: Change "update list" subcommand to accept a hierarchy.

[Bill Farner]

> On March 17, 2015, 10:59 p.m., Maxim Khutornenko wrote:
> > src/main/python/apache/aurora/client/cli/update.py, line 267
> > 
> >
> > How about adding all statuses here to allow searching by a specific job 
> > update status in addition to a group?

I considered that, but it makes for some pretty heinous help output.  I don't 
feel too strongly either way, but i'm currently of the mindset that we can add 
it later without breaking compatibility.


> On March 17, 2015, 10:59 p.m., Maxim Khutornenko wrote:
> > src/main/python/apache/aurora/client/cli/update.py, line 269
> > 
> >
> > typo

Fixed.


- Bill


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


On March 17, 2015, 8:50 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32171/
> ---
> 
> (Updated March 17, 2015, 8:50 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Zameer Manji.
> 
> 
> Bugs: AURORA-1168
> https://issues.apache.org/jira/browse/AURORA-1168
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Change "update list" subcommand to accept a hierarchy.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/cli/update.py 
> f025d46d50592156e2455313890e981722ab63a5 
>   src/test/python/apache/aurora/client/cli/test_supdate.py 
> cb66439a778349fc5add4985a7395655c9e1328a 
> 
> Diff: https://reviews.apache.org/r/32171/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 32171: Change "update list" subcommand to accept a hierarchy.

[Bill Farner]

> On March 17, 2015, 10:46 p.m., Zameer Manji wrote:
> > src/main/python/apache/aurora/client/cli/update.py, line 226
> > 
> >
> > This is a duplicate of the 'PartialJobKey' class in client/cli/jobs.py. 
> > Please consider reusing that class to prevent duplication.
> > 
> > Ideally you do this so your 'update_filter' function can be reused for 
> > parsing these CLI values.

I initially looked into sharing code, but the use cases are different.  For 
example, `PartialJobkey` and relevant code supports wildcards, which i do not 
wish to support here.  Given that the behavior was different, i did not feel 
that sharing the one-liner for the namedtuple was constructive.


[1] 
https://github.com/apache/incubator-aurora/blob/2f991069e60e47ddbbaf1d60d6403922bbb0e208/src/main/python/apache/aurora/client/cli/jobs.py#L68-L87


> On March 17, 2015, 10:46 p.m., Zameer Manji wrote:
> > src/main/python/apache/aurora/client/cli/update.py, line 281
> > 
> >
> > No need for .dedent here.

Removed.


- Bill


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


On March 17, 2015, 8:50 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32171/
> ---
> 
> (Updated March 17, 2015, 8:50 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Zameer Manji.
> 
> 
> Bugs: AURORA-1168
> https://issues.apache.org/jira/browse/AURORA-1168
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Change "update list" subcommand to accept a hierarchy.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/cli/update.py 
> f025d46d50592156e2455313890e981722ab63a5 
>   src/test/python/apache/aurora/client/cli/test_supdate.py 
> cb66439a778349fc5add4985a7395655c9e1328a 
> 
> Diff: https://reviews.apache.org/r/32171/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 32181: Only warn about insufficient GC resources when actually needed

[Maxim Khutornenko]

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



src/main/java/org/apache/aurora/scheduler/async/GcExecutorLauncher.java


This reordering will result in pulsing the hostname even when an offer is 
insufficient for GC thus leading to missed collections. I'd much rather 
suppress the warning (e.g. change it to Level.FINE) than risk a missed 
collection.


- Maxim Khutornenko


On March 17, 2015, 10:50 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32181/
> ---
> 
> (Updated March 17, 2015, 10:50 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Only warn about insufficient GC resources when actually needed
> 
> Whenever a garbage collection run finishes, the launcher is offered the same 
> resources again. Probably due to rounding errors, these are not considered 
> sufficient for another run. By changing the order of time check and resource 
> check we prevent unnecessary warnings about these small offers.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/async/GcExecutorLauncher.java 
> 31aa2bbaab3d97875493ad75c4d2c7c82ac7fa58 
>   src/test/java/org/apache/aurora/scheduler/async/GcExecutorLauncherTest.java 
> 422d5a9a42310979752eb7282658316c2b772419 
> 
> Diff: https://reviews.apache.org/r/32181/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 32181: Only warn about insufficient GC resources when actually needed

[Aurora ReviewBot]

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

Ship it!


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

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

- Aurora ReviewBot


On March 17, 2015, 10:50 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32181/
> ---
> 
> (Updated March 17, 2015, 10:50 p.m.)
> 
> 
> Review request for Aurora.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Only warn about insufficient GC resources when actually needed
> 
> Whenever a garbage collection run finishes, the launcher is offered the same 
> resources again. Probably due to rounding errors, these are not considered 
> sufficient for another run. By changing the order of time check and resource 
> check we prevent unnecessary warnings about these small offers.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/async/GcExecutorLauncher.java 
> 31aa2bbaab3d97875493ad75c4d2c7c82ac7fa58 
>   src/test/java/org/apache/aurora/scheduler/async/GcExecutorLauncherTest.java 
> 422d5a9a42310979752eb7282658316c2b772419 
> 
> Diff: https://reviews.apache.org/r/32181/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 32171: Change "update list" subcommand to accept a hierarchy.

[Maxim Khutornenko]

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



src/main/python/apache/aurora/client/cli/update.py


How about adding all statuses here to allow searching by a specific job 
update status in addition to a group?



src/main/python/apache/aurora/client/cli/update.py


typo


- Maxim Khutornenko


On March 17, 2015, 8:50 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32171/
> ---
> 
> (Updated March 17, 2015, 8:50 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Zameer Manji.
> 
> 
> Bugs: AURORA-1168
> https://issues.apache.org/jira/browse/AURORA-1168
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Change "update list" subcommand to accept a hierarchy.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/cli/update.py 
> f025d46d50592156e2455313890e981722ab63a5 
>   src/test/python/apache/aurora/client/cli/test_supdate.py 
> cb66439a778349fc5add4985a7395655c9e1328a 
> 
> Diff: https://reviews.apache.org/r/32171/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 32181: Only warn about insufficient GC resources when actually needed

[Stephan Erb]

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

(Updated March 17, 2015, 11:50 p.m.)


Review request for Aurora.


Summary (updated)
-

Only warn about insufficient GC resources when actually needed


Repository: aurora


Description
---

Only warn about insufficient GC resources when actually needed

Whenever a garbage collection run finishes, the launcher is offered the same 
resources again. Probably due to rounding errors, these are not considered 
sufficient for another run. By changing the order of time check and resource 
check we prevent unnecessary warnings about these small offers.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/async/GcExecutorLauncher.java 
31aa2bbaab3d97875493ad75c4d2c7c82ac7fa58 
  src/test/java/org/apache/aurora/scheduler/async/GcExecutorLauncherTest.java 
422d5a9a42310979752eb7282658316c2b772419 

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


Testing
---

./gradlew -Pq build


Thanks,

Stephan Erb

Review Request 32181: Fix clusters.patch contextmanager cleanup

[Stephan Erb]

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

Review request for Aurora.


Repository: aurora


Description
---

Only warn about insufficient GC resources when actually needed

Whenever a garbage collection run finishes, the launcher is offered the same 
resources again. Probably due to rounding errors, these are not considered 
sufficient for another run. By changing the order of time check and resource 
check we prevent unnecessary warnings about these small offers.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/async/GcExecutorLauncher.java 
31aa2bbaab3d97875493ad75c4d2c7c82ac7fa58 
  src/test/java/org/apache/aurora/scheduler/async/GcExecutorLauncherTest.java 
422d5a9a42310979752eb7282658316c2b772419 

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


Testing
---

./gradlew -Pq build


Thanks,

Stephan Erb

Re: Review Request 32171: Change "update list" subcommand to accept a hierarchy.

[Zameer Manji]

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



src/main/python/apache/aurora/client/cli/update.py


This is a duplicate of the 'PartialJobKey' class in client/cli/jobs.py. 
Please consider reusing that class to prevent duplication.

Ideally you do this so your 'update_filter' function can be reused for 
parsing these CLI values.



src/main/python/apache/aurora/client/cli/update.py


No need for .dedent here.


- Zameer Manji


On March 17, 2015, 1:50 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32171/
> ---
> 
> (Updated March 17, 2015, 1:50 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Zameer Manji.
> 
> 
> Bugs: AURORA-1168
> https://issues.apache.org/jira/browse/AURORA-1168
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Change "update list" subcommand to accept a hierarchy.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/cli/update.py 
> f025d46d50592156e2455313890e981722ab63a5 
>   src/test/python/apache/aurora/client/cli/test_supdate.py 
> cb66439a778349fc5add4985a7395655c9e1328a 
> 
> Diff: https://reviews.apache.org/r/32171/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 32175: Add a test to ensure annotations exist for AuroraSchedulerManager

[Joshua Cohen]

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

Ship it!


Ship It!

- Joshua Cohen


On March 17, 2015, 9:59 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32175/
> ---
> 
> (Updated March 17, 2015, 9:59 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1187
> https://issues.apache.org/jira/browse/AURORA-1187
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This addresses review feedback from https://reviews.apache.org/r/32141/ and 
> prevents divergence when api.thrift changes.
> 
> 
> Diffs
> -
> 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdminTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/32175/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 32175: Add a test to ensure annotations exist for AuroraSchedulerManager

[Aurora ReviewBot]

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

Ship it!


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

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

- Aurora ReviewBot


On March 17, 2015, 9:59 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32175/
> ---
> 
> (Updated March 17, 2015, 9:59 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1187
> https://issues.apache.org/jira/browse/AURORA-1187
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This addresses review feedback from https://reviews.apache.org/r/32141/ and 
> prevents divergence when api.thrift changes.
> 
> 
> Diffs
> -
> 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdminTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/32175/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 32175: Add a test to ensure annotations exist for AuroraSchedulerManager

[Kevin Sweeney]

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

(Updated March 17, 2015, 2:59 p.m.)


Review request for Aurora, Joshua Cohen and Bill Farner.


Changes
---

Appease the bot.


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


Repository: aurora


Description
---

This addresses review feedback from https://reviews.apache.org/r/32141/ and 
prevents divergence when api.thrift changes.


Diffs (updated)
-

  
src/test/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdminTest.java
 PRE-CREATION 

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


Testing
---

./gradlew build


Thanks,

Kevin Sweeney

Re: Review Request 32175: Add a test to ensure annotations exist for AuroraSchedulerManager

[Aurora ReviewBot]

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


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

: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
:checkstyleMain
:compileTestJava
:processTestResources
:testClasses
:checkstyleTest
:findbugsJmh
:findbugsMain
:findbugsTest FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':findbugsTest'.
> FindBugs rule violations were found. See the report at: 
> file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/findbugs/test.html

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

BUILD FAILED

Total time: 2 mins 54.393 secs


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

- Aurora ReviewBot


On March 17, 2015, 9:42 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32175/
> ---
> 
> (Updated March 17, 2015, 9:42 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1187
> https://issues.apache.org/jira/browse/AURORA-1187
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This addresses review feedback from https://reviews.apache.org/r/32141/ and 
> prevents divergence when api.thrift changes.
> 
> 
> Diffs
> -
> 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdminTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/32175/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Review Request 32175: Add a test to ensure annotations exist for AuroraSchedulerManager

[Kevin Sweeney]

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

Review request for Aurora, Joshua Cohen and Bill Farner.


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


Repository: aurora


Description
---

This addresses review feedback from https://reviews.apache.org/r/32141/ and 
prevents divergence when api.thrift changes.


Diffs
-

  
src/test/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdminTest.java
 PRE-CREATION 

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


Testing
---

./gradlew build


Thanks,

Kevin Sweeney

Re: Review Request 32013: Cache TaskDetectors for reuse.

[Zameer Manji]

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

Ship it!


Ship It!

- Zameer Manji


On March 17, 2015, 1:24 p.m., Brian Wickman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32013/
> ---
> 
> (Updated March 17, 2015, 1:24 p.m.)
> 
> 
> Review request for Aurora, Joe Smith and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> SSIA
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/path_detector.py 
> e4135cca09b43e204c52774b747f74212d0a5732 
>   src/main/python/apache/thermos/observer/detector.py 
> 5347cded7061b085b135d62fdb3a5c4a75f81e5c 
>   src/test/python/apache/aurora/executor/common/test_path_detector.py 
> 2d6edae420c1c5b51fe93f97e04c24976afacd51 
> 
> Diff: https://reviews.apache.org/r/32013/diff/
> 
> 
> Testing
> ---
> 
> ./pants test src/test/python/apache/thermos/observer:: + manual observation
> 
> 
> Thanks,
> 
> Brian Wickman
> 
>

Re: Review Request 32171: Change "update list" subcommand to accept a hierarchy.

[Aurora ReviewBot]

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


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

 src.test.python.apache.aurora.client.api.updater_util  
 .   SUCCESS
 src.test.python.apache.aurora.client.base  
 .   SUCCESS
 src.test.python.apache.aurora.client.binding_helper
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.api   
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.client
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.command_hooks 
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.config
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.context   
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.cron  
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.inspect   
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.job   
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.plugins   
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.quota 
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.sla   
 .   SUCCESS
 src.test.python.apache.aurora.client.cli.supdate   
 .   FAILURE
 src.test.python.apache.aurora.client.cli.task  
 .   SUCCESS
 src.test.python.apache.aurora.client.config
 .   SUCCESS
 src.test.python.apache.aurora.client.factory   
 .   SUCCESS
 src.test.python.apache.aurora.client.hooks.non_hooked_api  
 .   SUCCESS
 src.test.python.apache.aurora.common.test_aurora_job_key   
 .   SUCCESS
 src.test.python.apache.aurora.common.test_cluster  
 .   SUCCESS
 src.test.python.apache.aurora.common.test_cluster_option   
 .   SUCCESS
 src.test.python.apache.aurora.common.test_clusters 
 .   SUCCESS
 src.test.python.apache.aurora.common.test_http_signaler
 .   SUCCESS
 src.test.python.apache.aurora.common.test_pex_version  
 .   SUCCESS
 src.test.python.apache.aurora.common.test_shellify 
 .   SUCCESS
 src.test.python.apache.aurora.common.test_transport
 .   SUCCESS
 
src.test.python.apache.aurora.executor.common.path_detector 
.   SUCCESS
 src.test.python.apache.aurora.executor.common.task_info
 .   SUCCESS
 src.test.python.apache.thermos.cli.commands.commands   
 .   SUCCESS
 src.test.python.apache.thermos.cli.common  
 .   SUCCESS
 src.test.python.apache.thermos.cli.main
 .   SUCCESS
 src.test.python.apache.thermos.common.test_pathspec
 .   SUCCESS
 
src.test.python.apache.thermos.core.test_runner_integration 
.   SUCCESS
 src.test.python.apache.thermos.monitoring.test_disk
 .   SUCCESS
 
FAILURE


   FAILURE


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

- Aurora ReviewBot


On March 17, 2015, 8:50 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32171/
> ---
> 
> (Updated March 17, 2015, 8:50 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Zameer Manji.

Re: Review Request 32013: Cache TaskDetectors for reuse.

[Brian Wickman]

> On March 17, 2015, 7:49 p.m., Zameer Manji wrote:
> > src/main/python/apache/aurora/executor/common/path_detector.py, line 34
> > 
> >
> > What's the rationale behind this change? I think the callers should be 
> > doing the checking for existance and dealing with missing directories as 
> > needed.

The rationale is that some tasks will have checkpointing in the sandbox 
(0.9.0+) and some will have it outside the sandbox (<0.8.0, or 0.9.0+ but whose 
thermos runners haven't yet been forked.)

A typical production slave has 1000-2000 leftover framework directories, but 
since we haven't rolled sandbox checkpointing yet, that's 1000-2000 
TaskDetectors we must construct and query if we don't filter by tasks that have 
this feature enabled.  That filtering definitely seems like it belongs in the 
MesosPathDetector.


- Brian


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


On March 17, 2015, 8:24 p.m., Brian Wickman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32013/
> ---
> 
> (Updated March 17, 2015, 8:24 p.m.)
> 
> 
> Review request for Aurora, Joe Smith and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> SSIA
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/path_detector.py 
> e4135cca09b43e204c52774b747f74212d0a5732 
>   src/main/python/apache/thermos/observer/detector.py 
> 5347cded7061b085b135d62fdb3a5c4a75f81e5c 
>   src/test/python/apache/aurora/executor/common/test_path_detector.py 
> 2d6edae420c1c5b51fe93f97e04c24976afacd51 
> 
> Diff: https://reviews.apache.org/r/32013/diff/
> 
> 
> Testing
> ---
> 
> ./pants test src/test/python/apache/thermos/observer:: + manual observation
> 
> 
> Thanks,
> 
> Brian Wickman
> 
>

Review Request 32171: Change "update list" subcommand to accept a hierarchy.

[Bill Farner]

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

Review request for Aurora, David McLaughlin and Zameer Manji.


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


Repository: aurora


Description
---

Change "update list" subcommand to accept a hierarchy.


Diffs
-

  src/main/python/apache/aurora/client/cli/update.py 
f025d46d50592156e2455313890e981722ab63a5 
  src/test/python/apache/aurora/client/cli/test_supdate.py 
cb66439a778349fc5add4985a7395655c9e1328a 

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


Testing
---


Thanks,

Bill Farner

Re: Review Request 32013: Cache TaskDetectors for reuse.

[Aurora ReviewBot]

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

Ship it!


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

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

- Aurora ReviewBot


On March 17, 2015, 8:24 p.m., Brian Wickman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32013/
> ---
> 
> (Updated March 17, 2015, 8:24 p.m.)
> 
> 
> Review request for Aurora, Joe Smith and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> SSIA
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/path_detector.py 
> e4135cca09b43e204c52774b747f74212d0a5732 
>   src/main/python/apache/thermos/observer/detector.py 
> 5347cded7061b085b135d62fdb3a5c4a75f81e5c 
>   src/test/python/apache/aurora/executor/common/test_path_detector.py 
> 2d6edae420c1c5b51fe93f97e04c24976afacd51 
> 
> Diff: https://reviews.apache.org/r/32013/diff/
> 
> 
> Testing
> ---
> 
> ./pants test src/test/python/apache/thermos/observer:: + manual observation
> 
> 
> Thanks,
> 
> Brian Wickman
> 
>

Re: Review Request 32141: Introduce AnnotatedAuroraAdmin superclass for annotations.

[Joshua Cohen]

> On March 17, 2015, 7:56 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java,
> >  line 39
> > 
> >
> > Can you add a corresponding comment to api.thrift that any new methods 
> > (or parameters?) added should be added (or updated?) here.
> 
> Kevin Sweeney wrote:
> I'll followup with a test case that ensures all 
> AuroraSchedulerManager.Iface methods are represented here.

Even better, thanks!


- Joshua


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


On March 17, 2015, 8:03 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32141/
> ---
> 
> (Updated March 17, 2015, 8:03 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1187
> https://issues.apache.org/jira/browse/AURORA-1187
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add an AnnotatedAuroraAdmin subclass for AuroraAdmin.Iface and change code to 
> inherit from it. This gives us a place to put annotations like the 
> AuthorizingParam one introduced in this review without needing to copy-paste 
> them when we override a new method. A future diff will use these annotations 
> to determine which permission a method call needs by inspecting the annotated 
> parameter. I created a new interface to enable DRY - otherwise I'd need to 
> annotate both ForwardingThrift and SchedulerThriftInterface and keep them in 
> sync.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  6b15bfdf727e2db57327936a0341d5dce98bd47c 
>   src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
> d9184eb540b82c988e7ac590d5cff441f37e62fb 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java
>  f0e40b646092e96955fddc46c3a1e62a8237b00f 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java 
> df6b53a524b005cd5fabb099fd0c08d83e3b287d 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptorTest.java
>  ee98f66de7f671018fa0a0b4894f114c7a283eda 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> 3900c2228038668576cdbb37e87127246a33317c 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java 
> 7b1bf2ef8b413d2c1a08b41722a04af091305304 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
>  7e20aaa6836bd205261afe5b1244fb6af8a56356 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptorTest.java
>  aae5cd7709abe3896c2ae06c218a0c90ca11c576 
> 
> Diff: https://reviews.apache.org/r/32141/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 32013: Cache TaskDetectors for reuse.

[Joe Smith]

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

Ship it!


Ship It!

- Joe Smith


On March 17, 2015, 1:24 p.m., Brian Wickman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32013/
> ---
> 
> (Updated March 17, 2015, 1:24 p.m.)
> 
> 
> Review request for Aurora, Joe Smith and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> SSIA
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/path_detector.py 
> e4135cca09b43e204c52774b747f74212d0a5732 
>   src/main/python/apache/thermos/observer/detector.py 
> 5347cded7061b085b135d62fdb3a5c4a75f81e5c 
>   src/test/python/apache/aurora/executor/common/test_path_detector.py 
> 2d6edae420c1c5b51fe93f97e04c24976afacd51 
> 
> Diff: https://reviews.apache.org/r/32013/diff/
> 
> 
> Testing
> ---
> 
> ./pants test src/test/python/apache/thermos/observer:: + manual observation
> 
> 
> Thanks,
> 
> Brian Wickman
> 
>

Re: Review Request 32013: Cache TaskDetectors for reuse.

[Brian Wickman]

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

(Updated March 17, 2015, 8:24 p.m.)


Review request for Aurora, Joe Smith and Zameer Manji.


Changes
---

rebase against master


Repository: aurora


Description
---

SSIA


Diffs (updated)
-

  src/main/python/apache/aurora/executor/common/path_detector.py 
e4135cca09b43e204c52774b747f74212d0a5732 
  src/main/python/apache/thermos/observer/detector.py 
5347cded7061b085b135d62fdb3a5c4a75f81e5c 
  src/test/python/apache/aurora/executor/common/test_path_detector.py 
2d6edae420c1c5b51fe93f97e04c24976afacd51 

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


Testing
---

./pants test src/test/python/apache/thermos/observer:: + manual observation


Thanks,

Brian Wickman

Re: Review Request 32141: Introduce AnnotatedAuroraAdmin superclass for annotations.

[Aurora ReviewBot]

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

Ship it!


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

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

- Aurora ReviewBot


On March 17, 2015, 8:03 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32141/
> ---
> 
> (Updated March 17, 2015, 8:03 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1187
> https://issues.apache.org/jira/browse/AURORA-1187
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add an AnnotatedAuroraAdmin subclass for AuroraAdmin.Iface and change code to 
> inherit from it. This gives us a place to put annotations like the 
> AuthorizingParam one introduced in this review without needing to copy-paste 
> them when we override a new method. A future diff will use these annotations 
> to determine which permission a method call needs by inspecting the annotated 
> parameter. I created a new interface to enable DRY - otherwise I'd need to 
> annotate both ForwardingThrift and SchedulerThriftInterface and keep them in 
> sync.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  6b15bfdf727e2db57327936a0341d5dce98bd47c 
>   src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
> d9184eb540b82c988e7ac590d5cff441f37e62fb 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java
>  f0e40b646092e96955fddc46c3a1e62a8237b00f 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java 
> df6b53a524b005cd5fabb099fd0c08d83e3b287d 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptorTest.java
>  ee98f66de7f671018fa0a0b4894f114c7a283eda 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> 3900c2228038668576cdbb37e87127246a33317c 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java 
> 7b1bf2ef8b413d2c1a08b41722a04af091305304 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
>  7e20aaa6836bd205261afe5b1244fb6af8a56356 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptorTest.java
>  aae5cd7709abe3896c2ae06c218a0c90ca11c576 
> 
> Diff: https://reviews.apache.org/r/32141/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 32141: Introduce AnnotatedAuroraAdmin superclass for annotations.

[Kevin Sweeney]

> On March 17, 2015, 12:56 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java,
> >  line 39
> > 
> >
> > Can you add a corresponding comment to api.thrift that any new methods 
> > (or parameters?) added should be added (or updated?) here.

I'll followup with a test case that ensures all AuroraSchedulerManager.Iface 
methods are represented here.


- Kevin


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


On March 17, 2015, 1:03 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32141/
> ---
> 
> (Updated March 17, 2015, 1:03 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1187
> https://issues.apache.org/jira/browse/AURORA-1187
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add an AnnotatedAuroraAdmin subclass for AuroraAdmin.Iface and change code to 
> inherit from it. This gives us a place to put annotations like the 
> AuthorizingParam one introduced in this review without needing to copy-paste 
> them when we override a new method. A future diff will use these annotations 
> to determine which permission a method call needs by inspecting the annotated 
> parameter. I created a new interface to enable DRY - otherwise I'd need to 
> annotate both ForwardingThrift and SchedulerThriftInterface and keep them in 
> sync.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  6b15bfdf727e2db57327936a0341d5dce98bd47c 
>   src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
> d9184eb540b82c988e7ac590d5cff441f37e62fb 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java
>  f0e40b646092e96955fddc46c3a1e62a8237b00f 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java 
> df6b53a524b005cd5fabb099fd0c08d83e3b287d 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptorTest.java
>  ee98f66de7f671018fa0a0b4894f114c7a283eda 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> 3900c2228038668576cdbb37e87127246a33317c 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java 
> 7b1bf2ef8b413d2c1a08b41722a04af091305304 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
>  7e20aaa6836bd205261afe5b1244fb6af8a56356 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptorTest.java
>  aae5cd7709abe3896c2ae06c218a0c90ca11c576 
> 
> Diff: https://reviews.apache.org/r/32141/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 32141: Introduce AnnotatedAuroraAdmin superclass for annotations.

[Kevin Sweeney]

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

(Updated March 17, 2015, 1:03 p.m.)


Review request for Aurora, Joshua Cohen and Bill Farner.


Changes
---

Fix bad rebase.


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


Repository: aurora


Description
---

Add an AnnotatedAuroraAdmin subclass for AuroraAdmin.Iface and change code to 
inherit from it. This gives us a place to put annotations like the 
AuthorizingParam one introduced in this review without needing to copy-paste 
them when we override a new method. A future diff will use these annotations to 
determine which permission a method call needs by inspecting the annotated 
parameter. I created a new interface to enable DRY - otherwise I'd need to 
annotate both ForwardingThrift and SchedulerThriftInterface and keep them in 
sync.


Diffs (updated)
-

  
src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
 PRE-CREATION 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
6b15bfdf727e2db57327936a0341d5dce98bd47c 
  src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
d9184eb540b82c988e7ac590d5cff441f37e62fb 
  
src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java 
PRE-CREATION 
  
src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java 
f0e40b646092e96955fddc46c3a1e62a8237b00f 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java 
df6b53a524b005cd5fabb099fd0c08d83e3b287d 
  
src/test/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptorTest.java
 ee98f66de7f671018fa0a0b4894f114c7a283eda 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
3900c2228038668576cdbb37e87127246a33317c 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java 
7b1bf2ef8b413d2c1a08b41722a04af091305304 
  
src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
 7e20aaa6836bd205261afe5b1244fb6af8a56356 
  
src/test/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptorTest.java
 aae5cd7709abe3896c2ae06c218a0c90ca11c576 

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


Testing
---

./gradlew build


Thanks,

Kevin Sweeney

Re: Review Request 32141: Introduce AnnotatedAuroraAdmin superclass for annotations.

[Joshua Cohen]

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

Ship it!



src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java


Can you add a corresponding comment to api.thrift that any new methods (or 
parameters?) added should be added (or updated?) here.


- Joshua Cohen


On March 17, 2015, 7:41 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32141/
> ---
> 
> (Updated March 17, 2015, 7:41 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1187
> https://issues.apache.org/jira/browse/AURORA-1187
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add an AnnotatedAuroraAdmin subclass for AuroraAdmin.Iface and change code to 
> inherit from it. This gives us a place to put annotations like the 
> AuthorizingParam one introduced in this review without needing to copy-paste 
> them when we override a new method. A future diff will use these annotations 
> to determine which permission a method call needs by inspecting the annotated 
> parameter. I created a new interface to enable DRY - otherwise I'd need to 
> annotate both ForwardingThrift and SchedulerThriftInterface and keep them in 
> sync.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  6b15bfdf727e2db57327936a0341d5dce98bd47c 
>   src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
> d9184eb540b82c988e7ac590d5cff441f37e62fb 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java
>  f0e40b646092e96955fddc46c3a1e62a8237b00f 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java 
> df6b53a524b005cd5fabb099fd0c08d83e3b287d 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptorTest.java
>  ee98f66de7f671018fa0a0b4894f114c7a283eda 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> 3900c2228038668576cdbb37e87127246a33317c 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java 
> 7b1bf2ef8b413d2c1a08b41722a04af091305304 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
>  7e20aaa6836bd205261afe5b1244fb6af8a56356 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptorTest.java
>  aae5cd7709abe3896c2ae06c218a0c90ca11c576 
> 
> Diff: https://reviews.apache.org/r/32141/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 32161: Create a packer definition to build a base vagrant box.

[Joshua Cohen]

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


I tried this out locally and `vagrant up` wall time went from 5m24s before this 
change to 3m16s afterwards.

Overall +1 on this, but I definitely think we need to add a README in the 
packer directory to explain what's going on, how to build/upload a new box, etc.

- Joshua Cohen


On March 17, 2015, 4:23 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32161/
> ---
> 
> (Updated March 17, 2015, 4:23 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Kevin Sweeney.
> 
> 
> Bugs: AURORA-1203
> https://issues.apache.org/jira/browse/AURORA-1203
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Mostly getting the conversation started.  This change pushes most of the 
> lower-velocity components down into a vagrant base box [1], which we would 
> fetch from vagrant cloud for the time being, or on ASF infra if/when freemium 
> ends on Atlas [2].  I managed to an example project that served as a good 
> guide [3].
> 
> Getting ahead of potential requests - commenting within packer json is not 
> supported.
> 
> So far i have moved base package installation out of vagrant provisioning, 
> and have seeded the gradle cache.  With a cached machine image (~900 MiB), 
> `vagrant up` took 5m24s (caveat: sample size 1).  If we seed the python 
> artifact cache as well, we should be able to shave off another minure or two.
> 
> [1] http://docs.vagrantup.com/v2/boxes/base.html
> [2] https://atlas.hashicorp.com/
> [3] 
> https://bitbucket.org/ariya/packer-vagrant-linux/src/d0359d7e64e2?at=master
> 
> This change will not be submitted as-is, since i still need to host the 
> machine image, but i wanted to get your thoughts on it before i proceed 
> further.
> 
> 
> Diffs
> -
> 
>   Vagrantfile 2d6c2ae598e80035840f7e517e161be266b581dd 
>   examples/packer/aurora.json PRE-CREATION 
>   examples/packer/http/ubuntu-14.04-amd64/preseed.cfg PRE-CREATION 
>   examples/packer/scripts/aurora.sh PRE-CREATION 
>   examples/packer/scripts/compact.sh PRE-CREATION 
>   examples/packer/scripts/vagrant.sh PRE-CREATION 
>   examples/vagrant/provision-dev-cluster.sh 
> ae500436e703140065e5c16fc0e38dbe3214e69f 
> 
> Diff: https://reviews.apache.org/r/32161/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 32013: Cache TaskDetectors for reuse.

[Zameer Manji]

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



src/main/python/apache/aurora/executor/common/path_detector.py


What's the rationale behind this change? I think the callers should be 
doing the checking for existance and dealing with missing directories as needed.


- Zameer Manji


On March 16, 2015, 3:37 p.m., Brian Wickman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32013/
> ---
> 
> (Updated March 16, 2015, 3:37 p.m.)
> 
> 
> Review request for Aurora, Joe Smith and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> SSIA
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/executor/common/path_detector.py 
> e4135cca09b43e204c52774b747f74212d0a5732 
>   src/main/python/apache/thermos/observer/detector.py 
> 5347cded7061b085b135d62fdb3a5c4a75f81e5c 
>   src/test/python/apache/aurora/executor/common/test_path_detector.py 
> 2d6edae420c1c5b51fe93f97e04c24976afacd51 
>   src/test/python/apache/thermos/core/test_process.py 
> e261249b977802851ffc3d89437761c532fcd3f8 
> 
> Diff: https://reviews.apache.org/r/32013/diff/
> 
> 
> Testing
> ---
> 
> ./pants test src/test/python/apache/thermos/observer:: + manual observation
> 
> 
> Thanks,
> 
> Brian Wickman
> 
>

Re: Review Request 32141: Introduce AnnotatedAuroraAdmin superclass for annotations.

[Aurora ReviewBot]

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


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

make[2]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1'
make[1]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1'
make: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift'
:api:classesThriftNote: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.

:api:checkPython
:api:generateThriftEntitiesJava
:api:classesThriftEntities
:api:compileJava UP-TO-DATE
:api:generateThriftResources
:api:processResources UP-TO-DATE
:api:classes
:api:jar
:compileJava/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java:20:
 error: cannot find symbol
import 
org.apache.aurora.scheduler.thrift.SchedulerThriftInterface.EnableUpdater;
  ^
  symbol:   class EnableUpdater
  location: class SchedulerThriftInterface
Note: Writing 
file:/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/classes/main/com/twitter/common/args/apt/cmdline.arg.info.txt.2
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java:20:
 error: cannot find symbol
import 
org.apache.aurora.scheduler.thrift.SchedulerThriftInterface.EnableUpdater;
  ^
  symbol:   class EnableUpdater
  location: class SchedulerThriftInterface
1 error
 FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':compileJava'.
> Compilation failed; see the compiler error output for details.

* 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 23.181 secs


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

- Aurora ReviewBot


On March 17, 2015, 7:41 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32141/
> ---
> 
> (Updated March 17, 2015, 7:41 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1187
> https://issues.apache.org/jira/browse/AURORA-1187
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add an AnnotatedAuroraAdmin subclass for AuroraAdmin.Iface and change code to 
> inherit from it. This gives us a place to put annotations like the 
> AuthorizingParam one introduced in this review without needing to copy-paste 
> them when we override a new method. A future diff will use these annotations 
> to determine which permission a method call needs by inspecting the annotated 
> parameter. I created a new interface to enable DRY - otherwise I'd need to 
> annotate both ForwardingThrift and SchedulerThriftInterface and keep them in 
> sync.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  6b15bfdf727e2db57327936a0341d5dce98bd47c 
>   src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
> d9184eb540b82c988e7ac590d5cff441f37e62fb 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java
>  f0e40b646092e96955fddc46c3a1e62a8237b00f 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java 
> df6b53a524b005cd5fabb099fd0c08d83e3b287d 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptorTest.java
>  ee98f66de7f671018fa0a0b4894f114c7a283eda 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> 3900c2228038668576cdbb37e87127246a33317c 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java 
> 7b1bf2ef8b413d2c1a08b41722a04af091305304 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
>  7e20aaa6836bd205261afe5b1244fb6af8a56356 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptorTest.java
>  aae5cd7709abe3896c2ae06c218a0c90ca11c576 
> 
> Diff: https://reviews.apache.org/r/32141/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 32012: Improve TaskDetector performance by memoizing regular expressions

[Joe Smith]

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

Ship it!


- Joe Smith


On March 13, 2015, 12:38 p.m., Brian Wickman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32012/
> ---
> 
> (Updated March 13, 2015, 12:38 p.m.)
> 
> 
> Review request for Aurora, Joe Smith and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> We found that the new observer burns 10x the cpu in production.  Most of the 
> time is spent in compiling new regular expressions, so memoize them.  
> Follow-up review adds TaskDetector caching in observer.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/thermos/common/path.py 
> 4359a5143551fff3cf66e39e68ba9ba02b8d6e2e 
>   src/main/python/apache/thermos/monitoring/detector.py 
> e0922e61ef03fc578988f992aba46570ea1b5d6c 
>   src/test/python/apache/thermos/core/test_process.py 
> e261249b977802851ffc3d89437761c532fcd3f8 
>   src/test/python/apache/thermos/monitoring/BUILD 
> 4e99da403f322940579fd4f9412e022df0790884 
>   src/test/python/apache/thermos/monitoring/test_detector.py 
> e9005c402ec81bbb415d9c814aa7b4e759138304 
> 
> Diff: https://reviews.apache.org/r/32012/diff/
> 
> 
> Testing
> ---
> 
> Added generic TaskDetector test and made sure it passes before/after.
> 
> 
> Thanks,
> 
> Brian Wickman
> 
>

Re: Review Request 32141: Introduce AnnotatedAuroraAdmin superclass for annotations.

[Kevin Sweeney]

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

(Updated March 17, 2015, 12:41 p.m.)


Review request for Aurora, Joshua Cohen and Bill Farner.


Changes
---

rebase


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


Repository: aurora


Description
---

Add an AnnotatedAuroraAdmin subclass for AuroraAdmin.Iface and change code to 
inherit from it. This gives us a place to put annotations like the 
AuthorizingParam one introduced in this review without needing to copy-paste 
them when we override a new method. A future diff will use these annotations to 
determine which permission a method call needs by inspecting the annotated 
parameter. I created a new interface to enable DRY - otherwise I'd need to 
annotate both ForwardingThrift and SchedulerThriftInterface and keep them in 
sync.


Diffs (updated)
-

  
src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
 PRE-CREATION 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
6b15bfdf727e2db57327936a0341d5dce98bd47c 
  src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
d9184eb540b82c988e7ac590d5cff441f37e62fb 
  
src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java 
PRE-CREATION 
  
src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java 
f0e40b646092e96955fddc46c3a1e62a8237b00f 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java 
df6b53a524b005cd5fabb099fd0c08d83e3b287d 
  
src/test/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptorTest.java
 ee98f66de7f671018fa0a0b4894f114c7a283eda 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
3900c2228038668576cdbb37e87127246a33317c 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java 
7b1bf2ef8b413d2c1a08b41722a04af091305304 
  
src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
 7e20aaa6836bd205261afe5b1244fb6af8a56356 
  
src/test/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptorTest.java
 aae5cd7709abe3896c2ae06c218a0c90ca11c576 

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


Testing
---

./gradlew build


Thanks,

Kevin Sweeney

Re: Review Request 32141: Introduce AnnotatedAuroraAdmin superclass for annotations.

[Aurora ReviewBot]

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


This patch does not apply cleanly on master (38d0a6c), do you need to rebase?

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

- Aurora ReviewBot


On March 17, 2015, 7:18 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32141/
> ---
> 
> (Updated March 17, 2015, 7:18 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1187
> https://issues.apache.org/jira/browse/AURORA-1187
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add an AnnotatedAuroraAdmin subclass for AuroraAdmin.Iface and change code to 
> inherit from it. This gives us a place to put annotations like the 
> AuthorizingParam one introduced in this review without needing to copy-paste 
> them when we override a new method. A future diff will use these annotations 
> to determine which permission a method call needs by inspecting the annotated 
> parameter. I created a new interface to enable DRY - otherwise I'd need to 
> annotate both ForwardingThrift and SchedulerThriftInterface and keep them in 
> sync.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c60d1e78e0f5c474ff0dcf6393e0c305d865bded 
>   src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
> b1d633602f3fb021a026a087b91d41580798eeaf 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java
>  f0e40b646092e96955fddc46c3a1e62a8237b00f 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java 
> df6b53a524b005cd5fabb099fd0c08d83e3b287d 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptorTest.java
>  ee98f66de7f671018fa0a0b4894f114c7a283eda 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> 3900c2228038668576cdbb37e87127246a33317c 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java 
> 7b1bf2ef8b413d2c1a08b41722a04af091305304 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
>  7e20aaa6836bd205261afe5b1244fb6af8a56356 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptorTest.java
>  aae5cd7709abe3896c2ae06c218a0c90ca11c576 
> 
> Diff: https://reviews.apache.org/r/32141/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 32141: Introduce AnnotatedAuroraAdmin superclass for annotations.

[Kevin Sweeney]

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

(Updated March 17, 2015, 12:18 p.m.)


Review request for Aurora, Joshua Cohen and Bill Farner.


Changes
---

Remove @Inherited, which is only defined to have any meaning for class 
annotations.


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


Repository: aurora


Description
---

Add an AnnotatedAuroraAdmin subclass for AuroraAdmin.Iface and change code to 
inherit from it. This gives us a place to put annotations like the 
AuthorizingParam one introduced in this review without needing to copy-paste 
them when we override a new method. A future diff will use these annotations to 
determine which permission a method call needs by inspecting the annotated 
parameter. I created a new interface to enable DRY - otherwise I'd need to 
annotate both ForwardingThrift and SchedulerThriftInterface and keep them in 
sync.


Diffs (updated)
-

  
src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
 PRE-CREATION 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
c60d1e78e0f5c474ff0dcf6393e0c305d865bded 
  src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
b1d633602f3fb021a026a087b91d41580798eeaf 
  
src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java 
PRE-CREATION 
  
src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java 
f0e40b646092e96955fddc46c3a1e62a8237b00f 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java 
df6b53a524b005cd5fabb099fd0c08d83e3b287d 
  
src/test/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptorTest.java
 ee98f66de7f671018fa0a0b4894f114c7a283eda 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
3900c2228038668576cdbb37e87127246a33317c 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java 
7b1bf2ef8b413d2c1a08b41722a04af091305304 
  
src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
 7e20aaa6836bd205261afe5b1244fb6af8a56356 
  
src/test/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptorTest.java
 aae5cd7709abe3896c2ae06c218a0c90ca11c576 

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


Testing
---

./gradlew build


Thanks,

Kevin Sweeney

Re: Review Request 32141: Introduce AnnotatedAuroraAdmin superclass for annotations.

[Kevin Sweeney]

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

(Updated March 17, 2015, 12:16 p.m.)


Review request for Aurora, Joshua Cohen and Bill Farner.


Changes
---

More nullables.


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


Repository: aurora


Description
---

Add an AnnotatedAuroraAdmin subclass for AuroraAdmin.Iface and change code to 
inherit from it. This gives us a place to put annotations like the 
AuthorizingParam one introduced in this review without needing to copy-paste 
them when we override a new method. A future diff will use these annotations to 
determine which permission a method call needs by inspecting the annotated 
parameter. I created a new interface to enable DRY - otherwise I'd need to 
annotate both ForwardingThrift and SchedulerThriftInterface and keep them in 
sync.


Diffs (updated)
-

  
src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
 PRE-CREATION 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
c60d1e78e0f5c474ff0dcf6393e0c305d865bded 
  src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
b1d633602f3fb021a026a087b91d41580798eeaf 
  
src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java 
PRE-CREATION 
  
src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java 
f0e40b646092e96955fddc46c3a1e62a8237b00f 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java 
df6b53a524b005cd5fabb099fd0c08d83e3b287d 
  
src/test/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptorTest.java
 ee98f66de7f671018fa0a0b4894f114c7a283eda 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
3900c2228038668576cdbb37e87127246a33317c 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java 
7b1bf2ef8b413d2c1a08b41722a04af091305304 
  
src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
 7e20aaa6836bd205261afe5b1244fb6af8a56356 
  
src/test/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptorTest.java
 aae5cd7709abe3896c2ae06c218a0c90ca11c576 

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


Testing
---

./gradlew build


Thanks,

Kevin Sweeney

Re: Review Request 32141: Introduce AnnotatedAuroraAdmin superclass for annotations.

[Maxim Khutornenko]

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

Ship it!



src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java


You probably want @Nullable here as well.



src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java


same here


- Maxim Khutornenko


On March 17, 2015, 6:55 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32141/
> ---
> 
> (Updated March 17, 2015, 6:55 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1187
> https://issues.apache.org/jira/browse/AURORA-1187
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add an AnnotatedAuroraAdmin subclass for AuroraAdmin.Iface and change code to 
> inherit from it. This gives us a place to put annotations like the 
> AuthorizingParam one introduced in this review without needing to copy-paste 
> them when we override a new method. A future diff will use these annotations 
> to determine which permission a method call needs by inspecting the annotated 
> parameter. I created a new interface to enable DRY - otherwise I'd need to 
> annotate both ForwardingThrift and SchedulerThriftInterface and keep them in 
> sync.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c60d1e78e0f5c474ff0dcf6393e0c305d865bded 
>   src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
> b1d633602f3fb021a026a087b91d41580798eeaf 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java
>  f0e40b646092e96955fddc46c3a1e62a8237b00f 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java 
> df6b53a524b005cd5fabb099fd0c08d83e3b287d 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptorTest.java
>  ee98f66de7f671018fa0a0b4894f114c7a283eda 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> 3900c2228038668576cdbb37e87127246a33317c 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java 
> 7b1bf2ef8b413d2c1a08b41722a04af091305304 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
>  7e20aaa6836bd205261afe5b1244fb6af8a56356 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptorTest.java
>  aae5cd7709abe3896c2ae06c218a0c90ca11c576 
> 
> Diff: https://reviews.apache.org/r/32141/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 32141: Introduce AnnotatedAuroraAdmin superclass for annotations.

[Kevin Sweeney]

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

(Updated March 17, 2015, 11:55 a.m.)


Review request for Aurora, Joshua Cohen and Bill Farner.


Changes
---

Bill and Maxim's feedback.


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


Repository: aurora


Description
---

Add an AnnotatedAuroraAdmin subclass for AuroraAdmin.Iface and change code to 
inherit from it. This gives us a place to put annotations like the 
AuthorizingParam one introduced in this review without needing to copy-paste 
them when we override a new method. A future diff will use these annotations to 
determine which permission a method call needs by inspecting the annotated 
parameter. I created a new interface to enable DRY - otherwise I'd need to 
annotate both ForwardingThrift and SchedulerThriftInterface and keep them in 
sync.


Diffs (updated)
-

  
src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
 PRE-CREATION 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
c60d1e78e0f5c474ff0dcf6393e0c305d865bded 
  src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
b1d633602f3fb021a026a087b91d41580798eeaf 
  
src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java 
PRE-CREATION 
  
src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java 
f0e40b646092e96955fddc46c3a1e62a8237b00f 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java 
df6b53a524b005cd5fabb099fd0c08d83e3b287d 
  
src/test/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptorTest.java
 ee98f66de7f671018fa0a0b4894f114c7a283eda 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
3900c2228038668576cdbb37e87127246a33317c 
  src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java 
7b1bf2ef8b413d2c1a08b41722a04af091305304 
  
src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
 7e20aaa6836bd205261afe5b1244fb6af8a56356 
  
src/test/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptorTest.java
 aae5cd7709abe3896c2ae06c218a0c90ca11c576 

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


Testing
---

./gradlew build


Thanks,

Kevin Sweeney

Re: Review Request 32141: Introduce AnnotatedAuroraAdmin superclass for annotations.

[Kevin Sweeney]

> On March 17, 2015, 7:59 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java,
> >  line 27
> > 
> >
> >  for breaks.  check out the rendered javadoc in intellij to see what 
> > i mean.
> > 
> > ditto in other files

fixed


- Kevin


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


On March 16, 2015, 6:29 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32141/
> ---
> 
> (Updated March 16, 2015, 6:29 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1187
> https://issues.apache.org/jira/browse/AURORA-1187
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add an AnnotatedAuroraAdmin subclass for AuroraAdmin.Iface and change code to 
> inherit from it. This gives us a place to put annotations like the 
> AuthorizingParam one introduced in this review without needing to copy-paste 
> them when we override a new method. A future diff will use these annotations 
> to determine which permission a method call needs by inspecting the annotated 
> parameter. I created a new interface to enable DRY - otherwise I'd need to 
> annotate both ForwardingThrift and SchedulerThriftInterface and keep them in 
> sync.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c60d1e78e0f5c474ff0dcf6393e0c305d865bded 
>   src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
> b1d633602f3fb021a026a087b91d41580798eeaf 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java
>  f0e40b646092e96955fddc46c3a1e62a8237b00f 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java 
> df6b53a524b005cd5fabb099fd0c08d83e3b287d 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptorTest.java
>  ee98f66de7f671018fa0a0b4894f114c7a283eda 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> 3900c2228038668576cdbb37e87127246a33317c 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java 
> 7b1bf2ef8b413d2c1a08b41722a04af091305304 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
>  7e20aaa6836bd205261afe5b1244fb6af8a56356 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptorTest.java
>  aae5cd7709abe3896c2ae06c218a0c90ca11c576 
> 
> Diff: https://reviews.apache.org/r/32141/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 32141: Introduce AnnotatedAuroraAdmin superclass for annotations.

[Kevin Sweeney]

> On March 17, 2015, 9:31 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java,
> >  line 60
> > 
> >
> > What's the guidance behind usage of @Nullable here? Technically, the 
> > `shardsId` could come as NULL from thrift as well as `TaskQuery` in 
> > `killTasks`. Both are required for RPC operation but the latter one is 
> > annotated with @Nullable.

This is a good point - I was mistaken about the codegen's handling of null 
collection types. Added Nullable here as well.


- Kevin


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


On March 16, 2015, 6:29 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32141/
> ---
> 
> (Updated March 16, 2015, 6:29 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1187
> https://issues.apache.org/jira/browse/AURORA-1187
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add an AnnotatedAuroraAdmin subclass for AuroraAdmin.Iface and change code to 
> inherit from it. This gives us a place to put annotations like the 
> AuthorizingParam one introduced in this review without needing to copy-paste 
> them when we override a new method. A future diff will use these annotations 
> to determine which permission a method call needs by inspecting the annotated 
> parameter. I created a new interface to enable DRY - otherwise I'd need to 
> annotate both ForwardingThrift and SchedulerThriftInterface and keep them in 
> sync.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c60d1e78e0f5c474ff0dcf6393e0c305d865bded 
>   src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
> b1d633602f3fb021a026a087b91d41580798eeaf 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java
>  f0e40b646092e96955fddc46c3a1e62a8237b00f 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java 
> df6b53a524b005cd5fabb099fd0c08d83e3b287d 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptorTest.java
>  ee98f66de7f671018fa0a0b4894f114c7a283eda 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> 3900c2228038668576cdbb37e87127246a33317c 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java 
> 7b1bf2ef8b413d2c1a08b41722a04af091305304 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
>  7e20aaa6836bd205261afe5b1244fb6af8a56356 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptorTest.java
>  aae5cd7709abe3896c2ae06c218a0c90ca11c576 
> 
> Diff: https://reviews.apache.org/r/32141/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 32164: Moving pending task search into PreemptorImpl

[Bill Farner]

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

Ship it!


Ship It!

- Bill Farner


On March 17, 2015, 5:19 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32164/
> ---
> 
> (Updated March 17, 2015, 5:19 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a TODO left after refactoring in step 1.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
> 9b7a3f16ad489c296d5e3513a74264a0620942a6 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
>  e5cc48625cf019f1b6bb85749711528bf5dee4cd 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImpl.java 
> e11db4e0e04fe3ca089defc0304333cbb8bf7624 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
>  93cab3414e3cd76e02b34c439413e625037bf90c 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
>  f27ca62397e60b114f0c570cdfe9323a6c825645 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java
>  9a6e94dc941025fd272495de0c46f77b793e867c 
> 
> Diff: https://reviews.apache.org/r/32164/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 32164: Moving pending task search into PreemptorImpl

[Bill Farner]

> On March 17, 2015, 6:05 p.m., Bill Farner wrote:
> > I actually meant this should be outside the preemptor altogether.  The 
> > preemptor is being called, and then internally deciding the caller should 
> > have not called in the first place.  I claim this is odd behavior.
> > 
> > I think it would make sense, instead, if the caller opened a transaction, 
> > and continued that transaction into the preemptor once it has deemed the 
> > task is `PENDING` and has been in that state long enough to warrant a 
> > preemption search.
> 
> Maxim Khutornenko wrote:
> After the final refactoring step is implemented, the preemptor will be 
> guided by a scheduled background worker most likely residing within the 
> PreemptorImpl itself. So, it will be the only caller and consumer of pending 
> tasks. I don't think it's worth purging pending task search out at this stage 
> just to return it back shortly after.

That makes sense, thanks for adding context.


- Bill


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


On March 17, 2015, 5:19 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32164/
> ---
> 
> (Updated March 17, 2015, 5:19 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a TODO left after refactoring in step 1.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
> 9b7a3f16ad489c296d5e3513a74264a0620942a6 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
>  e5cc48625cf019f1b6bb85749711528bf5dee4cd 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImpl.java 
> e11db4e0e04fe3ca089defc0304333cbb8bf7624 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
>  93cab3414e3cd76e02b34c439413e625037bf90c 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
>  f27ca62397e60b114f0c570cdfe9323a6c825645 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java
>  9a6e94dc941025fd272495de0c46f77b793e867c 
> 
> Diff: https://reviews.apache.org/r/32164/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 32164: Moving pending task search into PreemptorImpl

[Maxim Khutornenko]

> On March 17, 2015, 6:05 p.m., Bill Farner wrote:
> > I actually meant this should be outside the preemptor altogether.  The 
> > preemptor is being called, and then internally deciding the caller should 
> > have not called in the first place.  I claim this is odd behavior.
> > 
> > I think it would make sense, instead, if the caller opened a transaction, 
> > and continued that transaction into the preemptor once it has deemed the 
> > task is `PENDING` and has been in that state long enough to warrant a 
> > preemption search.

After the final refactoring step is implemented, the preemptor will be guided 
by a scheduled background worker most likely residing within the PreemptorImpl 
itself. So, it will be the only caller and consumer of pending tasks. I don't 
think it's worth purging pending task search out at this stage just to return 
it back shortly after.


- Maxim


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


On March 17, 2015, 5:19 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32164/
> ---
> 
> (Updated March 17, 2015, 5:19 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a TODO left after refactoring in step 1.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
> 9b7a3f16ad489c296d5e3513a74264a0620942a6 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
>  e5cc48625cf019f1b6bb85749711528bf5dee4cd 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImpl.java 
> e11db4e0e04fe3ca089defc0304333cbb8bf7624 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
>  93cab3414e3cd76e02b34c439413e625037bf90c 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
>  f27ca62397e60b114f0c570cdfe9323a6c825645 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java
>  9a6e94dc941025fd272495de0c46f77b793e867c 
> 
> Diff: https://reviews.apache.org/r/32164/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 32164: Moving pending task search into PreemptorImpl

[Bill Farner]

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


I actually meant this should be outside the preemptor altogether.  The 
preemptor is being called, and then internally deciding the caller should have 
not called in the first place.  I claim this is odd behavior.

I think it would make sense, instead, if the caller opened a transaction, and 
continued that transaction into the preemptor once it has deemed the task is 
`PENDING` and has been in that state long enough to warrant a preemption search.

- Bill Farner


On March 17, 2015, 5:19 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32164/
> ---
> 
> (Updated March 17, 2015, 5:19 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a TODO left after refactoring in step 1.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
> 9b7a3f16ad489c296d5e3513a74264a0620942a6 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
>  e5cc48625cf019f1b6bb85749711528bf5dee4cd 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImpl.java 
> e11db4e0e04fe3ca089defc0304333cbb8bf7624 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
>  93cab3414e3cd76e02b34c439413e625037bf90c 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
>  f27ca62397e60b114f0c570cdfe9323a6c825645 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java
>  9a6e94dc941025fd272495de0c46f77b793e867c 
> 
> Diff: https://reviews.apache.org/r/32164/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 32164: Moving pending task search into PreemptorImpl

[Aurora ReviewBot]

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

Ship it!


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

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

- Aurora ReviewBot


On March 17, 2015, 5:19 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32164/
> ---
> 
> (Updated March 17, 2015, 5:19 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Addressing a TODO left after refactoring in step 1.
> 
> 
> Diffs
> -
> 
>   src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
> 9b7a3f16ad489c296d5e3513a74264a0620942a6 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
>  e5cc48625cf019f1b6bb85749711528bf5dee4cd 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImpl.java 
> e11db4e0e04fe3ca089defc0304333cbb8bf7624 
>   
> src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
>  93cab3414e3cd76e02b34c439413e625037bf90c 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
>  f27ca62397e60b114f0c570cdfe9323a6c825645 
>   
> src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java
>  9a6e94dc941025fd272495de0c46f77b793e867c 
> 
> Diff: https://reviews.apache.org/r/32164/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Review Request 32164: Moving pending task search into PreemptorImpl

[Maxim Khutornenko]

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

Review request for Aurora and Bill Farner.


Repository: aurora


Description
---

Addressing a TODO left after refactoring in step 1.


Diffs
-

  src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java 
9b7a3f16ad489c296d5e3513a74264a0620942a6 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionSlotFinder.java
 e5cc48625cf019f1b6bb85749711528bf5dee4cd 
  src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImpl.java 
e11db4e0e04fe3ca089defc0304333cbb8bf7624 
  
src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java 
93cab3414e3cd76e02b34c439413e625037bf90c 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
 f27ca62397e60b114f0c570cdfe9323a6c825645 
  
src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorSlotFinderTest.java
 9a6e94dc941025fd272495de0c46f77b793e867c 

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


Testing
---

./gradlew -Pq build


Thanks,

Maxim Khutornenko

Re: Review Request 32161: Create a packer definition to build a base vagrant box.

[Aurora ReviewBot]

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

Ship it!


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

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

- Aurora ReviewBot


On March 17, 2015, 4:23 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32161/
> ---
> 
> (Updated March 17, 2015, 4:23 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Kevin Sweeney.
> 
> 
> Bugs: AURORA-1203
> https://issues.apache.org/jira/browse/AURORA-1203
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Mostly getting the conversation started.  This change pushes most of the 
> lower-velocity components down into a vagrant base box [1], which we would 
> fetch from vagrant cloud for the time being, or on ASF infra if/when freemium 
> ends on Atlas [2].  I managed to an example project that served as a good 
> guide [3].
> 
> Getting ahead of potential requests - commenting within packer json is not 
> supported.
> 
> So far i have moved base package installation out of vagrant provisioning, 
> and have seeded the gradle cache.  With a cached machine image (~900 MiB), 
> `vagrant up` took 5m24s (caveat: sample size 1).  If we seed the python 
> artifact cache as well, we should be able to shave off another minure or two.
> 
> [1] http://docs.vagrantup.com/v2/boxes/base.html
> [2] https://atlas.hashicorp.com/
> [3] 
> https://bitbucket.org/ariya/packer-vagrant-linux/src/d0359d7e64e2?at=master
> 
> This change will not be submitted as-is, since i still need to host the 
> machine image, but i wanted to get your thoughts on it before i proceed 
> further.
> 
> 
> Diffs
> -
> 
>   Vagrantfile 2d6c2ae598e80035840f7e517e161be266b581dd 
>   examples/packer/aurora.json PRE-CREATION 
>   examples/packer/http/ubuntu-14.04-amd64/preseed.cfg PRE-CREATION 
>   examples/packer/scripts/aurora.sh PRE-CREATION 
>   examples/packer/scripts/compact.sh PRE-CREATION 
>   examples/packer/scripts/vagrant.sh PRE-CREATION 
>   examples/vagrant/provision-dev-cluster.sh 
> ae500436e703140065e5c16fc0e38dbe3214e69f 
> 
> Diff: https://reviews.apache.org/r/32161/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 32141: Introduce AnnotatedAuroraAdmin superclass for annotations.

[Maxim Khutornenko]

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



src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java


What's the guidance behind usage of @Nullable here? Technically, the 
`shardsId` could come as NULL from thrift as well as `TaskQuery` in 
`killTasks`. Both are required for RPC operation but the latter one is 
annotated with @Nullable.


- Maxim Khutornenko


On March 17, 2015, 1:29 a.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32141/
> ---
> 
> (Updated March 17, 2015, 1:29 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1187
> https://issues.apache.org/jira/browse/AURORA-1187
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add an AnnotatedAuroraAdmin subclass for AuroraAdmin.Iface and change code to 
> inherit from it. This gives us a place to put annotations like the 
> AuthorizingParam one introduced in this review without needing to copy-paste 
> them when we override a new method. A future diff will use these annotations 
> to determine which permission a method call needs by inspecting the annotated 
> parameter. I created a new interface to enable DRY - otherwise I'd need to 
> annotate both ForwardingThrift and SchedulerThriftInterface and keep them in 
> sync.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c60d1e78e0f5c474ff0dcf6393e0c305d865bded 
>   src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
> b1d633602f3fb021a026a087b91d41580798eeaf 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java
>  f0e40b646092e96955fddc46c3a1e62a8237b00f 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java 
> df6b53a524b005cd5fabb099fd0c08d83e3b287d 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptorTest.java
>  ee98f66de7f671018fa0a0b4894f114c7a283eda 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> 3900c2228038668576cdbb37e87127246a33317c 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java 
> 7b1bf2ef8b413d2c1a08b41722a04af091305304 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
>  7e20aaa6836bd205261afe5b1244fb6af8a56356 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptorTest.java
>  aae5cd7709abe3896c2ae06c218a0c90ca11c576 
> 
> Diff: https://reviews.apache.org/r/32141/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Review Request 32161: Create a packer definition to build a base vagrant box.

[Bill Farner]

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

Review request for Aurora, Joshua Cohen and Kevin Sweeney.


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


Repository: aurora


Description
---

Mostly getting the conversation started.  This change pushes most of the 
lower-velocity components down into a vagrant base box [1], which we would 
fetch from vagrant cloud for the time being, or on ASF infra if/when freemium 
ends on Atlas [2].  I managed to an example project that served as a good guide 
[3].

Getting ahead of potential requests - commenting within packer json is not 
supported.

So far i have moved base package installation out of vagrant provisioning, and 
have seeded the gradle cache.  With a cached machine image (~900 MiB), `vagrant 
up` took 5m24s (caveat: sample size 1).  If we seed the python artifact cache 
as well, we should be able to shave off another minure or two.

[1] http://docs.vagrantup.com/v2/boxes/base.html
[2] https://atlas.hashicorp.com/
[3] https://bitbucket.org/ariya/packer-vagrant-linux/src/d0359d7e64e2?at=master

This change will not be submitted as-is, since i still need to host the machine 
image, but i wanted to get your thoughts on it before i proceed further.


Diffs
-

  Vagrantfile 2d6c2ae598e80035840f7e517e161be266b581dd 
  examples/packer/aurora.json PRE-CREATION 
  examples/packer/http/ubuntu-14.04-amd64/preseed.cfg PRE-CREATION 
  examples/packer/scripts/aurora.sh PRE-CREATION 
  examples/packer/scripts/compact.sh PRE-CREATION 
  examples/packer/scripts/vagrant.sh PRE-CREATION 
  examples/vagrant/provision-dev-cluster.sh 
ae500436e703140065e5c16fc0e38dbe3214e69f 

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


Testing
---


Thanks,

Bill Farner

Re: Review Request 30818: Support separate routes for job controller tabs.

[Bill Farner]

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


Update or discard?

- Bill Farner


On Feb. 10, 2015, 6:01 a.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30818/
> ---
> 
> (Updated Feb. 10, 2015, 6:01 a.m.)
> 
> 
> Review request for Aurora and David McLaughlin.
> 
> 
> Bugs: AURORA-696
> https://issues.apache.org/jira/browse/AURORA-696
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Previously navigating between the active/completed/all tabs on the
> job controller page did not alter the browser history. Now it does,
> meaning that you can navigate with the back button as well as link
> directly to a tab.
> 
> 
> Diffs
> -
> 
>   src/main/resources/scheduler/assets/job.html 
> 4a00a8863d676f168fbfce9f45ec4bad0244199f 
>   src/main/resources/scheduler/assets/js/app.js 
> b66409f628046c67b62c92a75cbeed20c09b4ec1 
>   src/main/resources/scheduler/assets/js/controllers.js 
> 092e7d5df2121f45f99f5a788187d52bebb7e5dd 
> 
> Diff: https://reviews.apache.org/r/30818/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew jsHint
> 
> Verified push state worked in vagrant.
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>

Re: Review Request 32141: Introduce AnnotatedAuroraAdmin superclass for annotations.

[Bill Farner]

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

Ship it!


LGTM mod nits below and satisfying the build bot.


src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java


 for breaks.  check out the rendered javadoc in intellij to see what i 
mean.

ditto in other files



src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java


Maybe s/Annotated/Authorized/?  So as to say - implementations of this 
interface can be assured authorization has happened.  I don't feel strongly 
about this.


- Bill Farner


On March 17, 2015, 1:29 a.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32141/
> ---
> 
> (Updated March 17, 2015, 1:29 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1187
> https://issues.apache.org/jira/browse/AURORA-1187
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add an AnnotatedAuroraAdmin subclass for AuroraAdmin.Iface and change code to 
> inherit from it. This gives us a place to put annotations like the 
> AuthorizingParam one introduced in this review without needing to copy-paste 
> them when we override a new method. A future diff will use these annotations 
> to determine which permission a method call needs by inspecting the annotated 
> parameter. I created a new interface to enable DRY - otherwise I'd need to 
> annotate both ForwardingThrift and SchedulerThriftInterface and keep them in 
> sync.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/AuthorizingParam.java
>  PRE-CREATION 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  c60d1e78e0f5c474ff0dcf6393e0c305d865bded 
>   src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
> b1d633602f3fb021a026a087b91d41580798eeaf 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/ApiSecurityIT.java
>  f0e40b646092e96955fddc46c3a1e62a8237b00f 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/AopModuleTest.java 
> df6b53a524b005cd5fabb099fd0c08d83e3b287d 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptorTest.java
>  ee98f66de7f671018fa0a0b4894f114c7a283eda 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> 3900c2228038668576cdbb37e87127246a33317c 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java 
> 7b1bf2ef8b413d2c1a08b41722a04af091305304 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
>  7e20aaa6836bd205261afe5b1244fb6af8a56356 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/aop/ThriftStatsExporterInterceptorTest.java
>  aae5cd7709abe3896c2ae06c218a0c90ca11c576 
> 
> Diff: https://reviews.apache.org/r/32141/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 31380: Now that we've identified not calling .converge as a source of flakiness, remove epsilon from health checker tests.

[Bill Farner]

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


Is this review stale?  If so, discard?

- Bill Farner


On Feb. 24, 2015, 9:27 p.m., Brian Wickman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31380/
> ---
> 
> (Updated Feb. 24, 2015, 9:27 p.m.)
> 
> 
> Review request for Aurora and Joe Smith.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Now that we've identified not calling .converge as the source of the issue, 
> remove epsilon from health checker tests.
> 
> 
> Diffs
> -
> 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 1b4423a1eb95cc950206a355fa657c8082c8d93f 
> 
> Diff: https://reviews.apache.org/r/31380/diff/
> 
> 
> Testing
> ---
> 
> ¯\_(?)_/¯ 
> 
> Ran this for 20 minutes on Linux and OS X:
> 
> while true; do THERMOS_DEBUG=1 ./pants test.pytest --options='-v' 
> src/test/python/apache/aurora/executor/common:health_checker; done;
> 
> 
> Thanks,
> 
> Brian Wickman
> 
>