Re: Review Request 30890: Offer filtering for static vetoes. Part 2 of 4: Veto groups.

2015-02-12 Thread Bill Farner

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

Ship it!


Ship It!

- Bill Farner


On Feb. 12, 2015, 6:05 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30890/
 ---
 
 (Updated Feb. 12, 2015, 6:05 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Bugs: AURORA-909
 https://issues.apache.org/jira/browse/AURORA-909
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Offer filtering for static vetoes. Part 2 of 4: Exposing Veto groups and 
 refactoring TaskVars.
 
 Will not apply cleanly: diffed with https://reviews.apache.org/r/30888/ as a 
 parent.
 
 Original RB: https://reviews.apache.org/r/28617/
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/TaskVars.java 
 f017cdd26ca40138a7e141f21613ed567314c399 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
 6a43bcd1719e8aa32fd3fcb7387d0318c3c0b804 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
 f06fdaeb92e154d0982bdabed5df93e7bcba9048 
   src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
 4e7efb3c1214c3d193afd61f162713490eb8effb 
   
 src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
  52ee7c1e3742d9315c7e7aaa77677121e1e9288d 
 
 Diff: https://reviews.apache.org/r/30890/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30890: Offer filtering for static vetoes. Part 2 of 4: Veto groups.

2015-02-12 Thread Bill Farner


 On Feb. 11, 2015, 11:54 p.m., Bill Farner wrote:
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, 
  line 34
  https://reviews.apache.org/r/30890/diff/1/?file=861039#file861039line34
 
  I feel like `VetoType` still applies and is actually a better name.  
  It's odd for an enum value to be considered a 'group'.  (When i hear group 
  i think collection.)
 
 Maxim Khutornenko wrote:
 VetoType is used to describe the particular type of veto applied. This 
 enum denotes similar typed Veto groups. I am fine changing it to something 
 else if VetoGroup is confusing. Any suggestions?

My mistake, the diff convinced me that `VetoType` was renamed, but i see that 
it still exists.  How about `VetoReason` and s/`EMPTY`/`NONE`/?  Reads a bit 
more naturally from the calling side, but i don't feel strongly on that.


 On Feb. 11, 2015, 11:54 p.m., Bill Farner wrote:
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java, 
  line 115
  https://reviews.apache.org/r/30890/diff/1/?file=861040#file861040line115
 
  What's the thought process behind converting this to a `Set`?  The code 
  doesn't create duplicates, and you pick up the performance hit of a hash 
  table.
  
  I'm actually wondering if signatures should use `Iterable` all the way 
  out of this class.
 
 Maxim Khutornenko wrote:
 The only consumer of this method is already converting everything to a 
 set. It feels awkward to have Iterable in places where there should be no 
 duplicates to start with. I am -1 on converting all signatures from Set to 
 Iterable in this class. The interface contract should clearly state the 
 expectations.

I generally agree that `Set` should be preferred to communicate uniqueness to 
callers.  However, in the same vein as the performance sensitivity advised 
above, constructing a hash table non-negligible performance hit.  In case i was 
unclear, i suggest you change the `SchedulingFilter` signature as well, and 
avoid ever turning the result into a `Set`.

How would you feel about peeling off a ticket to explore this?  As i follow the 
result, it looks like it could ripple fairly wide and distract from this 
review.  I might have a chance to pick this up next sprint.


- Bill


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


On Feb. 12, 2015, 6:05 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30890/
 ---
 
 (Updated Feb. 12, 2015, 6:05 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Bugs: AURORA-909
 https://issues.apache.org/jira/browse/AURORA-909
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Offer filtering for static vetoes. Part 2 of 4: Exposing Veto groups and 
 refactoring TaskVars.
 
 Will not apply cleanly: diffed with https://reviews.apache.org/r/30888/ as a 
 parent.
 
 Original RB: https://reviews.apache.org/r/28617/
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/TaskVars.java 
 f017cdd26ca40138a7e141f21613ed567314c399 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
 6a43bcd1719e8aa32fd3fcb7387d0318c3c0b804 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
 f06fdaeb92e154d0982bdabed5df93e7bcba9048 
   src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
 4e7efb3c1214c3d193afd61f162713490eb8effb 
   
 src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
  52ee7c1e3742d9315c7e7aaa77677121e1e9288d 
 
 Diff: https://reviews.apache.org/r/30890/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30890: Offer filtering for static vetoes. Part 2 of 4: Veto groups.

2015-02-12 Thread Maxim Khutornenko


 On Feb. 11, 2015, 11:54 p.m., Bill Farner wrote:
  src/main/java/org/apache/aurora/scheduler/TaskVars.java, line 253
  https://reviews.apache.org/r/30890/diff/1/?file=861038#file861038line253
 
  s/SchedulingFilter.//

Done.


 On Feb. 11, 2015, 11:54 p.m., Bill Farner wrote:
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, 
  line 34
  https://reviews.apache.org/r/30890/diff/1/?file=861039#file861039line34
 
  I feel like `VetoType` still applies and is actually a better name.  
  It's odd for an enum value to be considered a 'group'.  (When i hear group 
  i think collection.)

VetoType is used to describe the particular type of veto applied. This enum 
denotes similar typed Veto groups. I am fine changing it to something else if 
VetoGroup is confusing. Any suggestions?


 On Feb. 11, 2015, 11:54 p.m., Bill Farner wrote:
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, 
  line 174
  https://reviews.apache.org/r/30890/diff/1/?file=861039#file861039line174
 
  Is this comment useful?  Ignoring the performance requirements, it 
  seems like a perfectly rational implementation.

Ignoring the performance requirements, it seems like a perfectly rational 
implementation.

It's an attempt to keep it as such (e.g. avoiding Optional and the like).


 On Feb. 11, 2015, 11:54 p.m., Bill Farner wrote:
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, 
  line 274
  https://reviews.apache.org/r/30890/diff/1/?file=861039#file861039line274
 
  Revert to former location?  Seems like noise in the diff

Split/merge side-effect. Fixed.


 On Feb. 11, 2015, 11:54 p.m., Bill Farner wrote:
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java, 
  line 115
  https://reviews.apache.org/r/30890/diff/1/?file=861040#file861040line115
 
  What's the thought process behind converting this to a `Set`?  The code 
  doesn't create duplicates, and you pick up the performance hit of a hash 
  table.
  
  I'm actually wondering if signatures should use `Iterable` all the way 
  out of this class.

The only consumer of this method is already converting everything to a set. It 
feels awkward to have Iterable in places where there should be no duplicates to 
start with. I am -1 on converting all signatures from Set to Iterable in this 
class. The interface contract should clearly state the expectations.


- Maxim


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


On Feb. 11, 2015, 9:38 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30890/
 ---
 
 (Updated Feb. 11, 2015, 9:38 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Bugs: AURORA-909
 https://issues.apache.org/jira/browse/AURORA-909
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Offer filtering for static vetoes. Part 2 of 4: Exposing Veto groups and 
 refactoring TaskVars.
 
 Will not apply cleanly: diffed with https://reviews.apache.org/r/30888/ as a 
 parent.
 
 Original RB: https://reviews.apache.org/r/28617/
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/TaskVars.java 
 f017cdd26ca40138a7e141f21613ed567314c399 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
 6a43bcd1719e8aa32fd3fcb7387d0318c3c0b804 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
 f06fdaeb92e154d0982bdabed5df93e7bcba9048 
   src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
 4e7efb3c1214c3d193afd61f162713490eb8effb 
   
 src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
  52ee7c1e3742d9315c7e7aaa77677121e1e9288d 
 
 Diff: https://reviews.apache.org/r/30890/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30890: Offer filtering for static vetoes. Part 2 of 4: Veto groups.

2015-02-12 Thread Maxim Khutornenko

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

(Updated Feb. 12, 2015, 6:05 p.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

Bill's comments.


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


Repository: aurora


Description
---

Offer filtering for static vetoes. Part 2 of 4: Exposing Veto groups and 
refactoring TaskVars.

Will not apply cleanly: diffed with https://reviews.apache.org/r/30888/ as a 
parent.

Original RB: https://reviews.apache.org/r/28617/


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/TaskVars.java 
f017cdd26ca40138a7e141f21613ed567314c399 
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
6a43bcd1719e8aa32fd3fcb7387d0318c3c0b804 
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
f06fdaeb92e154d0982bdabed5df93e7bcba9048 
  src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
4e7efb3c1214c3d193afd61f162713490eb8effb 
  
src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java 
52ee7c1e3742d9315c7e7aaa77677121e1e9288d 

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


Testing
---

./gradlew -Pq build


Thanks,

Maxim Khutornenko



Review Request 30942: Displaying blockIfNoPulseAfterMs in the UI

2015-02-12 Thread Maxim Khutornenko

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

Review request for Aurora and David McLaughlin.


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


Repository: aurora


Description
---

Displaying blockIfNoPulseAfterMs in the UI


Diffs
-

  src/main/resources/scheduler/assets/updateSettings.html 
5e48402561beb51efecd356f6ee44bdd1e465905 

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


Testing
---


Thanks,

Maxim Khutornenko



Re: Review Request 30890: Offer filtering for static vetoes. Part 2 of 4: Veto groups.

2015-02-12 Thread Aurora ReviewBot

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


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

 src.test.python.apache.aurora.client.cli.config
 .   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   
 .   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.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.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 Feb. 12, 2015, 6:05 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30890/
 ---
 
 (Updated Feb. 12, 2015, 6:05 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 

Re: Review Request 30942: Displaying blockIfNoPulseAfterMs in the UI

2015-02-12 Thread Aurora ReviewBot

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


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

However, it appears that it might lack test coverage.

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

- Aurora ReviewBot


On Feb. 12, 2015, 6:40 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30942/
 ---
 
 (Updated Feb. 12, 2015, 6:40 p.m.)
 
 
 Review request for Aurora and David McLaughlin.
 
 
 Bugs: AURORA-1088
 https://issues.apache.org/jira/browse/AURORA-1088
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Displaying blockIfNoPulseAfterMs in the UI
 
 
 Diffs
 -
 
   src/main/resources/scheduler/assets/updateSettings.html 
 5e48402561beb51efecd356f6ee44bdd1e465905 
 
 Diff: https://reviews.apache.org/r/30942/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30461: Adding pulse_interval_secs into client UpdateConfig.

2015-02-12 Thread Aurora ReviewBot

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

Ship it!


Master (994d669) 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 Feb. 12, 2015, 5:40 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30461/
 ---
 
 (Updated Feb. 12, 2015, 5:40 p.m.)
 
 
 Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner.
 
 
 Bugs: AURORA-1071
 https://issues.apache.org/jira/browse/AURORA-1071
 
 
 Repository: aurora
 
 
 Description
 ---
 
 - Adding pulse_interval_secs into client UpdateConfig and validating its range
 - Raising an error in client updater for pulse_interval_secs
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/api/updater.py 
 9f91de625f55514530a4f948d7ecdf7b5614b594 
   src/main/python/apache/aurora/client/api/updater_util.py 
 9d2e893a6ecff0fc48c7944575578443d41ced78 
   src/main/python/apache/aurora/config/schema/base.py 
 e4433d2d47668f59bce169359131284d361bad09 
   src/test/python/apache/aurora/client/api/test_updater.py 
 dd3f228c5062d388b4393aa4fd5b60a685bdb3a6 
   src/test/python/apache/aurora/client/api/test_updater_util.py 
 fe3ac49491ca710761632405ac09de0cc0d038a5 
 
 Diff: https://reviews.apache.org/r/30461/diff/
 
 
 Testing
 ---
 
 ./pants test.pytest src/test/python/apache/aurora/client::
 
 
 Thanks,
 
 Maxim Khutornenko
 




Review Request 30950: Add the option to make a non-hooked API.

2015-02-12 Thread Joshua Cohen

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

Review request for Aurora, David McLaughlin and Zameer Manji.


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


Repository: aurora


Description
---

Add the option to make a non-hooked API.


Diffs
-

  src/main/python/apache/aurora/client/cli/context.py 
f8b289938201da1edd7d6dde2c65124b80b58adb 
  src/main/python/apache/aurora/client/factory.py 
85a1398106a575c1b7759b904b32db6b07da7c1b 
  src/test/python/apache/aurora/client/BUILD 
c55adfe9825b77f418e41fa9a4ba43926bd991ed 
  src/test/python/apache/aurora/client/cli/BUILD 
7319962b285b449b275196eb0c4033e963579d8e 
  src/test/python/apache/aurora/client/cli/test_context.py PRE-CREATION 
  src/test/python/apache/aurora/client/cli/util.py 
12360c64a6c6f46d5b96f604c002b59d0f1a9b0e 
  src/test/python/apache/aurora/client/test_factory.py PRE-CREATION 
  src/test/python/apache/aurora/client/util.py PRE-CREATION 

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


Testing
---

./pants test.pytest --no-fast src/test/python/apache/aurora/client::


Thanks,

Joshua Cohen



Re: Review Request 30950: Add the option to make a non-hooked API.

2015-02-12 Thread Zameer Manji

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



src/test/python/apache/aurora/client/cli/util.py
https://reviews.apache.org/r/30950/#comment118308

Why don't you just use the imported values instead of declaring them here 
again?


- Zameer Manji


On Feb. 12, 2015, 1:59 p.m., Joshua Cohen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30950/
 ---
 
 (Updated Feb. 12, 2015, 1:59 p.m.)
 
 
 Review request for Aurora, David McLaughlin and Zameer Manji.
 
 
 Bugs: AURORA-1120
 https://issues.apache.org/jira/browse/AURORA-1120
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Add the option to make a non-hooked API.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/context.py 
 f8b289938201da1edd7d6dde2c65124b80b58adb 
   src/main/python/apache/aurora/client/factory.py 
 85a1398106a575c1b7759b904b32db6b07da7c1b 
   src/test/python/apache/aurora/client/BUILD 
 c55adfe9825b77f418e41fa9a4ba43926bd991ed 
   src/test/python/apache/aurora/client/cli/BUILD 
 7319962b285b449b275196eb0c4033e963579d8e 
   src/test/python/apache/aurora/client/cli/test_context.py PRE-CREATION 
   src/test/python/apache/aurora/client/cli/util.py 
 12360c64a6c6f46d5b96f604c002b59d0f1a9b0e 
   src/test/python/apache/aurora/client/test_factory.py PRE-CREATION 
   src/test/python/apache/aurora/client/util.py PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/30950/diff/
 
 
 Testing
 ---
 
 ./pants test.pytest --no-fast src/test/python/apache/aurora/client::
 
 
 Thanks,
 
 Joshua Cohen
 




Re: Review Request 30950: Add the option to make a non-hooked API.

2015-02-12 Thread Joshua Cohen


 On Feb. 12, 2015, 10:03 p.m., Zameer Manji wrote:
  src/test/python/apache/aurora/client/cli/util.py, line 313
  https://reviews.apache.org/r/30950/diff/1/?file=862321#file862321line313
 
  Why don't you just use the imported values instead of declaring them 
  here again?

Just trying to limit the scope of the changes. I didn't want to duplicate these 
values up at a higher level so I moved them, but I also don't want to change 
every test that references these values where they currently live.


- Joshua


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


On Feb. 12, 2015, 9:59 p.m., Joshua Cohen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30950/
 ---
 
 (Updated Feb. 12, 2015, 9:59 p.m.)
 
 
 Review request for Aurora, David McLaughlin and Zameer Manji.
 
 
 Bugs: AURORA-1120
 https://issues.apache.org/jira/browse/AURORA-1120
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Add the option to make a non-hooked API.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/context.py 
 f8b289938201da1edd7d6dde2c65124b80b58adb 
   src/main/python/apache/aurora/client/factory.py 
 85a1398106a575c1b7759b904b32db6b07da7c1b 
   src/test/python/apache/aurora/client/BUILD 
 c55adfe9825b77f418e41fa9a4ba43926bd991ed 
   src/test/python/apache/aurora/client/cli/BUILD 
 7319962b285b449b275196eb0c4033e963579d8e 
   src/test/python/apache/aurora/client/cli/test_context.py PRE-CREATION 
   src/test/python/apache/aurora/client/cli/util.py 
 12360c64a6c6f46d5b96f604c002b59d0f1a9b0e 
   src/test/python/apache/aurora/client/test_factory.py PRE-CREATION 
   src/test/python/apache/aurora/client/util.py PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/30950/diff/
 
 
 Testing
 ---
 
 ./pants test.pytest --no-fast src/test/python/apache/aurora/client::
 
 
 Thanks,
 
 Joshua Cohen
 




Re: Review Request 30950: Add the option to make a non-hooked API.

2015-02-12 Thread Zameer Manji

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

Ship it!


Ship It!

- Zameer Manji


On Feb. 12, 2015, 1:59 p.m., Joshua Cohen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30950/
 ---
 
 (Updated Feb. 12, 2015, 1:59 p.m.)
 
 
 Review request for Aurora, David McLaughlin and Zameer Manji.
 
 
 Bugs: AURORA-1120
 https://issues.apache.org/jira/browse/AURORA-1120
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Add the option to make a non-hooked API.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/context.py 
 f8b289938201da1edd7d6dde2c65124b80b58adb 
   src/main/python/apache/aurora/client/factory.py 
 85a1398106a575c1b7759b904b32db6b07da7c1b 
   src/test/python/apache/aurora/client/BUILD 
 c55adfe9825b77f418e41fa9a4ba43926bd991ed 
   src/test/python/apache/aurora/client/cli/BUILD 
 7319962b285b449b275196eb0c4033e963579d8e 
   src/test/python/apache/aurora/client/cli/test_context.py PRE-CREATION 
   src/test/python/apache/aurora/client/cli/util.py 
 12360c64a6c6f46d5b96f604c002b59d0f1a9b0e 
   src/test/python/apache/aurora/client/test_factory.py PRE-CREATION 
   src/test/python/apache/aurora/client/util.py PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/30950/diff/
 
 
 Testing
 ---
 
 ./pants test.pytest --no-fast src/test/python/apache/aurora/client::
 
 
 Thanks,
 
 Joshua Cohen
 




Re: Review Request 30950: Add the option to make a non-hooked API.

2015-02-12 Thread Aurora ReviewBot

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

Ship it!


Master (ab8fae2) 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 Feb. 12, 2015, 9:59 p.m., Joshua Cohen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30950/
 ---
 
 (Updated Feb. 12, 2015, 9:59 p.m.)
 
 
 Review request for Aurora, David McLaughlin and Zameer Manji.
 
 
 Bugs: AURORA-1120
 https://issues.apache.org/jira/browse/AURORA-1120
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Add the option to make a non-hooked API.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/context.py 
 f8b289938201da1edd7d6dde2c65124b80b58adb 
   src/main/python/apache/aurora/client/factory.py 
 85a1398106a575c1b7759b904b32db6b07da7c1b 
   src/test/python/apache/aurora/client/BUILD 
 c55adfe9825b77f418e41fa9a4ba43926bd991ed 
   src/test/python/apache/aurora/client/cli/BUILD 
 7319962b285b449b275196eb0c4033e963579d8e 
   src/test/python/apache/aurora/client/cli/test_context.py PRE-CREATION 
   src/test/python/apache/aurora/client/cli/util.py 
 12360c64a6c6f46d5b96f604c002b59d0f1a9b0e 
   src/test/python/apache/aurora/client/test_factory.py PRE-CREATION 
   src/test/python/apache/aurora/client/util.py PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/30950/diff/
 
 
 Testing
 ---
 
 ./pants test.pytest --no-fast src/test/python/apache/aurora/client::
 
 
 Thanks,
 
 Joshua Cohen
 




Re: Review Request 30942: Displaying blockIfNoPulseAfterMs in the UI

2015-02-12 Thread David McLaughlin

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

Ship it!


Ship It!

- David McLaughlin


On Feb. 12, 2015, 6:40 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30942/
 ---
 
 (Updated Feb. 12, 2015, 6:40 p.m.)
 
 
 Review request for Aurora and David McLaughlin.
 
 
 Bugs: AURORA-1088
 https://issues.apache.org/jira/browse/AURORA-1088
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Displaying blockIfNoPulseAfterMs in the UI
 
 
 Diffs
 -
 
   src/main/resources/scheduler/assets/updateSettings.html 
 5e48402561beb51efecd356f6ee44bdd1e465905 
 
 Diff: https://reviews.apache.org/r/30942/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30890: Offer filtering for static vetoes. Part 2 of 4: Veto groups.

2015-02-12 Thread Maxim Khutornenko


 On Feb. 13, 2015, 1:17 a.m., Kevin Sweeney wrote:
  src/main/java/org/apache/aurora/scheduler/TaskVars.java, line 78
  https://reviews.apache.org/r/30890/diff/3/?file=862710#file862710line78
 
  Can you add a getCounterName() abstract method to the VetoGroup enum? 
  That will make the compiler guarantee that this mapping is complete.

We have discussed this before in https://reviews.apache.org/r/28617/. I still 
don't feel VetoGroup should be aware of any counters. I have added a 
comprehensive test instead that will break when a new value is added here.


 On Feb. 13, 2015, 1:17 a.m., Kevin Sweeney wrote:
  src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java, line 358
  https://reviews.apache.org/r/30890/diff/3/?file=862713#file862713line358
 
  (Consider moving this test to compile-time with the above approach)

Answered above.


 On Feb. 13, 2015, 1:17 a.m., Kevin Sweeney wrote:
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, 
  line 181
  https://reviews.apache.org/r/30890/diff/3/?file=862711#file862711line181
 
  Can you use != here since this is an enum?

Pmd does not like it: Use equals() to compare object references.


- Maxim


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


On Feb. 13, 2015, 12:58 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30890/
 ---
 
 (Updated Feb. 13, 2015, 12:58 a.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Bugs: AURORA-909
 https://issues.apache.org/jira/browse/AURORA-909
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Offer filtering for static vetoes. Part 2 of 4: Exposing Veto groups and 
 refactoring TaskVars.
 
 Original RB: https://reviews.apache.org/r/28617/
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/TaskVars.java 
 f017cdd26ca40138a7e141f21613ed567314c399 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
 6a43bcd1719e8aa32fd3fcb7387d0318c3c0b804 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
 f06fdaeb92e154d0982bdabed5df93e7bcba9048 
   src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
 4e7efb3c1214c3d193afd61f162713490eb8effb 
   
 src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
  52ee7c1e3742d9315c7e7aaa77677121e1e9288d 
 
 Diff: https://reviews.apache.org/r/30890/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30950: Add the option to make a non-hooked API.

2015-02-12 Thread David McLaughlin

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

Ship it!


Ship It!

- David McLaughlin


On Feb. 12, 2015, 9:59 p.m., Joshua Cohen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30950/
 ---
 
 (Updated Feb. 12, 2015, 9:59 p.m.)
 
 
 Review request for Aurora, David McLaughlin and Zameer Manji.
 
 
 Bugs: AURORA-1120
 https://issues.apache.org/jira/browse/AURORA-1120
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Add the option to make a non-hooked API.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/context.py 
 f8b289938201da1edd7d6dde2c65124b80b58adb 
   src/main/python/apache/aurora/client/factory.py 
 85a1398106a575c1b7759b904b32db6b07da7c1b 
   src/test/python/apache/aurora/client/BUILD 
 c55adfe9825b77f418e41fa9a4ba43926bd991ed 
   src/test/python/apache/aurora/client/cli/BUILD 
 7319962b285b449b275196eb0c4033e963579d8e 
   src/test/python/apache/aurora/client/cli/test_context.py PRE-CREATION 
   src/test/python/apache/aurora/client/cli/util.py 
 12360c64a6c6f46d5b96f604c002b59d0f1a9b0e 
   src/test/python/apache/aurora/client/test_factory.py PRE-CREATION 
   src/test/python/apache/aurora/client/util.py PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/30950/diff/
 
 
 Testing
 ---
 
 ./pants test.pytest --no-fast src/test/python/apache/aurora/client::
 
 
 Thanks,
 
 Joshua Cohen
 




Re: Review Request 30888: Offer filtering for static vetoes. Part 1 of 4: TaskAssigner.

2015-02-12 Thread Maxim Khutornenko

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

(Updated Feb. 12, 2015, 11:44 p.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

Making pmd happy.


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


Repository: aurora


Description
---

Offer filtering for static vetoes. Part 1 of 4: TaskAssigner changes to return 
a new result object.

Original RB: https://reviews.apache.org/r/28617/


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 
f66383830140e5eaba436f35ebb5192eee65947a 
  src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
ce47ff152e303fd2116bc3b9e91c0c1a8f76f258 
  src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
e1c29747c9854cf75bf63f6f085cf40ca68989af 
  src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 
4cf602ad32b972c18eb5a81e9b2f59c67859bdb2 
  src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
5647349854a5e04de749c4d809684a0066d4da06 
  src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
6cc13231560996b144101eba36577f49017aba06 
  src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
411a55a8d85f60bb2703468f2d69b64b2736eee4 

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


Testing
---

./gradlew -Pq build


Thanks,

Maxim Khutornenko



Re: Review Request 30913: Adding UPDATE_COORDINATOR role access into pause/resume/abort RPCs

2015-02-12 Thread David McLaughlin

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

Ship it!


Ship It!

- David McLaughlin


On Feb. 12, 2015, 2:47 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30913/
 ---
 
 (Updated Feb. 12, 2015, 2:47 a.m.)
 
 
 Review request for Aurora, David McLaughlin and Bill Farner.
 
 
 Bugs: AURORA-1119
 https://issues.apache.org/jira/browse/AURORA-1119
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Adding UPDATE_COORDINATOR role access into pause/resume/abort RPCs
 
 
 Diffs
 -
 
   
 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
  2a9d36ab2c01960dc5384fc3ed90df4e11c0b12a 
   
 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
  ee329496adf051de717d42c60410c0469f7e90da 
 
 Diff: https://reviews.apache.org/r/30913/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30890: Offer filtering for static vetoes. Part 2 of 4: Veto groups.

2015-02-12 Thread Aurora ReviewBot

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

Ship it!


Master (b62ec61) 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 Feb. 12, 2015, 6:05 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30890/
 ---
 
 (Updated Feb. 12, 2015, 6:05 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Bugs: AURORA-909
 https://issues.apache.org/jira/browse/AURORA-909
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Offer filtering for static vetoes. Part 2 of 4: Exposing Veto groups and 
 refactoring TaskVars.
 
 Will not apply cleanly: diffed with https://reviews.apache.org/r/30888/ as a 
 parent.
 
 Original RB: https://reviews.apache.org/r/28617/
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/TaskVars.java 
 f017cdd26ca40138a7e141f21613ed567314c399 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
 6a43bcd1719e8aa32fd3fcb7387d0318c3c0b804 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
 f06fdaeb92e154d0982bdabed5df93e7bcba9048 
   src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
 4e7efb3c1214c3d193afd61f162713490eb8effb 
   
 src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
  52ee7c1e3742d9315c7e7aaa77677121e1e9288d 
 
 Diff: https://reviews.apache.org/r/30890/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30888: Offer filtering for static vetoes. Part 1 of 4: TaskAssigner.

2015-02-12 Thread Aurora ReviewBot

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

Ship it!


Master (b62ec61) 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 Feb. 12, 2015, 11:44 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30888/
 ---
 
 (Updated Feb. 12, 2015, 11:44 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Bugs: AURORA-909
 https://issues.apache.org/jira/browse/AURORA-909
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Offer filtering for static vetoes. Part 1 of 4: TaskAssigner changes to 
 return a new result object.
 
 Original RB: https://reviews.apache.org/r/28617/
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 
 f66383830140e5eaba436f35ebb5192eee65947a 
   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
 ce47ff152e303fd2116bc3b9e91c0c1a8f76f258 
   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
 e1c29747c9854cf75bf63f6f085cf40ca68989af 
   src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 
 4cf602ad32b972c18eb5a81e9b2f59c67859bdb2 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
 5647349854a5e04de749c4d809684a0066d4da06 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
 6cc13231560996b144101eba36577f49017aba06 
   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
 411a55a8d85f60bb2703468f2d69b64b2736eee4 
 
 Diff: https://reviews.apache.org/r/30888/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30890: Offer filtering for static vetoes. Part 2 of 4: Veto groups.

2015-02-12 Thread Maxim Khutornenko

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

(Updated Feb. 13, 2015, 12:58 a.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

Rebasing.


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


Repository: aurora


Description
---

Offer filtering for static vetoes. Part 2 of 4: Exposing Veto groups and 
refactoring TaskVars.

Will not apply cleanly: diffed with https://reviews.apache.org/r/30888/ as a 
parent.

Original RB: https://reviews.apache.org/r/28617/


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/TaskVars.java 
f017cdd26ca40138a7e141f21613ed567314c399 
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
6a43bcd1719e8aa32fd3fcb7387d0318c3c0b804 
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
f06fdaeb92e154d0982bdabed5df93e7bcba9048 
  src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
4e7efb3c1214c3d193afd61f162713490eb8effb 
  
src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java 
52ee7c1e3742d9315c7e7aaa77677121e1e9288d 

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


Testing
---

./gradlew -Pq build


Thanks,

Maxim Khutornenko



Re: Review Request 30957: Extract ReadOnlyScheduler to its own implementation class

2015-02-12 Thread Aurora ReviewBot

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


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

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

- Aurora ReviewBot


On Feb. 13, 2015, 12:13 a.m., Kevin Sweeney wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30957/
 ---
 
 (Updated Feb. 13, 2015, 12:13 a.m.)
 
 
 Review request for Aurora, Joshua Cohen and Bill Farner.
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Extract ReadOnlyScheduler to its own implementation class and delegate from 
 the existing SchedulerThriftInterface to it.
 
 This enables serving ReadOnlyScheduler from a separate HTTP endpoint (e.g. an 
 unauthenticated one).
 
 This is almost a pure tool-driven refactor-rename change. Also renames Util 
 to Responses for improved ergonomics.
 
 To boost confidence in this change tests have been left in place. I will 
 follow-up with a subsequent review to split out ReadOnlySchedulerImplTest.
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
 53ea03bac3784baebc630ad1ce235d263985cafe 
   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
 PRE-CREATION 
   
 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
  8c19f3b08135eb5f3098591ebf9931b42a086318 
   src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
 7b28eb87767d7cd19e9365e3287f3e943d87dea5 
   src/main/java/org/apache/aurora/scheduler/thrift/Util.java 
 55242d18e08ea5cb6dd297bd7f18744d952580b3 
   
 src/main/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptor.java
  e176a0df3141dcb088e05203cca4de3f0d3feea7 
   
 src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 
 cad63c77e8144bb64c6b2acaf1b9199be13e4145 
   
 src/main/java/org/apache/aurora/scheduler/thrift/aop/UserCapabilityInterceptor.java
  5e6577d64b1037c69c3a952008240dcafe3b9f94 
   
 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
  03d1fba76c23570c2c4102a48daf5ce035ecaaa3 
 
 Diff: https://reviews.apache.org/r/30957/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Kevin Sweeney
 




Review Request 30957: Extract ReadOnlyScheduler to its own implementation class

2015-02-12 Thread Kevin Sweeney

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

Review request for Aurora, Joshua Cohen and Bill Farner.


Repository: aurora


Description
---

Extract ReadOnlyScheduler to its own implementation class and delegate from the 
existing SchedulerThriftInterface to it.

This enables serving ReadOnlyScheduler from a separate HTTP endpoint (e.g. an 
unauthenticated one).

This is almost a pure tool-driven refactor-rename change. Also renames Util to 
Responses for improved ergonomics.

To boost confidence in this change tests have been left in place. I will 
follow-up with a subsequent review to split out ReadOnlySchedulerImplTest.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
53ea03bac3784baebc630ad1ce235d263985cafe 
  src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
PRE-CREATION 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
8c19f3b08135eb5f3098591ebf9931b42a086318 
  src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
7b28eb87767d7cd19e9365e3287f3e943d87dea5 
  src/main/java/org/apache/aurora/scheduler/thrift/Util.java 
55242d18e08ea5cb6dd297bd7f18744d952580b3 
  
src/main/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptor.java
 e176a0df3141dcb088e05203cca4de3f0d3feea7 
  src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 
cad63c77e8144bb64c6b2acaf1b9199be13e4145 
  
src/main/java/org/apache/aurora/scheduler/thrift/aop/UserCapabilityInterceptor.java
 5e6577d64b1037c69c3a952008240dcafe3b9f94 
  
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 

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


Testing
---

./gradlew -Pq build


Thanks,

Kevin Sweeney



Re: Review Request 30890: Offer filtering for static vetoes. Part 2 of 4: Veto groups.

2015-02-12 Thread Maxim Khutornenko

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

(Updated Feb. 13, 2015, 12:58 a.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

Updated description.


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


Repository: aurora


Description (updated)
---

Offer filtering for static vetoes. Part 2 of 4: Exposing Veto groups and 
refactoring TaskVars.

Original RB: https://reviews.apache.org/r/28617/


Diffs
-

  src/main/java/org/apache/aurora/scheduler/TaskVars.java 
f017cdd26ca40138a7e141f21613ed567314c399 
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
6a43bcd1719e8aa32fd3fcb7387d0318c3c0b804 
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
f06fdaeb92e154d0982bdabed5df93e7bcba9048 
  src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
4e7efb3c1214c3d193afd61f162713490eb8effb 
  
src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java 
52ee7c1e3742d9315c7e7aaa77677121e1e9288d 

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


Testing
---

./gradlew -Pq build


Thanks,

Maxim Khutornenko



Re: Review Request 30890: Offer filtering for static vetoes. Part 2 of 4: Veto groups.

2015-02-12 Thread Kevin Sweeney

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

Ship it!



src/main/java/org/apache/aurora/scheduler/TaskVars.java
https://reviews.apache.org/r/30890/#comment118384

Can you add a getCounterName() abstract method to the VetoGroup enum? That 
will make the compiler guarantee that this mapping is complete.



src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java
https://reviews.apache.org/r/30890/#comment118385

Can you use != here since this is an enum?



src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java
https://reviews.apache.org/r/30890/#comment118386

(Consider moving this test to compile-time with the above approach)


- Kevin Sweeney


On Feb. 12, 2015, 4:58 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30890/
 ---
 
 (Updated Feb. 12, 2015, 4:58 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Bugs: AURORA-909
 https://issues.apache.org/jira/browse/AURORA-909
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Offer filtering for static vetoes. Part 2 of 4: Exposing Veto groups and 
 refactoring TaskVars.
 
 Original RB: https://reviews.apache.org/r/28617/
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/TaskVars.java 
 f017cdd26ca40138a7e141f21613ed567314c399 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
 6a43bcd1719e8aa32fd3fcb7387d0318c3c0b804 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
 f06fdaeb92e154d0982bdabed5df93e7bcba9048 
   src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
 4e7efb3c1214c3d193afd61f162713490eb8effb 
   
 src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
  52ee7c1e3742d9315c7e7aaa77677121e1e9288d 
 
 Diff: https://reviews.apache.org/r/30890/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.

2015-02-12 Thread Aurora ReviewBot

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


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

import org.apache.aurora.scheduler.filter.SchedulingFilter.VetoGroup;
  ^
  symbol:   class VetoGroup
  location: interface SchedulingFilter
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/state/TaskAssigner.java:33:
 error: cannot find symbol
import org.apache.aurora.scheduler.filter.SchedulingFilter.VetoGroup;
  ^
  symbol:   class VetoGroup
  location: interface SchedulingFilter
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java:117:
 error: cannot find symbol
  VetoGroup vetoGroup = Veto.identifyGroup(vetoes);
  ^
  symbol:   class VetoGroup
  location: class Assignment
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java:117:
 error: cannot find symbol
  VetoGroup vetoGroup = Veto.identifyGroup(vetoes);
^
  symbol:   method identifyGroup(SetVeto)
  location: class Veto
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java:118:
 error: cannot find symbol
  if (vetoGroup == VetoGroup.STATIC) {
   ^
  symbol:   variable VetoGroup
  location: class Assignment
4 errors
 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 17.762 secs


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

- Aurora ReviewBot


On Feb. 13, 2015, 1:04 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30891/
 ---
 
 (Updated Feb. 13, 2015, 1:04 a.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Bugs: AURORA-909
 https://issues.apache.org/jira/browse/AURORA-909
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Offer filtering for static vetoes. Part 3 of 4: Filtering out statically 
 banned offers.
 
 Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a 
 parent.
 
 Original RB: https://reviews.apache.org/r/28617/
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 
 332338b7bef7d622333f8ea6508c4f5970b8e7c4 
   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
 ced3bdef1731354aedc82bb12f45ba6f040e1ab7 
   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 
   src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 
 2b5dc4902f57e508d76f5e16997ae09d04464220 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
 d0e11932e8b5ba1393279137c8465a308e1d6bf5 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
 74e31334bc139c47eb8b0beee46ee7bad62a2f80 
 
 Diff: https://reviews.apache.org/r/30891/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30890: Offer filtering for static vetoes. Part 2 of 4: Veto groups.

2015-02-12 Thread Aurora ReviewBot

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

Ship it!


Master (f3473a3) 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 Feb. 13, 2015, 12:58 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30890/
 ---
 
 (Updated Feb. 13, 2015, 12:58 a.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Bugs: AURORA-909
 https://issues.apache.org/jira/browse/AURORA-909
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Offer filtering for static vetoes. Part 2 of 4: Exposing Veto groups and 
 refactoring TaskVars.
 
 Original RB: https://reviews.apache.org/r/28617/
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/TaskVars.java 
 f017cdd26ca40138a7e141f21613ed567314c399 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
 6a43bcd1719e8aa32fd3fcb7387d0318c3c0b804 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
 f06fdaeb92e154d0982bdabed5df93e7bcba9048 
   src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
 4e7efb3c1214c3d193afd61f162713490eb8effb 
   
 src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
  52ee7c1e3742d9315c7e7aaa77677121e1e9288d 
 
 Diff: https://reviews.apache.org/r/30890/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30890: Offer filtering for static vetoes. Part 2 of 4: Veto groups.

2015-02-12 Thread Maxim Khutornenko

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


@ReviewBot retry

- Maxim Khutornenko


On Feb. 12, 2015, 6:05 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30890/
 ---
 
 (Updated Feb. 12, 2015, 6:05 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Bugs: AURORA-909
 https://issues.apache.org/jira/browse/AURORA-909
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Offer filtering for static vetoes. Part 2 of 4: Exposing Veto groups and 
 refactoring TaskVars.
 
 Will not apply cleanly: diffed with https://reviews.apache.org/r/30888/ as a 
 parent.
 
 Original RB: https://reviews.apache.org/r/28617/
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/TaskVars.java 
 f017cdd26ca40138a7e141f21613ed567314c399 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
 6a43bcd1719e8aa32fd3fcb7387d0318c3c0b804 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
 f06fdaeb92e154d0982bdabed5df93e7bcba9048 
   src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
 4e7efb3c1214c3d193afd61f162713490eb8effb 
   
 src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
  52ee7c1e3742d9315c7e7aaa77677121e1e9288d 
 
 Diff: https://reviews.apache.org/r/30890/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 29117: Adding thrift API changes document.

2015-02-12 Thread Maxim Khutornenko

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

(Updated Feb. 13, 2015, 12:35 a.m.)


Review request for Aurora and Bill Farner.


Changes
---

Rebased.


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


Repository: aurora


Description
---

This is a first stab at documenting thrift deprecation. Any 
suggestions/comments are welcome.


Diffs (updated)
-

  docs/developing-aurora-client.md e002d908ce07020513fa06c7e70c41f9e2f55b1d 
  docs/developing-aurora-scheduler.md 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f 
  docs/thrift-deprecation.md PRE-CREATION 

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


Testing
---

https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md


Thanks,

Maxim Khutornenko



Re: Review Request 29117: Adding thrift API changes document.

2015-02-12 Thread David McLaughlin

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



docs/thrift-deprecation.md
https://reviews.apache.org/r/29117/#comment118378

Sorry I missed this first review, but AFAIK this isn't true for dynamic 
languages (incl. our own scheduler UI)?


- David McLaughlin


On Feb. 13, 2015, 12:35 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29117/
 ---
 
 (Updated Feb. 13, 2015, 12:35 a.m.)
 
 
 Review request for Aurora and Bill Farner.
 
 
 Bugs: AURORA-973
 https://issues.apache.org/jira/browse/AURORA-973
 
 
 Repository: aurora
 
 
 Description
 ---
 
 This is a first stab at documenting thrift deprecation. Any 
 suggestions/comments are welcome.
 
 
 Diffs
 -
 
   docs/developing-aurora-client.md e002d908ce07020513fa06c7e70c41f9e2f55b1d 
   docs/developing-aurora-scheduler.md 
 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f 
   docs/thrift-deprecation.md PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29117/diff/
 
 
 Testing
 ---
 
 https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 29117: Adding thrift API changes document.

2015-02-12 Thread Maxim Khutornenko


 On Feb. 13, 2015, 12:41 a.m., David McLaughlin wrote:
  docs/thrift-deprecation.md, line 20
  https://reviews.apache.org/r/29117/diff/4/?file=862696#file862696line20
 
  Sorry I missed this first review, but AFAIK this isn't true for dynamic 
  languages (incl. our own scheduler UI)?

Can you elaborate? The above statement assumes renaming the field throughout 
the codebase (including java, python and JS). Current +- 1 versions of client 
will still be able to communicate with the scheduler. UI ships with the 
scheduler and thus will not have the renaming problelm. What am I missing?


- Maxim


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


On Feb. 13, 2015, 12:35 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29117/
 ---
 
 (Updated Feb. 13, 2015, 12:35 a.m.)
 
 
 Review request for Aurora and Bill Farner.
 
 
 Bugs: AURORA-973
 https://issues.apache.org/jira/browse/AURORA-973
 
 
 Repository: aurora
 
 
 Description
 ---
 
 This is a first stab at documenting thrift deprecation. Any 
 suggestions/comments are welcome.
 
 
 Diffs
 -
 
   docs/developing-aurora-client.md e002d908ce07020513fa06c7e70c41f9e2f55b1d 
   docs/developing-aurora-scheduler.md 
 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f 
   docs/thrift-deprecation.md PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29117/diff/
 
 
 Testing
 ---
 
 https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30957: Extract ReadOnlyScheduler to its own implementation class

2015-02-12 Thread Kevin Sweeney

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

(Updated Feb. 12, 2015, 4:55 p.m.)


Review request for Aurora, Joshua Cohen and Bill Farner.


Changes
---

fix bad rebase.


Repository: aurora


Description
---

Extract ReadOnlyScheduler to its own implementation class and delegate from the 
existing SchedulerThriftInterface to it.

This enables serving ReadOnlyScheduler from a separate HTTP endpoint (e.g. an 
unauthenticated one).

This is almost a pure tool-driven refactor-rename change. Also renames Util to 
Responses for improved ergonomics.

To boost confidence in this change tests have been left in place. I will 
follow-up with a subsequent review to split out ReadOnlySchedulerImplTest.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
53ea03bac3784baebc630ad1ce235d263985cafe 
  src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
PRE-CREATION 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
7014d598d41f007b0dee0b2db1aa2d4cdd592be6 
  src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
7b28eb87767d7cd19e9365e3287f3e943d87dea5 
  src/main/java/org/apache/aurora/scheduler/thrift/Util.java 
55242d18e08ea5cb6dd297bd7f18744d952580b3 
  
src/main/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptor.java
 e176a0df3141dcb088e05203cca4de3f0d3feea7 
  src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 
cad63c77e8144bb64c6b2acaf1b9199be13e4145 
  
src/main/java/org/apache/aurora/scheduler/thrift/aop/UserCapabilityInterceptor.java
 5e6577d64b1037c69c3a952008240dcafe3b9f94 
  
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 491aa34d76a6f3fe91188bbc73a2cc7464f3644b 

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


Testing
---

./gradlew -Pq build


Thanks,

Kevin Sweeney



Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.

2015-02-12 Thread Maxim Khutornenko

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

(Updated Feb. 13, 2015, 1:04 a.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

Rebased.


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


Repository: aurora


Description
---

Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned 
offers.

Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a 
parent.

Original RB: https://reviews.apache.org/r/28617/


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 
332338b7bef7d622333f8ea6508c4f5970b8e7c4 
  src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
ced3bdef1731354aedc82bb12f45ba6f040e1ab7 
  src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 
  src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 
2b5dc4902f57e508d76f5e16997ae09d04464220 
  src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
d0e11932e8b5ba1393279137c8465a308e1d6bf5 
  src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
74e31334bc139c47eb8b0beee46ee7bad62a2f80 

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


Testing
---

./gradlew -Pq build


Thanks,

Maxim Khutornenko



Re: Review Request 30890: Offer filtering for static vetoes. Part 2 of 4: Veto groups.

2015-02-12 Thread Maxim Khutornenko


 On Feb. 11, 2015, 11:54 p.m., Bill Farner wrote:
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, 
  line 34
  https://reviews.apache.org/r/30890/diff/1/?file=861039#file861039line34
 
  I feel like `VetoType` still applies and is actually a better name.  
  It's odd for an enum value to be considered a 'group'.  (When i hear group 
  i think collection.)
 
 Maxim Khutornenko wrote:
 VetoType is used to describe the particular type of veto applied. This 
 enum denotes similar typed Veto groups. I am fine changing it to something 
 else if VetoGroup is confusing. Any suggestions?
 
 Bill Farner wrote:
 My mistake, the diff convinced me that `VetoType` was renamed, but i see 
 that it still exists.  How about `VetoReason` and s/`EMPTY`/`NONE`/?  Reads a 
 bit more naturally from the calling side, but i don't feel strongly on that.

We already have a veto reason to describe why the Veto was issued. This really 
feels like a group definition rather than reason. I'd like to keep it if no 
strong objection.


 On Feb. 11, 2015, 11:54 p.m., Bill Farner wrote:
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java, 
  line 115
  https://reviews.apache.org/r/30890/diff/1/?file=861040#file861040line115
 
  What's the thought process behind converting this to a `Set`?  The code 
  doesn't create duplicates, and you pick up the performance hit of a hash 
  table.
  
  I'm actually wondering if signatures should use `Iterable` all the way 
  out of this class.
 
 Maxim Khutornenko wrote:
 The only consumer of this method is already converting everything to a 
 set. It feels awkward to have Iterable in places where there should be no 
 duplicates to start with. I am -1 on converting all signatures from Set to 
 Iterable in this class. The interface contract should clearly state the 
 expectations.
 
 Bill Farner wrote:
 I generally agree that `Set` should be preferred to communicate 
 uniqueness to callers.  However, in the same vein as the performance 
 sensitivity advised above, constructing a hash table non-negligible 
 performance hit.  In case i was unclear, i suggest you change the 
 `SchedulingFilter` signature as well, and avoid ever turning the result into 
 a `Set`.
 
 How would you feel about peeling off a ticket to explore this?  As i 
 follow the result, it looks like it could ripple fairly wide and distract 
 from this review.  I might have a chance to pick this up next sprint.

Created AURORA-1121, though I still don't think the gain is meaningful here.


- Maxim


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


On Feb. 12, 2015, 6:05 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30890/
 ---
 
 (Updated Feb. 12, 2015, 6:05 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Bugs: AURORA-909
 https://issues.apache.org/jira/browse/AURORA-909
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Offer filtering for static vetoes. Part 2 of 4: Exposing Veto groups and 
 refactoring TaskVars.
 
 Will not apply cleanly: diffed with https://reviews.apache.org/r/30888/ as a 
 parent.
 
 Original RB: https://reviews.apache.org/r/28617/
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/TaskVars.java 
 f017cdd26ca40138a7e141f21613ed567314c399 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
 6a43bcd1719e8aa32fd3fcb7387d0318c3c0b804 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
 f06fdaeb92e154d0982bdabed5df93e7bcba9048 
   src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
 4e7efb3c1214c3d193afd61f162713490eb8effb 
   
 src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
  52ee7c1e3742d9315c7e7aaa77677121e1e9288d 
 
 Diff: https://reviews.apache.org/r/30890/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30913: Adding UPDATE_COORDINATOR role access into pause/resume/abort RPCs

2015-02-12 Thread Bill Farner

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

Ship it!


Ship It!

- Bill Farner


On Feb. 12, 2015, 2:47 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30913/
 ---
 
 (Updated Feb. 12, 2015, 2:47 a.m.)
 
 
 Review request for Aurora, David McLaughlin and Bill Farner.
 
 
 Bugs: AURORA-1119
 https://issues.apache.org/jira/browse/AURORA-1119
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Adding UPDATE_COORDINATOR role access into pause/resume/abort RPCs
 
 
 Diffs
 -
 
   
 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
  2a9d36ab2c01960dc5384fc3ed90df4e11c0b12a 
   
 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
  ee329496adf051de717d42c60410c0469f7e90da 
 
 Diff: https://reviews.apache.org/r/30913/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30888: Offer filtering for static vetoes. Part 1 of 4: TaskAssigner.

2015-02-12 Thread Kevin Sweeney

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

Ship it!


Ship It!

- Kevin Sweeney


On Feb. 11, 2015, 2:52 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30888/
 ---
 
 (Updated Feb. 11, 2015, 2:52 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Bugs: AURORA-909
 https://issues.apache.org/jira/browse/AURORA-909
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Offer filtering for static vetoes. Part 1 of 4: TaskAssigner changes to 
 return a new result object.
 
 Original RB: https://reviews.apache.org/r/28617/
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 
 f66383830140e5eaba436f35ebb5192eee65947a 
   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
 ce47ff152e303fd2116bc3b9e91c0c1a8f76f258 
   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
 e1c29747c9854cf75bf63f6f085cf40ca68989af 
   src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 
 4cf602ad32b972c18eb5a81e9b2f59c67859bdb2 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
 5647349854a5e04de749c4d809684a0066d4da06 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
 6cc13231560996b144101eba36577f49017aba06 
   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
 411a55a8d85f60bb2703468f2d69b64b2736eee4 
 
 Diff: https://reviews.apache.org/r/30888/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30957: Extract ReadOnlyScheduler to its own implementation class

2015-02-12 Thread Aurora ReviewBot

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


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

 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   
 .   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.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 Feb. 13, 2015, 12:55 a.m., Kevin Sweeney wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30957/
 ---
 
 (Updated Feb. 13, 2015, 12:55 a.m.)
 
 
 Review request for Aurora, Joshua Cohen and Bill Farner.
 
 
 

Re: Review Request 30915: Increase findbugs heap size.

2015-02-12 Thread Bill Farner

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

Ship it!


Thanks.

Findbugs also acknoewledges that this is a commonly necessary:

 FindBugs generally requires a large amount of memory. For a very large 
 project, using 1500 megabytes is not unusual.

http://findbugs.sourceforge.net/manual/running.html

- Bill Farner


On Feb. 12, 2015, 2:57 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30915/
 ---
 
 (Updated Feb. 12, 2015, 2:57 a.m.)
 
 
 Review request for Aurora and Bill Farner.
 
 
 Repository: aurora
 
 
 Description
 ---
 
 I am no longer able to successfully run gradle as findbugs routinely OOMs. 
 Raising the max heap size to 1g (it's hovering around 450MB before failing).
 
 
 Diffs
 -
 
   build.gradle 32302ec5b2b9546d5420836b700c07f43875e90d 
 
 Diff: https://reviews.apache.org/r/30915/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30957: Extract ReadOnlyScheduler to its own implementation class

2015-02-12 Thread Kevin Sweeney

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

(Updated Feb. 12, 2015, 4:33 p.m.)


Review request for Aurora, Joshua Cohen and Bill Farner.


Changes
---

rebase


Repository: aurora


Description
---

Extract ReadOnlyScheduler to its own implementation class and delegate from the 
existing SchedulerThriftInterface to it.

This enables serving ReadOnlyScheduler from a separate HTTP endpoint (e.g. an 
unauthenticated one).

This is almost a pure tool-driven refactor-rename change. Also renames Util to 
Responses for improved ergonomics.

To boost confidence in this change tests have been left in place. I will 
follow-up with a subsequent review to split out ReadOnlySchedulerImplTest.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
53ea03bac3784baebc630ad1ce235d263985cafe 
  src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
PRE-CREATION 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
7014d598d41f007b0dee0b2db1aa2d4cdd592be6 
  src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
7b28eb87767d7cd19e9365e3287f3e943d87dea5 
  src/main/java/org/apache/aurora/scheduler/thrift/Util.java 
55242d18e08ea5cb6dd297bd7f18744d952580b3 
  
src/main/java/org/apache/aurora/scheduler/thrift/aop/FeatureToggleInterceptor.java
 e176a0df3141dcb088e05203cca4de3f0d3feea7 
  src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 
cad63c77e8144bb64c6b2acaf1b9199be13e4145 
  
src/main/java/org/apache/aurora/scheduler/thrift/aop/UserCapabilityInterceptor.java
 5e6577d64b1037c69c3a952008240dcafe3b9f94 
  
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 491aa34d76a6f3fe91188bbc73a2cc7464f3644b 

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


Testing
---

./gradlew -Pq build


Thanks,

Kevin Sweeney



Re: Review Request 29117: Adding thrift API changes document.

2015-02-12 Thread Aurora ReviewBot

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

Ship it!


Master (b62ec61) 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 Feb. 13, 2015, 12:35 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29117/
 ---
 
 (Updated Feb. 13, 2015, 12:35 a.m.)
 
 
 Review request for Aurora and Bill Farner.
 
 
 Bugs: AURORA-973
 https://issues.apache.org/jira/browse/AURORA-973
 
 
 Repository: aurora
 
 
 Description
 ---
 
 This is a first stab at documenting thrift deprecation. Any 
 suggestions/comments are welcome.
 
 
 Diffs
 -
 
   docs/developing-aurora-client.md e002d908ce07020513fa06c7e70c41f9e2f55b1d 
   docs/developing-aurora-scheduler.md 
 7f6cc2e6c8e01115a9b7a7dc7633bcd88ba02a0f 
   docs/thrift-deprecation.md PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29117/diff/
 
 
 Testing
 ---
 
 https://github.com/maxim111333/incubator-aurora/blob/populated_deprecation/docs/thrift-deprecation.md
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.

2015-02-12 Thread Maxim Khutornenko

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

(Updated Feb. 13, 2015, 2:27 a.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

Rebased.


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


Repository: aurora


Description
---

Offer filtering for static vetoes. Part 3 of 4: Filtering out statically banned 
offers.

Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a 
parent.

Original RB: https://reviews.apache.org/r/28617/


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 
332338b7bef7d622333f8ea6508c4f5970b8e7c4 
  src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
ced3bdef1731354aedc82bb12f45ba6f040e1ab7 
  src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 
  src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 
2b5dc4902f57e508d76f5e16997ae09d04464220 
  src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
d0e11932e8b5ba1393279137c8465a308e1d6bf5 
  src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
74e31334bc139c47eb8b0beee46ee7bad62a2f80 

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


Testing
---

./gradlew -Pq build


Thanks,

Maxim Khutornenko



Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.

2015-02-12 Thread Aurora ReviewBot

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

Ship it!


Master (7637200) 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 Feb. 13, 2015, 2:27 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/30891/
 ---
 
 (Updated Feb. 13, 2015, 2:27 a.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Bugs: AURORA-909
 https://issues.apache.org/jira/browse/AURORA-909
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Offer filtering for static vetoes. Part 3 of 4: Filtering out statically 
 banned offers.
 
 Will not apply cleanly: diffed with https://reviews.apache.org/r/30890 as a 
 parent.
 
 Original RB: https://reviews.apache.org/r/28617/
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 
 332338b7bef7d622333f8ea6508c4f5970b8e7c4 
   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
 ced3bdef1731354aedc82bb12f45ba6f040e1ab7 
   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
 5a0f7ddb7e8fa6869cbb0fdfd07c6881780c6917 
   src/test/java/org/apache/aurora/scheduler/async/OfferQueueImplTest.java 
 2b5dc4902f57e508d76f5e16997ae09d04464220 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
 d0e11932e8b5ba1393279137c8465a308e1d6bf5 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
 74e31334bc139c47eb8b0beee46ee7bad62a2f80 
 
 Diff: https://reviews.apache.org/r/30891/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Maxim Khutornenko
 




Re: Review Request 30461: Adding pulse_interval_secs into client UpdateConfig.

2015-02-12 Thread Maxim Khutornenko

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

(Updated Feb. 12, 2015, 5:40 p.m.)


Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner.


Changes
---

Stylecheck fixes.


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


Repository: aurora


Description
---

- Adding pulse_interval_secs into client UpdateConfig and validating its range
- Raising an error in client updater for pulse_interval_secs


Diffs (updated)
-

  src/main/python/apache/aurora/client/api/updater.py 
9f91de625f55514530a4f948d7ecdf7b5614b594 
  src/main/python/apache/aurora/client/api/updater_util.py 
9d2e893a6ecff0fc48c7944575578443d41ced78 
  src/main/python/apache/aurora/config/schema/base.py 
e4433d2d47668f59bce169359131284d361bad09 
  src/test/python/apache/aurora/client/api/test_updater.py 
dd3f228c5062d388b4393aa4fd5b60a685bdb3a6 
  src/test/python/apache/aurora/client/api/test_updater_util.py 
fe3ac49491ca710761632405ac09de0cc0d038a5 

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


Testing
---

./pants test.pytest src/test/python/apache/aurora/client::


Thanks,

Maxim Khutornenko