from:"Maxim Khutornenko"


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

Review request for Aurora, Kevin Sweeney and Bill Farner.


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


Repository: aurora


Description
---

Adding PMD rule to check @Timed annotation placement.


Diffs
-

  config/pmd/design.xml 2741e407e35826e73bd4dc8335c0ef6294086398 

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


Testing
---

```
$ ./gradlew -Pq --rerun-tasks pmdMain
:buildSrc:compileJava UP-TO-DATE
:buildSrc:compileGroovy
:buildSrc:processResources UP-TO-DATE
:buildSrc:classes
:buildSrc:jar
:buildSrc:assemble
:buildSrc:compileTestJava UP-TO-DATE
:buildSrc:compileTestGroovy UP-TO-DATE
:buildSrc:processTestResources UP-TO-DATE
:buildSrc:testClasses UP-TO-DATE
:buildSrc:test UP-TO-DATE
:buildSrc:check UP-TO-DATE
:buildSrc:build
:pmdMain
/Users/mkhutornenko/workspace3/aurora/src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java:204:
   A method must be overridable to have a @Timed annotation.
:pmdMain FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':pmdMain'.
 1 PMD rule violations were found. See the report at: 
 file:///Users/mkhutornenko/workspace3/aurora/dist/reports/pmd/main.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: 12.981 secs
```


Thanks,

Maxim Khutornenko

Re: Review Request 28914: Adding PMD rule to check @Timed annotation placement.


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

(Updated Dec. 10, 2014, 8:02 p.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

Fixed description.


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


Repository: aurora


Description
---

Adding PMD rule to check @Timed annotation placement.


Diffs (updated)
-

  config/pmd/design.xml 2741e407e35826e73bd4dc8335c0ef6294086398 

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


Testing
---

```
$ ./gradlew -Pq --rerun-tasks pmdMain
:buildSrc:compileJava UP-TO-DATE
:buildSrc:compileGroovy
:buildSrc:processResources UP-TO-DATE
:buildSrc:classes
:buildSrc:jar
:buildSrc:assemble
:buildSrc:compileTestJava UP-TO-DATE
:buildSrc:compileTestGroovy UP-TO-DATE
:buildSrc:processTestResources UP-TO-DATE
:buildSrc:testClasses UP-TO-DATE
:buildSrc:test UP-TO-DATE
:buildSrc:check UP-TO-DATE
:buildSrc:build
:pmdMain
/Users/mkhutornenko/workspace3/aurora/src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java:204:
   A method must be overridable to have a @Timed annotation.
:pmdMain FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':pmdMain'.
 1 PMD rule violations were found. See the report at: 
 file:///Users/mkhutornenko/workspace3/aurora/dist/reports/pmd/main.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: 12.981 secs
```


Thanks,

Maxim Khutornenko

Re: Review Request 28914: Adding PMD rule to check @Timed annotation placement.



 On Dec. 10, 2014, 8 p.m., Kevin Sweeney wrote:
  config/pmd/design.xml, lines 1914-1915
  https://reviews.apache.org/r/28914/diff/1/?file=788485#file788485line1914
 
  Fix description?

Yup, just noticed and updated.


- Maxim


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


On Dec. 10, 2014, 8:02 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28914/
 ---
 
 (Updated Dec. 10, 2014, 8:02 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Bugs: AURORA-967
 https://issues.apache.org/jira/browse/AURORA-967
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Adding PMD rule to check @Timed annotation placement.
 
 
 Diffs
 -
 
   config/pmd/design.xml 2741e407e35826e73bd4dc8335c0ef6294086398 
 
 Diff: https://reviews.apache.org/r/28914/diff/
 
 
 Testing
 ---
 
 ```
 $ ./gradlew -Pq --rerun-tasks pmdMain
 :buildSrc:compileJava UP-TO-DATE
 :buildSrc:compileGroovy
 :buildSrc:processResources UP-TO-DATE
 :buildSrc:classes
 :buildSrc:jar
 :buildSrc:assemble
 :buildSrc:compileTestJava UP-TO-DATE
 :buildSrc:compileTestGroovy UP-TO-DATE
 :buildSrc:processTestResources UP-TO-DATE
 :buildSrc:testClasses UP-TO-DATE
 :buildSrc:test UP-TO-DATE
 :buildSrc:check UP-TO-DATE
 :buildSrc:build
 :pmdMain
 /Users/mkhutornenko/workspace3/aurora/src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java:204:
  A method must be overridable to have a @Timed annotation.
 :pmdMain FAILED
 
 FAILURE: Build failed with an exception.
 
 * What went wrong:
 Execution failed for task ':pmdMain'.
  1 PMD rule violations were found. See the report at: 
  file:///Users/mkhutornenko/workspace3/aurora/dist/reports/pmd/main.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: 12.981 secs
 ```
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28914: Adding PMD rule to check @Timed annotation placement.



 On Dec. 10, 2014, 8:01 p.m., Kevin Sweeney wrote:
  config/pmd/design.xml, line 1909
  https://reviews.apache.org/r/28914/diff/1/?file=788485#file788485line1909
 
  Also, consider adding this to a custom.xml file.

Good idea. Done.


- Maxim


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


On Dec. 10, 2014, 8:02 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28914/
 ---
 
 (Updated Dec. 10, 2014, 8:02 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Bugs: AURORA-967
 https://issues.apache.org/jira/browse/AURORA-967
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Adding PMD rule to check @Timed annotation placement.
 
 
 Diffs
 -
 
   config/pmd/design.xml 2741e407e35826e73bd4dc8335c0ef6294086398 
 
 Diff: https://reviews.apache.org/r/28914/diff/
 
 
 Testing
 ---
 
 ```
 $ ./gradlew -Pq --rerun-tasks pmdMain
 :buildSrc:compileJava UP-TO-DATE
 :buildSrc:compileGroovy
 :buildSrc:processResources UP-TO-DATE
 :buildSrc:classes
 :buildSrc:jar
 :buildSrc:assemble
 :buildSrc:compileTestJava UP-TO-DATE
 :buildSrc:compileTestGroovy UP-TO-DATE
 :buildSrc:processTestResources UP-TO-DATE
 :buildSrc:testClasses UP-TO-DATE
 :buildSrc:test UP-TO-DATE
 :buildSrc:check UP-TO-DATE
 :buildSrc:build
 :pmdMain
 /Users/mkhutornenko/workspace3/aurora/src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java:204:
  A method must be overridable to have a @Timed annotation.
 :pmdMain FAILED
 
 FAILURE: Build failed with an exception.
 
 * What went wrong:
 Execution failed for task ':pmdMain'.
  1 PMD rule violations were found. See the report at: 
  file:///Users/mkhutornenko/workspace3/aurora/dist/reports/pmd/main.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: 12.981 secs
 ```
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28914: Adding PMD rule to check @Timed annotation placement.


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

(Updated Dec. 10, 2014, 8:06 p.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

Kevin's comments.


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


Repository: aurora


Description
---

Adding PMD rule to check @Timed annotation placement.


Diffs (updated)
-

  config/pmd/custom.xml PRE-CREATION 

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


Testing
---

```
$ ./gradlew -Pq --rerun-tasks pmdMain
:buildSrc:compileJava UP-TO-DATE
:buildSrc:compileGroovy
:buildSrc:processResources UP-TO-DATE
:buildSrc:classes
:buildSrc:jar
:buildSrc:assemble
:buildSrc:compileTestJava UP-TO-DATE
:buildSrc:compileTestGroovy UP-TO-DATE
:buildSrc:processTestResources UP-TO-DATE
:buildSrc:testClasses UP-TO-DATE
:buildSrc:test UP-TO-DATE
:buildSrc:check UP-TO-DATE
:buildSrc:build
:pmdMain
/Users/mkhutornenko/workspace3/aurora/src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java:204:
   A method must be overridable to have a @Timed annotation.
:pmdMain FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':pmdMain'.
 1 PMD rule violations were found. See the report at: 
 file:///Users/mkhutornenko/workspace3/aurora/dist/reports/pmd/main.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: 12.981 secs
```


Thanks,

Maxim Khutornenko

Re: Review Request 28914: Adding PMD rule to check @Timed annotation placement.



 On Dec. 10, 2014, 9:07 p.m., Bill Farner wrote:
  config/pmd/design.xml, line 1923
  https://reviews.apache.org/r/28914/diff/2/?file=788486#file788486line1923
 
  Should this be =1?  JDK8 supports multiple annotations of the same 
  type, so while silly it might be good to keep in mind.

Sure, 0 does the trick.


- Maxim


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


On Dec. 10, 2014, 8:06 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28914/
 ---
 
 (Updated Dec. 10, 2014, 8:06 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Bugs: AURORA-967
 https://issues.apache.org/jira/browse/AURORA-967
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Adding PMD rule to check @Timed annotation placement.
 
 
 Diffs
 -
 
   config/pmd/custom.xml PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/28914/diff/
 
 
 Testing
 ---
 
 ```
 $ ./gradlew -Pq --rerun-tasks pmdMain
 :buildSrc:compileJava UP-TO-DATE
 :buildSrc:compileGroovy
 :buildSrc:processResources UP-TO-DATE
 :buildSrc:classes
 :buildSrc:jar
 :buildSrc:assemble
 :buildSrc:compileTestJava UP-TO-DATE
 :buildSrc:compileTestGroovy UP-TO-DATE
 :buildSrc:processTestResources UP-TO-DATE
 :buildSrc:testClasses UP-TO-DATE
 :buildSrc:test UP-TO-DATE
 :buildSrc:check UP-TO-DATE
 :buildSrc:build
 :pmdMain
 /Users/mkhutornenko/workspace3/aurora/src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java:204:
  A method must be overridable to have a @Timed annotation.
 :pmdMain FAILED
 
 FAILURE: Build failed with an exception.
 
 * What went wrong:
 Execution failed for task ':pmdMain'.
  1 PMD rule violations were found. See the report at: 
  file:///Users/mkhutornenko/workspace3/aurora/dist/reports/pmd/main.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: 12.981 secs
 ```
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28914: Adding PMD rule to check @Timed annotation placement.



 On Dec. 10, 2014, 8:31 p.m., Kevin Sweeney wrote:
  config/pmd/custom.xml, line 21
  https://reviews.apache.org/r/28914/diff/3/?file=788487#file788487line21
 
  Name = Aurora?

Sure.


 On Dec. 10, 2014, 8:31 p.m., Kevin Sweeney wrote:
  config/pmd/custom.xml, line 15
  https://reviews.apache.org/r/28914/diff/3/?file=788487#file788487line15
 
  Remove this comment.

Removed.


 On Dec. 10, 2014, 8:31 p.m., Kevin Sweeney wrote:
  config/pmd/custom.xml, line 36
  https://reviews.apache.org/r/28914/diff/3/?file=788487#file788487line36
 
  Used with the @Timed annotation. Also consider adding a link to the 
  guice docs on method interceptor limitations for those unfamiliar.

Added.


 On Dec. 10, 2014, 8:31 p.m., Kevin Sweeney wrote:
  config/pmd/custom.xml, line 43
  https://reviews.apache.org/r/28914/diff/3/?file=788487#file788487line43
 
  My XPath-foo is lacking - will this catch all of the following forms:
  
  ```java
  @Timed(test_var)
  private void myMethod() {}
  
  @Timed
  void myMethod2() {}
  
  @Timed(value = time_var)
  static void myMethod3() {}
  ```
  
  Also is there a way to limit this match to the Timed annotation from 
  com.twitter.common.inject.TimedInterceptor.Timed?

Modifed to support all use cases.

As for the type resolution, this would be much more involved and would require 
implementing part of the rule in java to use ClassLoader. I am punting on this.


- Maxim


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


On Dec. 10, 2014, 8:06 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28914/
 ---
 
 (Updated Dec. 10, 2014, 8:06 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Bugs: AURORA-967
 https://issues.apache.org/jira/browse/AURORA-967
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Adding PMD rule to check @Timed annotation placement.
 
 
 Diffs
 -
 
   config/pmd/custom.xml PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/28914/diff/
 
 
 Testing
 ---
 
 ```
 $ ./gradlew -Pq --rerun-tasks pmdMain
 :buildSrc:compileJava UP-TO-DATE
 :buildSrc:compileGroovy
 :buildSrc:processResources UP-TO-DATE
 :buildSrc:classes
 :buildSrc:jar
 :buildSrc:assemble
 :buildSrc:compileTestJava UP-TO-DATE
 :buildSrc:compileTestGroovy UP-TO-DATE
 :buildSrc:processTestResources UP-TO-DATE
 :buildSrc:testClasses UP-TO-DATE
 :buildSrc:test UP-TO-DATE
 :buildSrc:check UP-TO-DATE
 :buildSrc:build
 :pmdMain
 /Users/mkhutornenko/workspace3/aurora/src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java:204:
  A method must be overridable to have a @Timed annotation.
 :pmdMain FAILED
 
 FAILURE: Build failed with an exception.
 
 * What went wrong:
 Execution failed for task ':pmdMain'.
  1 PMD rule violations were found. See the report at: 
  file:///Users/mkhutornenko/workspace3/aurora/dist/reports/pmd/main.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: 12.981 secs
 ```
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28914: Adding PMD rule to check @Timed annotation placement.


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

(Updated Dec. 10, 2014, 10:37 p.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

CR comments.


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


Repository: aurora


Description
---

Adding PMD rule to check @Timed annotation placement.


Diffs (updated)
-

  config/pmd/custom.xml PRE-CREATION 

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


Testing
---

```
$ ./gradlew -Pq --rerun-tasks pmdMain
:buildSrc:compileJava UP-TO-DATE
:buildSrc:compileGroovy
:buildSrc:processResources UP-TO-DATE
:buildSrc:classes
:buildSrc:jar
:buildSrc:assemble
:buildSrc:compileTestJava UP-TO-DATE
:buildSrc:compileTestGroovy UP-TO-DATE
:buildSrc:processTestResources UP-TO-DATE
:buildSrc:testClasses UP-TO-DATE
:buildSrc:test UP-TO-DATE
:buildSrc:check UP-TO-DATE
:buildSrc:build
:pmdMain
/Users/mkhutornenko/workspace3/aurora/src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java:204:
   A method must be overridable to have a @Timed annotation.
:pmdMain FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':pmdMain'.
 1 PMD rule violations were found. See the report at: 
 file:///Users/mkhutornenko/workspace3/aurora/dist/reports/pmd/main.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: 12.981 secs
```


Thanks,

Maxim Khutornenko

Re: Review Request 28915: Use pip to pre-fetch python dependencies.


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

Ship it!


Ship It!

- Maxim Khutornenko


On Dec. 10, 2014, 9:39 p.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28915/
 ---
 
 (Updated Dec. 10, 2014, 9:39 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
 
 
 Repository: aurora
 
 
 Description
 ---
 
 This is to hopefully eradicate flaked builds like the following:
 ```
   File 
 /home/jenkins/jenkins-slave/workspace/Aurora/build-support/pants.venv/lib/python2.7/site-packages/pants/backend/python/commands/build.py,
  line 130, in _python_build
 debug=self._verbose)
   File 
 /home/jenkins/jenkins-slave/workspace/Aurora/build-support/pants.venv/lib/python2.7/site-packages/pants/backend/python/python_builder.py,
  line 43, in build
 debug=debug).run()
   File 
 /home/jenkins/jenkins-slave/workspace/Aurora/build-support/pants.venv/lib/python2.7/site-packages/pants/backend/python/test_builder.py,
  line 88, in run
 rv = self._run_tests([target], stdout, stderr)
   File 
 /home/jenkins/jenkins-slave/workspace/Aurora/build-support/pants.venv/lib/python2.7/site-packages/pants/backend/python/test_builder.py,
  line 316, in _run_tests
 with self._test_runner(targets, stdout, stderr) as (pex, test_args):
   File /usr/lib/python2.7/contextlib.py, line 17, in __enter__
 return self.gen.next()
   File 
 /home/jenkins/jenkins-slave/workspace/Aurora/build-support/pants.venv/lib/python2.7/site-packages/pants/backend/python/test_builder.py,
  line 295, in _test_runner
 builder = chroot.dump()
   File 
 /home/jenkins/jenkins-slave/workspace/Aurora/build-support/pants.venv/lib/python2.7/site-packages/pants/backend/python/python_chroot.py,
  line 191, in dump
 conn_timeout=self._conn_timeout)
   File 
 /home/jenkins/jenkins-slave/workspace/Aurora/build-support/pants.venv/lib/python2.7/site-packages/pants/backend/python/resolver.py,
  line 100, in resolve_multi
 platform=platform)
   File 
 /home/jenkins/jenkins-slave/workspace/Aurora/build-support/pants.venv/lib/python2.7/site-packages/pex/resolver.py,
  line 101, in resolve
 raise Unsatisfiable('Cannot satisfy requirements: %s' % 
 requirement_set[requirement.key])
 Unsatisfiable: Cannot satisfy requirements: 
 [PythonRequirement(pystachio==0.7.2)]
 ```
 
 Thanks to Kevin for providing the pointers here.
 
 
 Diffs
 -
 
   build-support/jenkins/build.sh 40ef938929af3729d105dcd624f1b919b1868afa 
 
 Diff: https://reviews.apache.org/r/28915/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Bill Farner

Re: Review Request 28876: Suppressing redundant client command error messaging.

On Dec. 9, 2014, 11:29 p.m., Zameer Manji wrote:
src/main/python/apache/aurora/client/cli/__init__.py, line 384
https://reviews.apache.org/r/28876/diff/1/?file=787343#file787343line384

This solution seems to be papering over the problem we have with output
in the client.

Instead of creating a new type of exception can we just prevent the
double printing in general?

Maxim Khutornenko wrote:
Not sure I buy it. This `print_err()` is needed when a CommandError is
raised to bail out due to some internal problem (i.e. not related to
scheduler call and not routed through context.check_and_log_response). The
fact that the same error type is used for both is exactly the reason we have
these redundant messages. There are plenty of legitimate cases where the
error needs to be logged. Consider this example:
```
$ aurora2 job kill devcluster/www-data/prod/hello
Error executing command: The instances list cannot be omitted in a kill
command!; use killall to kill all instances
```

Bill Farner wrote:
Part of the problem here is that logging to stdout/stderr happens
throughout the stack. This seems to inevitably lead towards redundant
output. Perhaps we should take the approach of limiting use of stdout/stderr
to the top of the stack whenever possible? The only use case that seems to
not fit that today seems to be client-side job updates, which we could ignore
since it's scheduled for removal.

Maxim Khutornenko wrote:
| Perhaps we should take the approach of limiting use of stdout/stderr to
the top of the stack whenever possible?

Agree, that would make sense. There are plenty of examples where that
happens. E.g. compare [1] and [2]. The first one has redundant print_err()
and will result in duplicate messages. The second one (used as example above)
only loggged by the CommineLine handler. This diff addresses a problem where
a redundant incomplete message is logged after unsuccessful scheduler call. I
suggest we tackle the [1] separately.

[1] -
https://github.com/apache/incubator-aurora/blob/master/src/main/python/apache/aurora/client/cli/jobs.py#L342-L344
[2] -
https://github.com/apache/incubator-aurora/blob/master/src/main/python/apache/aurora/client/cli/jobs.py#L368-L370

Bill Farner wrote:
Going back to Zameer's comment, though, maybe `check_and_log_response`
should refrain from calling `print_err`? Seems like the receiving end of
`CommandError` should be responsible for presenting the error.

Maxim Khutornenko wrote:
I am not sure what it really buys us. Right now, the scheduler response
is entirely handled within the check_and_log_response that does centralized
logging to both stdout and stderr. IMO, delegating just stderr handling
elsewhere creates more ambiguity and is not solving any additional problems.

Zameer Manji wrote:
Maxim, if you agree to tackle the redundant print_err()/CommandError in
another RB I am comfortable with this change.

These are two different issues and I was going to address the other one in a
separate RB. Filed AURORA-968 to track it.

- Maxim

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

On Dec. 9, 2014, 10:32 p.m., Maxim Khutornenko wrote:

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

(Updated Dec. 9, 2014, 10:32 p.m.)

Review request for Aurora, Bill Farner and Zameer Manji.

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

Repository: aurora

Description
---

Added a new error type to raise when errors are logged explicitly.

Diffs
-

src/main/python/apache/aurora/client/cli/__init__.py
c2eb89cae536838ac4dd46b0125a248d84f6e54c
src/main/python/apache/aurora/client/cli/context.py
ad27eb5674fb85c5d3a26a014349abdea4fa2d64
src/test/python/apache/aurora/client/cli/test_update.py
1f061a39279bf5ef9d4e7279016bf07e164014bb

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

Testing
---

./pants src/test/python/apache/aurora/client/cli:all

Vagrant before:

```
$ aurora2 job create devcluster/www-data/prod/hello
aurora/examples/jobs/hello_world.aurora
INFO] Creating job hello
Job creation failed due to error:
Job already exists: www-data/prod/hello
Error executing command: Job creation failed due to error:
```

Vagrant after:
```
$ aurora2 job create devcluster/www-data/prod/hello
aurora/examples/jobs/hello_world.aurora
INFO] Creating job hello
Job creation failed due to error:
Job already exists: www-data/prod/hello
```

Thanks

Re: Review Request 28872: Improving quota check message in the client.


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

(Updated Dec. 11, 2014, 2:54 a.m.)


Review request for Aurora, Kevin Sweeney and Zameer Manji.


Changes
---

CR comments.


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


Repository: aurora


Description
---

A minor fix improving the quota check message on the client. Minor enough to 
consider for a dying client updater.


Diffs (updated)
-

  src/main/python/apache/aurora/client/api/quota_check.py 
c994050d76a9b617f70dfcbf60cf10e02d4a4bb9 
  src/test/python/apache/aurora/client/api/test_quota_check.py 
2fc76d21ca63ae7f33b1e03ccb88f52fe82dc76c 

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


Testing
---

./pants src/test/python/apache/aurora/client/api:quota_check

Vagrant:
```
$ aurora2 job update devcluster/www-data/prod/hello 
aurora/examples/jobs/hello_world.aurora 
 INFO] 
 INFO] 
 INFO] Updating job: hello
 INFO] Not enough quota to create/update job.
 INFO] Total allocated quota for www-data:
CPU 2.0
RAM 2.00 GB
Disk2.00 GB
 INFO] Consumed quota for www-data:
CPU 1.9
RAM 1.125977 GB
Disk1.125000 GB
 INFO] Requested for hello:
CPU 9.1
RAM 0.00 GB
Disk0.00 GB
 INFO] Additional quota required for www-data:
CPU 9.0
RAM 0.00 GB
Disk0.00 GB
Update failed due to error:
Unable to start job update: Response from scheduler: INVALID_REQUEST 
(message: Failed quota check.)
Error executing command: Update failed due to error:
```


Thanks,

Maxim Khutornenko

Re: Review Request 28872: Improving quota check message in the client.



 On Dec. 9, 2014, 11:41 p.m., Zameer Manji wrote:
  src/test/python/apache/aurora/client/api/test_quota_check.py, line 124
  https://reviews.apache.org/r/28872/diff/1/?file=787283#file787283line124
 
  Can you set all the calls to print quota?

Sure, done.


- Maxim


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


On Dec. 9, 2014, 9:30 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28872/
 ---
 
 (Updated Dec. 9, 2014, 9:30 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Zameer Manji.
 
 
 Bugs: AURORA-469
 https://issues.apache.org/jira/browse/AURORA-469
 
 
 Repository: aurora
 
 
 Description
 ---
 
 A minor fix improving the quota check message on the client. Minor enough to 
 consider for a dying client updater.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/api/quota_check.py 
 c994050d76a9b617f70dfcbf60cf10e02d4a4bb9 
   src/test/python/apache/aurora/client/api/test_quota_check.py 
 2fc76d21ca63ae7f33b1e03ccb88f52fe82dc76c 
 
 Diff: https://reviews.apache.org/r/28872/diff/
 
 
 Testing
 ---
 
 ./pants src/test/python/apache/aurora/client/api:quota_check
 
 Vagrant:
 ```
 $ aurora2 job update devcluster/www-data/prod/hello 
 aurora/examples/jobs/hello_world.aurora 
  INFO] 
  INFO] 
  INFO] Updating job: hello
  INFO] Not enough quota to create/update job.
  INFO] Total allocated quota for www-data:
   CPU 2.0
   RAM 2.00 GB
   Disk2.00 GB
  INFO] Consumed quota for www-data:
   CPU 1.9
   RAM 1.125977 GB
   Disk1.125000 GB
  INFO] Requested for hello:
   CPU 9.1
   RAM 0.00 GB
   Disk0.00 GB
  INFO] Additional quota required for www-data:
   CPU 9.0
   RAM 0.00 GB
   Disk0.00 GB
 Update failed due to error:
   Unable to start job update: Response from scheduler: INVALID_REQUEST 
 (message: Failed quota check.)
 Error executing command: Update failed due to error:
 ```
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28872: Improving quota check message in the client.


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

(Updated Dec. 11, 2014, 2:55 a.m.)


Review request for Aurora, Kevin Sweeney and Zameer Manji.


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


Repository: aurora


Description
---

A minor fix improving the quota check message on the client. Minor enough to 
consider for a dying client updater.


Diffs (updated)
-

  src/main/python/apache/aurora/client/api/quota_check.py 
c994050d76a9b617f70dfcbf60cf10e02d4a4bb9 
  src/test/python/apache/aurora/client/api/test_quota_check.py 
2fc76d21ca63ae7f33b1e03ccb88f52fe82dc76c 

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


Testing
---

./pants src/test/python/apache/aurora/client/api:quota_check

Vagrant:
```
$ aurora2 job update devcluster/www-data/prod/hello 
aurora/examples/jobs/hello_world.aurora 
 INFO] 
 INFO] 
 INFO] Updating job: hello
 INFO] Not enough quota to create/update job.
 INFO] Total allocated quota for www-data:
CPU 2.0
RAM 2.00 GB
Disk2.00 GB
 INFO] Consumed quota for www-data:
CPU 1.9
RAM 1.125977 GB
Disk1.125000 GB
 INFO] Requested for hello:
CPU 9.1
RAM 0.00 GB
Disk0.00 GB
 INFO] Additional quota required for www-data:
CPU 9.0
RAM 0.00 GB
Disk0.00 GB
Update failed due to error:
Unable to start job update: Response from scheduler: INVALID_REQUEST 
(message: Failed quota check.)
Error executing command: Update failed due to error:
```


Thanks,

Maxim Khutornenko

Re: Review Request 28831: Changing the default --batch-size to 1.



 On Dec. 9, 2014, 2:33 a.m., Bill Farner wrote:
  src/test/python/apache/aurora/client/cli/test_kill.py, line 134
  https://reviews.apache.org/r/28831/diff/1/?file=786267#file786267line134
 
  I believe this should be of the form `assert foo.mock_calls == [x]`
  
  Ditto elsewhere.

Doh, thanks for catching. I wasn't having a good day and python made it worse 
by allowing an illegal assignment. 

Adding asserts revealed deeper problems with JobMonitor where scheduler calls 
were made in arbitrary fashion. Mocked out JobMonitor to avoid randomness. 
These tests still need more love to become truly unit rather than integration 
tests but I have to stop here to avoid a complete overhaul.


- Maxim


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


On Dec. 9, 2014, 1:25 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28831/
 ---
 
 (Updated Dec. 9, 2014, 1:25 a.m.)
 
 
 Review request for Aurora and Bill Farner.
 
 
 Bugs: AURORA-961
 https://issues.apache.org/jira/browse/AURORA-961
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Also, cleaned up kill command tests a bit.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/options.py 
 e844cf340583b631ce194352f403bbaec71655b7 
   src/test/python/apache/aurora/client/cli/test_kill.py 
 1eda72af4c19831ae27733f506858e67772b2075 
   src/test/python/apache/aurora/client/cli/util.py 
 67d7eaa6eff4e1dbaaa485166e084812a4f04074 
 
 Diff: https://reviews.apache.org/r/28831/diff/
 
 
 Testing
 ---
 
 ./pants src/test/python/apache/aurora/client/cli:all
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28831: Changing the default --batch-size to 1.


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

(Updated Dec. 9, 2014, 6:14 p.m.)


Review request for Aurora and Bill Farner.


Changes
---

CR comments.


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


Repository: aurora


Description
---

Also, cleaned up kill command tests a bit.


Diffs (updated)
-

  src/main/python/apache/aurora/client/cli/options.py 
e844cf340583b631ce194352f403bbaec71655b7 
  src/test/python/apache/aurora/client/cli/test_kill.py 
1eda72af4c19831ae27733f506858e67772b2075 
  src/test/python/apache/aurora/client/cli/util.py 
67d7eaa6eff4e1dbaaa485166e084812a4f04074 

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


Testing
---

./pants src/test/python/apache/aurora/client/cli:all


Thanks,

Maxim Khutornenko

Re: Review Request 28831: Changing the default --batch-size to 1.


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

(Updated Dec. 9, 2014, 8:18 p.m.)


Review request for Aurora and Bill Farner.


Changes
---

Addressed Kevin's comments.


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


Repository: aurora


Description
---

Also, cleaned up kill command tests a bit.


Diffs (updated)
-

  src/main/python/apache/aurora/client/cli/options.py 
e844cf340583b631ce194352f403bbaec71655b7 
  src/test/python/apache/aurora/client/cli/test_kill.py 
1eda72af4c19831ae27733f506858e67772b2075 
  src/test/python/apache/aurora/client/cli/util.py 
67d7eaa6eff4e1dbaaa485166e084812a4f04074 

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


Testing
---

./pants src/test/python/apache/aurora/client/cli:all


Thanks,

Maxim Khutornenko

Review Request 28872: Improving quota check message in the client.


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

Review request for Aurora, Kevin Sweeney and Zameer Manji.


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


Repository: aurora


Description
---

A minor fix improving the quota check message on the client. Minor enough to 
consider for a dying client updater.


Diffs
-

  src/main/python/apache/aurora/client/api/quota_check.py 
c994050d76a9b617f70dfcbf60cf10e02d4a4bb9 
  src/test/python/apache/aurora/client/api/test_quota_check.py 
2fc76d21ca63ae7f33b1e03ccb88f52fe82dc76c 

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


Testing
---

./pants src/test/python/apache/aurora/client/api:quota_check

Vagrant:
```
$ aurora2 job update devcluster/www-data/prod/hello 
aurora/examples/jobs/hello_world.aurora 
 INFO] 
 INFO] 
 INFO] Updating job: hello
 INFO] Not enough quota to create/update job.
 INFO] Total allocated quota for www-data:
CPU 2.0
RAM 2.00 GB
Disk2.00 GB
 INFO] Consumed quota for www-data:
CPU 1.9
RAM 1.125977 GB
Disk1.125000 GB
 INFO] Requested for hello:
CPU 9.1
RAM 0.00 GB
Disk0.00 GB
 INFO] Additional quota required for www-data:
CPU 9.0
RAM 0.00 GB
Disk0.00 GB
Update failed due to error:
Unable to start job update: Response from scheduler: INVALID_REQUEST 
(message: Failed quota check.)
Error executing command: Update failed due to error:
```


Thanks,

Maxim Khutornenko

Review Request 28876: Suppressing redundant client command error messaging.


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

Review request for Aurora, Bill Farner and Zameer Manji.


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


Repository: aurora


Description
---

Added a new error type to raise when errors are logged explicitly.


Diffs
-

  src/main/python/apache/aurora/client/cli/__init__.py 
c2eb89cae536838ac4dd46b0125a248d84f6e54c 
  src/main/python/apache/aurora/client/cli/context.py 
ad27eb5674fb85c5d3a26a014349abdea4fa2d64 
  src/test/python/apache/aurora/client/cli/test_update.py 
1f061a39279bf5ef9d4e7279016bf07e164014bb 

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


Testing
---

./pants src/test/python/apache/aurora/client/cli:all

Vagrant before:

```
$ aurora2 job create devcluster/www-data/prod/hello 
aurora/examples/jobs/hello_world.aurora 
 INFO] Creating job hello
Job creation failed due to error:
Job already exists: www-data/prod/hello
Error executing command: Job creation failed due to error:
```

Vagrant after:
```
$ aurora2 job create devcluster/www-data/prod/hello 
aurora/examples/jobs/hello_world.aurora 
 INFO] Creating job hello
Job creation failed due to error:
Job already exists: www-data/prod/hello
```


Thanks,

Maxim Khutornenko

Re: Review Request 28876: Suppressing redundant client command error messaging.



 On Dec. 9, 2014, 11:29 p.m., Zameer Manji wrote:
  src/main/python/apache/aurora/client/cli/__init__.py, line 384
  https://reviews.apache.org/r/28876/diff/1/?file=787343#file787343line384
 
  This solution seems to be papering over the problem we have with output 
  in the client.
  
  Instead of creating a new type of exception can we just prevent the 
  double printing in general?

Not sure I buy it. This `print_err()` is needed when a CommandError is raised 
to bail out due to some internal problem (i.e. not related to scheduler call 
and not routed through context.check_and_log_response). The fact that the same 
error type is used for both is exactly the reason we have these redundant 
messages. There are plenty of legitimate cases where the error needs to be 
logged. Consider this example:
```
$ aurora2 job kill devcluster/www-data/prod/hello
Error executing command: The instances list cannot be omitted in a kill 
command!; use killall to kill all instances
```


 On Dec. 9, 2014, 11:29 p.m., Zameer Manji wrote:
  src/test/python/apache/aurora/client/cli/test_update.py, line 69
  https://reviews.apache.org/r/28876/diff/1/?file=787345#file787345line69
 
  Is it at all possible to avoid the use of raw Mock objects?

This goes into Pystachio namespace that I am hesitant to tap into. Given the 
additional complexity I don't see much value in creating a concrete object for 
a parent-mocked container.


- Maxim


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


On Dec. 9, 2014, 10:32 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28876/
 ---
 
 (Updated Dec. 9, 2014, 10:32 p.m.)
 
 
 Review request for Aurora, Bill Farner and Zameer Manji.
 
 
 Bugs: AURORA-965
 https://issues.apache.org/jira/browse/AURORA-965
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Added a new error type to raise when errors are logged explicitly.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/__init__.py 
 c2eb89cae536838ac4dd46b0125a248d84f6e54c 
   src/main/python/apache/aurora/client/cli/context.py 
 ad27eb5674fb85c5d3a26a014349abdea4fa2d64 
   src/test/python/apache/aurora/client/cli/test_update.py 
 1f061a39279bf5ef9d4e7279016bf07e164014bb 
 
 Diff: https://reviews.apache.org/r/28876/diff/
 
 
 Testing
 ---
 
 ./pants src/test/python/apache/aurora/client/cli:all
 
 Vagrant before:
 
 ```
 $ aurora2 job create devcluster/www-data/prod/hello 
 aurora/examples/jobs/hello_world.aurora 
  INFO] Creating job hello
 Job creation failed due to error:
   Job already exists: www-data/prod/hello
 Error executing command: Job creation failed due to error:
 ```
 
 Vagrant after:
 ```
 $ aurora2 job create devcluster/www-data/prod/hello 
 aurora/examples/jobs/hello_world.aurora 
  INFO] Creating job hello
 Job creation failed due to error:
   Job already exists: www-data/prod/hello
 ```
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28876: Suppressing redundant client command error messaging.

On Dec. 9, 2014, 11:29 p.m., Zameer Manji wrote:
src/main/python/apache/aurora/client/cli/__init__.py, line 384
https://reviews.apache.org/r/28876/diff/1/?file=787343#file787343line384

This solution seems to be papering over the problem we have with output
in the client.

Instead of creating a new type of exception can we just prevent the
double printing in general?

| Perhaps we should take the approach of limiting use of stdout/stderr to the
top of the stack whenever possible?

Agree, that would make sense. There are plenty of examples where that happens.
E.g. compare [1] and [2]. The first one has redundant print_err() and will
result in duplicate messages. The second one (used as example above) only
loggged by the CommineLine handler. This diff addresses a problem where a
redundant incomplete message is logged after unsuccessful scheduler call. I
suggest we tackle the [1] separately.

- Maxim

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

On Dec. 9, 2014, 10:32 p.m., Maxim Khutornenko wrote:

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

(Updated Dec. 9, 2014, 10:32 p.m.)

Review request for Aurora, Bill Farner and Zameer Manji.

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

Repository: aurora

Description
---

Added a new error type to raise when errors are logged explicitly.

Diffs
-

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

Testing
---

./pants src/test/python/apache/aurora/client/cli:all

Vagrant before:

Thanks,

Maxim Khutornenko

Review Request 28883: Fixing @Timed method visibility.


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

Review request for Aurora and Bill Farner.


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


Repository: aurora


Description
---

Fixing @Timed method visibility.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
ead9d28100673440168a32d114ecaa15874978a6 

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


Testing
---

Tested in vagrant:
```
task_schedule_attempt_events 5
task_schedule_attempt_events_per_sec 0.9989254079902053
task_schedule_attempt_locked_events 5
task_schedule_attempt_locked_events_per_sec 1.0019749236357087
task_schedule_attempt_locked_nanos_per_event 1570156.6526848162
task_schedule_attempt_locked_nanos_total 45453749
task_schedule_attempt_locked_nanos_total_per_sec 1573257.5921699686
task_schedule_attempt_nanos_per_event 1759287.885810068
task_schedule_attempt_nanos_total 48291560
task_schedule_attempt_nanos_total_per_sec 1757397.369105048
```


Thanks,

Maxim Khutornenko

Re: Review Request 28710: Adding JMH framework support for scheduler performance analysis.



 On Dec. 10, 2014, 1:09 a.m., Kevin Sweeney wrote:
  src/jmh/java/org/apache/aurora/benchmark/SchedulerBenchmark.java, lines 
  29-32
  https://reviews.apache.org/r/28710/diff/4/?file=784251#file784251line29
 
  What does the main get us here? Isn't the gradle plugin wiring this up?

Good point. I used it to run benchmarks from command line but given the command 
line execution is rather tedious with all jmh flags passed into it, it's 
cleaner to support only one way via gradle. Dropped.


- Maxim


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


On Dec. 6, 2014, 12:33 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28710/
 ---
 
 (Updated Dec. 6, 2014, 12:33 a.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Repository: aurora
 
 
 Description
 ---
 
 This RB is superseding the https://reviews.apache.org/r/28474/.
 
 
 I have spent some time researching the available microbenchmark frameworks 
 and JMH [1] came as a clear winner:
 - Active development trail [2]
 - Advanced featureset and built-in optimizations improving accuracy and 
 consistency [3]
 - Well documented set of examples [4]
 - Large community experience and collective wisdom. 
 
 
 This RB adds gradle support for running JMH benchmarks and is relying on JMH 
 gradle plugin [5]. The benchmarks are run via `./gradlew jmh` command.
 
 [1] - http://openjdk.java.net/projects/code-tools/jmh/
 [2] - http://hg.openjdk.java.net/code-tools/jmh/ 
 [3] - 
 https://groups.google.com/forum/#!msg/mechanical-sympathy/m4opvy4xq3U/7lY8x8SvHgwJ
 [4] - 
 http://hg.openjdk.java.net/code-tools/jmh/file/adb6047266d8/jmh-samples/src/main/java/org/openjdk/jmh/samples
 [5] - https://github.com/melix/jmh-gradle-plugin
 
 
 Diffs
 -
 
   build.gradle 152ba631e2dd07f0306e58e355274e10a4128140 
   config/findbugs/excludeFilter.xml d6c1b1681c2d8505a088f9fb082ce11ac400126f 
   src/jmh/java/org/apache/aurora/benchmark/SchedulerBenchmark.java 
 PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/28710/diff/
 
 
 Testing
 ---
 
 $ ./gradlew jmh
 
 Sample results generated:
 
 ```
 # VM invoker: 
 /Library/Java/JavaVirtualMachines/jdk1.7.0_25.jdk/Contents/Home/jre/bin/java
 # VM options: -Dfile.encoding=UTF-8 -Duser.country=US -Duser.language=en 
 -Duser.variant
 # Warmup: 1 iterations, 1 s each
 # Measurement: 3 iterations, 1 s each
 # Timeout: 10 min per iteration
 # Threads: 1 thread, will synchronize iterations
 # Benchmark mode: Throughput, ops/time
 # Benchmark: org.apache.aurora.benchmark.SchedulerBenchmark.example
 
 # Run progress: 0.00% complete, ETA 00:00:04
 # Fork: 1 of 1
 # Warmup Iteration   1: 3156839103.911 ops/s
 Iteration   1: 544897.411 ops/s
 Iteration   2: 3357230627.218 ops/s
 Iteration   3: 3461073727.560 ops/s
 
 
 Result: 3383949750.729 ±(99.9%) 1237528915.517 ops/s [Average]
   Statistics: (min, avg, max) = (544897.411, 3383949750.729, 
 3461073727.560), stdev = 67833135.714
   Confidence interval (99.9%): [2146420835.212, 4621478666.247]
 
 
 # Run complete. Total time: 00:00:05
 
 Benchmark  Mode  Samples   Score  
   Error  Units
 o.a.a.b.SchedulerBenchmark.examplethrpt3  3383949750.729 ± 
 1237528915.517  ops/s
 
 ```
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28710: Adding JMH framework support for scheduler performance analysis.


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

(Updated Dec. 10, 2014, 1:22 a.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

CR comments.


Repository: aurora


Description
---

This RB is superseding the https://reviews.apache.org/r/28474/.


I have spent some time researching the available microbenchmark frameworks and 
JMH [1] came as a clear winner:
- Active development trail [2]
- Advanced featureset and built-in optimizations improving accuracy and 
consistency [3]
- Well documented set of examples [4]
- Large community experience and collective wisdom. 


This RB adds gradle support for running JMH benchmarks and is relying on JMH 
gradle plugin [5]. The benchmarks are run via `./gradlew jmh` command.

[1] - http://openjdk.java.net/projects/code-tools/jmh/
[2] - http://hg.openjdk.java.net/code-tools/jmh/ 
[3] - 
https://groups.google.com/forum/#!msg/mechanical-sympathy/m4opvy4xq3U/7lY8x8SvHgwJ
[4] - 
http://hg.openjdk.java.net/code-tools/jmh/file/adb6047266d8/jmh-samples/src/main/java/org/openjdk/jmh/samples
[5] - https://github.com/melix/jmh-gradle-plugin


Diffs (updated)
-

  build.gradle 152ba631e2dd07f0306e58e355274e10a4128140 
  config/findbugs/excludeFilter.xml d6c1b1681c2d8505a088f9fb082ce11ac400126f 
  src/jmh/java/org/apache/aurora/benchmark/SchedulerBenchmark.java PRE-CREATION 

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


Testing
---

$ ./gradlew jmh

Sample results generated:

```
# VM invoker: 
/Library/Java/JavaVirtualMachines/jdk1.7.0_25.jdk/Contents/Home/jre/bin/java
# VM options: -Dfile.encoding=UTF-8 -Duser.country=US -Duser.language=en 
-Duser.variant
# Warmup: 1 iterations, 1 s each
# Measurement: 3 iterations, 1 s each
# Timeout: 10 min per iteration
# Threads: 1 thread, will synchronize iterations
# Benchmark mode: Throughput, ops/time
# Benchmark: org.apache.aurora.benchmark.SchedulerBenchmark.example

# Run progress: 0.00% complete, ETA 00:00:04
# Fork: 1 of 1
# Warmup Iteration   1: 3156839103.911 ops/s
Iteration   1: 544897.411 ops/s
Iteration   2: 3357230627.218 ops/s
Iteration   3: 3461073727.560 ops/s


Result: 3383949750.729 ±(99.9%) 1237528915.517 ops/s [Average]
  Statistics: (min, avg, max) = (544897.411, 3383949750.729, 
3461073727.560), stdev = 67833135.714
  Confidence interval (99.9%): [2146420835.212, 4621478666.247]


# Run complete. Total time: 00:00:05

Benchmark  Mode  Samples   Score
Error  Units
o.a.a.b.SchedulerBenchmark.examplethrpt3  3383949750.729 ± 
1237528915.517  ops/s

```


Thanks,

Maxim Khutornenko

Re: Review Request 28876: Suppressing redundant client command error messaging.

On Dec. 9, 2014, 11:29 p.m., Zameer Manji wrote:
src/main/python/apache/aurora/client/cli/__init__.py, line 384
https://reviews.apache.org/r/28876/diff/1/?file=787343#file787343line384

This solution seems to be papering over the problem we have with output
in the client.

Instead of creating a new type of exception can we just prevent the
double printing in general?

Maxim Khutornenko wrote:
| Perhaps we should take the approach of limiting use of stdout/stderr to
the top of the stack whenever possible?

I am not sure what it really buys us. Right now, the scheduler response is
entirely handled within the check_and_log_response that does centralized
logging to both stdout and stderr. IMO, delegating just stderr handling
elsewhere creates more ambiguity and is not solving any additional problems.

- Maxim

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

On Dec. 9, 2014, 10:32 p.m., Maxim Khutornenko wrote:

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

(Updated Dec. 9, 2014, 10:32 p.m.)

Review request for Aurora, Bill Farner and Zameer Manji.

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

Repository: aurora

Description
---

Added a new error type to raise when errors are logged explicitly.

Diffs
-

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

Testing
---

./pants src/test/python/apache/aurora/client/cli:all

Vagrant before:

Thanks,

Maxim Khutornenko

Review Request 28806: More logging in MaintenanceController.


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

Review request for Aurora and Bill Farner.


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


Repository: aurora


Description
---

More logging in MaintenanceController.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java 
8e6f4e7c18df4bac4dab747f05803e8e25b43cda 

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


Testing
---

./gradlew -Pq build


Thanks,

Maxim Khutornenko

Re: Review Request 28742: Simplify logging in the client.


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



src/main/python/apache/aurora/client/cli/client.py
https://reviews.apache.org/r/28742/#comment106906

Is there a test for this?



src/main/python/apache/aurora/client/cli/context.py
https://reviews.apache.org/r/28742/#comment106904

This can now be simplified with `base.combine_messages(resp)`.


- Maxim Khutornenko


On Dec. 8, 2014, 7:58 p.m., Zameer Manji wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28742/
 ---
 
 (Updated Dec. 8, 2014, 7:58 p.m.)
 
 
 Review request for Aurora, Maxim Khutornenko and Bill Farner.
 
 
 Bugs: AURORA-919
 https://issues.apache.org/jira/browse/AURORA-919
 
 
 Repository: aurora
 
 
 Description
 ---
 
 This patch makes multiple changes to simplify the logging done in the Aurora 
 client:
 1. Remove the TRANSCRIPT log level and replaced all instances with the 
 standard Python DEBUG level.
 2. Remove the custom aurora_client logger. This logger was designed to give 
 each invocation of the client a unique id and record the username of the user 
 with the intention that a hook could take this information and ship it to the 
 cluster administer. However a hook could capture logs by adding a handler to 
 the root log handler and generate a unique id itself.
 3. Remove the 'print_log' method of the context and replaced all callers with 
 the standard python logging facilities.
 4. Removed duplicate printing/logging messages by just printing the 
 information to the user.
 5. Removed the custom PlainFormatter implementation and replaced it with 
 Python's default formatter.
 6. Replaced the --verbose-logging and --logging-level flags with a single 
 --verbose/-v flag which enables DEBUG logging. Without this flag the user 
 sees INFO and up.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/BUILD 
 ebe681a0d1735b7cc695dc3b7a14c4292d87ae32 
   src/main/python/apache/aurora/client/cli/__init__.py 
 6e553d8af459e575b2d62282a3bc0d1e266203d8 
   src/main/python/apache/aurora/client/cli/client.py 
 0cb69448cd24372067ac845eca5862bc3d3a46a9 
   src/main/python/apache/aurora/client/cli/command_hooks.py 
 aa850bf941bede1d3bd8aae4811cb094ba77965f 
   src/main/python/apache/aurora/client/cli/context.py 
 51c7d24dca664e476e62f1864d095416dfab70e4 
   src/main/python/apache/aurora/client/cli/jobs.py 
 8f349c09637c16e2499e85f2dc96eb7ccffd0aaf 
   src/main/python/apache/aurora/client/cli/logsetup.py 
 55d99c42f643910db0bf3c24022596383e160276 
   src/main/python/apache/aurora/client/cli/task.py 
 91175facdc8c9fd59ab66781f86ee8b5940a 
   src/test/python/apache/aurora/client/cli/BUILD 
 e1f9ebf96774b8f5c75de8570c6ba87d953ab649 
   src/test/python/apache/aurora/client/cli/test_logging.py 
 6285fbb07442291c2dc4096e68eb285c98994097 
   src/test/python/apache/aurora/client/cli/test_plugins.py 
 7a0a31818cbc57de952d7817f8e7c8fa1e84b25a 
   src/test/python/apache/aurora/client/cli/test_task.py 
 c69a624ec7063973d365846f7df3516047ceeb68 
 
 Diff: https://reviews.apache.org/r/28742/diff/
 
 
 Testing
 ---
 
 ./pants ./src/test/python/apache/aurora::
 
 
 vagrant@vagrant-ubuntu-trusty-64:~$ aurora2 config list 
 ./aurora/examples/jobs/hello_world.aurora
 jobs=[devcluster/www-data/prod/hello]
 
 vagrant@vagrant-ubuntu-trusty-64:~$ aurora2 job create 
 devcluster/www-data/prod/hello ./aurora/examples/jobs/hello_world.aurora
  INFO] Creating job hello
  INFO] Checking status of devcluster/www-data/prod/hello
 job create succeeded: job 
 url=http://vagrant-ubuntu-trusty-64:8081/scheduler/www-data/prod/hello
 
 vagrant@vagrant-ubuntu-trusty-64:~$ aurora2 job list devcluster/*
  INFO] Retrieving jobs for role None
 devcluster/www-data/prod/hello
 
 vagrant@vagrant-ubuntu-trusty-64:~$ aurora2 job update 
 devcluster/www-data/prod/hello ./aurora/examples/jobs/hello_world.aurora
  INFO] Updating job: hello
  INFO] Instances to update: [0, 1]
  INFO] Processing in parallel with 1 worker thread(s)
  INFO] Examining instance: 0
  INFO] Skipping unchanged instance: 0
  INFO] Examining instance: 1
  INFO] Adding instance: 1
  INFO] Added: 1
  INFO] Watching instances: [1]
  INFO] Instance 1 was not reported healthy within 60 seconds
  INFO] Failed instances: set([1])
  WARN] Not restarting failed instances [1], which exceeded maximum allowed 
 instance failure limit of 0
 ERROR] 1 failed instances observed, maximum allowed is 0
 ERROR] 1 instance failures for instance 1, maximum allowed is 0
  INFO] Reverting update for: [1, 0]
  INFO] Processing in parallel with 1 worker thread(s)
  INFO] Reverting instance: 1
  INFO] Examining instance: 1
  INFO] Killing instance: 1
  INFO] Killed

Review Request 28811: Improving logging experience in admin drain_hosts.


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

Review request for Aurora and Bill Farner.


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


Repository: aurora


Description
---

Improving logging experience in admin drain_hosts.


Diffs
-

  src/main/python/apache/aurora/admin/host_maintenance.py 
bff8afd2b52fdf3977f681a73c97000a38773498 
  src/test/python/apache/aurora/admin/test_host_maintenance.py 
4b8072c0349a9b0905ebb249ed97c7dfe8e8b1de 

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


Testing
---

./pants src/test/python/apache/aurora/admin:host_maintenance


Thanks,

Maxim Khutornenko

Re: Review Request 28811: Improving logging experience in admin drain_hosts.



 On Dec. 8, 2014, 9:21 p.m., Bill Farner wrote:
  src/main/python/apache/aurora/admin/host_maintenance.py, line 85
  https://reviews.apache.org/r/28811/diff/1/?file=785748#file785748line85
 
  Should this be:
  
  `self.check_if_drained(drainable_hostnames)`

  It seems strange that if we're watching 2 hosts, and 1 moves to 
  DRAINED, we keep querying for its state.
  
  Coupled with the logging change in this diff, it appears that you'll 
  keep logging the same hostnames over and over.

Not sure I understand. The `not_drained_hostnames =` assignment takes care of 
progressively reducing the logged hostset, so a DRAINED host will never show up 
in any of the subsequent iterations.


 On Dec. 8, 2014, 9:21 p.m., Bill Farner wrote:
  src/main/python/apache/aurora/admin/host_maintenance.py, line 61
  https://reviews.apache.org/r/28811/diff/1/?file=785748#file785748line61
 
  Can you elaborate why this is an improvement?  AFAICT the former 
  messaging was more accurate - we print out the hosts that you're waiting 
  for, and will be queried/tracked.
  
  This change might make sense if we did _not_ do the initial DRAINED 
  filtering here, and allow the caller to report that these hosts are ready.
  
  Finally - you should consider inlining this function - there's hardly 
  any behavior here, and there's only one caller.

The current log placement may generate an empty message when draining is done 
faster than the initial wait interval, giving a somewhat confusing user 
experience. The reordering here makes sure a user sees the initial draining 
batch in the log at least once. I am fine with inlining if it makes things 
easier to read.


- Maxim


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


On Dec. 8, 2014, 9:13 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28811/
 ---
 
 (Updated Dec. 8, 2014, 9:13 p.m.)
 
 
 Review request for Aurora and Bill Farner.
 
 
 Bugs: AURORA-943
 https://issues.apache.org/jira/browse/AURORA-943
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Improving logging experience in admin drain_hosts.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/admin/host_maintenance.py 
 bff8afd2b52fdf3977f681a73c97000a38773498 
   src/test/python/apache/aurora/admin/test_host_maintenance.py 
 4b8072c0349a9b0905ebb249ed97c7dfe8e8b1de 
 
 Diff: https://reviews.apache.org/r/28811/diff/
 
 
 Testing
 ---
 
 ./pants src/test/python/apache/aurora/admin:host_maintenance
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28811: Improving logging experience in admin drain_hosts.



 On Dec. 8, 2014, 9:21 p.m., Bill Farner wrote:
  src/main/python/apache/aurora/admin/host_maintenance.py, line 85
  https://reviews.apache.org/r/28811/diff/1/?file=785748#file785748line85
 
  Should this be:
  
  `self.check_if_drained(drainable_hostnames)`

  It seems strange that if we're watching 2 hosts, and 1 moves to 
  DRAINED, we keep querying for its state.
  
  Coupled with the logging change in this diff, it appears that you'll 
  keep logging the same hostnames over and over.
 
 Maxim Khutornenko wrote:
 Not sure I understand. The `not_drained_hostnames =` assignment takes 
 care of progressively reducing the logged hostset, so a DRAINED host will 
 never show up in any of the subsequent iterations.

Got it now, you probably meant `not_drained_hostnames` as a parameter there. 
Makes sense.


- Maxim


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


On Dec. 8, 2014, 9:13 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28811/
 ---
 
 (Updated Dec. 8, 2014, 9:13 p.m.)
 
 
 Review request for Aurora and Bill Farner.
 
 
 Bugs: AURORA-943
 https://issues.apache.org/jira/browse/AURORA-943
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Improving logging experience in admin drain_hosts.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/admin/host_maintenance.py 
 bff8afd2b52fdf3977f681a73c97000a38773498 
   src/test/python/apache/aurora/admin/test_host_maintenance.py 
 4b8072c0349a9b0905ebb249ed97c7dfe8e8b1de 
 
 Diff: https://reviews.apache.org/r/28811/diff/
 
 
 Testing
 ---
 
 ./pants src/test/python/apache/aurora/admin:host_maintenance
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28811: Improving logging experience in admin drain_hosts.


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

(Updated Dec. 8, 2014, 10:43 p.m.)


Review request for Aurora and Bill Farner.


Changes
---

CR comments.


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


Repository: aurora


Description
---

Improving logging experience in admin drain_hosts.


Diffs (updated)
-

  src/main/python/apache/aurora/admin/host_maintenance.py 
bff8afd2b52fdf3977f681a73c97000a38773498 
  src/test/python/apache/aurora/admin/test_host_maintenance.py 
4b8072c0349a9b0905ebb249ed97c7dfe8e8b1de 

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


Testing
---

./pants src/test/python/apache/aurora/admin:host_maintenance


Thanks,

Maxim Khutornenko

Re: Review Request 28617: Implemented offer filtering for tasks with static vetoes.



 On Dec. 8, 2014, 7:29 p.m., Kevin Sweeney wrote:
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, 
  line 102
  https://reviews.apache.org/r/28617/diff/2/?file=784162#file784162line102
 
  Can you add this as a member to the Veto interface instead? This will 
  ensure we don't add new Veto types without updating this map.
  
  Like
  
  ```java
  interface Veto {
VetoGroup getVetoGroup();
  }
  ```

There is no Veto interface and I am hesitant to create one just for this 
method. I have moved the group assignment into the VetoType instead. That also 
helped to get rid of a null check code not reachable from unit tests.


 On Dec. 8, 2014, 7:29 p.m., Kevin Sweeney wrote:
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, 
  line 181
  https://reviews.apache.org/r/28617/diff/2/?file=784162#file784162line181
 
  If you follow the comment above this block becomes unnecessary.

Gone.


- Maxim


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


On Dec. 5, 2014, 10:57 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28617/
 ---
 
 (Updated Dec. 5, 2014, 10:57 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
 ---
 
 Modified the task offer/task matching logic to skip offer matching for tasks 
 previously vetoed statically.
 
 Preliminary testing in vagrant (see pictures below) shows close to 50% 
 improvement in task scheduling performance.
 
 Update:
 Testing with JMH (https://reviews.apache.org/r/28710/ and 
 https://reviews.apache.org/r/28731/) shows over 97% better perf:
 
 ```
 Master with cluster fillup 0.9:
 Benchmark 
Mode  SamplesScoreError  Units
 o.a.a.b.SchedulingBenchmarks.ConstraintMismatchsSchedulingBenchmark.example   
avgt  100  8291046.074 Â± 145251.995  ns/op
 o.a.a.b.SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.example 
avgt  100  7522269.050 Â± 142446.265  ns/op
 
 This RB with cluster fillup 0.9:
 Benchmark 
Mode  Samples   Score  Error  Units
 o.a.a.b.SchedulingBenchmarks.ConstraintMismatchsSchedulingBenchmark.example   
avgt  100  204171.046 Â± 3800.124  ns/op
 o.a.a.b.SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.example 
avgt  100  215854.129 Â± 8959.851  ns/op
 ```
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/TaskVars.java 
 f017cdd26ca40138a7e141f21613ed567314c399 
   src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 
 f66383830140e5eaba436f35ebb5192eee65947a 
   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
 ead9d28100673440168a32d114ecaa15874978a6 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
 c2a342ce07bfb223193886038761f0da5230135d 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
 1cb56f19c331508a1585077e9c4a98f52aac343b 
   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
 e1c29747c9854cf75bf63f6f085cf40ca68989af 
   src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
 4e7efb3c1214c3d193afd61f162713490eb8effb 
   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/filter/SchedulingFilterImplTest.java
  265c38d20136210e7639ac8ea915d307a4b72949 
   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
 411a55a8d85f60bb2703468f2d69b64b2736eee4 
 
 Diff: https://reviews.apache.org/r/28617/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 Tested in vagrant
 
 
 File Attachments
 
 
 NoStaticVetoFiltering.png
   
 https://reviews.apache.org/media/uploaded/files/2014/12/03/7945c60b-4135-4016-a9bf-8d4815a4a573__NoStaticVetoFiltering.png
 StaticVetoFiltering.png
   
 https://reviews.apache.org/media/uploaded/files/2014/12/03/2f73b94a-5ba9-43b6-922e-e9e4ec18d0bb__StaticVetoFiltering.png
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28617: Implemented offer filtering for tasks with static vetoes.


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

(Updated Dec. 8, 2014, 10:57 p.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

CR comments.


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


Repository: aurora


Description
---

Modified the task offer/task matching logic to skip offer matching for tasks 
previously vetoed statically.

Preliminary testing in vagrant (see pictures below) shows close to 50% 
improvement in task scheduling performance.

Update:
Testing with JMH (https://reviews.apache.org/r/28710/ and 
https://reviews.apache.org/r/28731/) shows over 97% better perf:

```
Master with cluster fillup 0.9:
Benchmark   
 Mode  SamplesScoreError  Units
o.a.a.b.SchedulingBenchmarks.ConstraintMismatchsSchedulingBenchmark.example 
 avgt  100  8291046.074 Â± 145251.995  ns/op
o.a.a.b.SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.example   
 avgt  100  7522269.050 Â± 142446.265  ns/op

This RB with cluster fillup 0.9:
Benchmark   
 Mode  Samples   Score  Error  Units
o.a.a.b.SchedulingBenchmarks.ConstraintMismatchsSchedulingBenchmark.example 
 avgt  100  204171.046 Â± 3800.124  ns/op
o.a.a.b.SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.example   
 avgt  100  215854.129 Â± 8959.851  ns/op
```


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/TaskVars.java 
f017cdd26ca40138a7e141f21613ed567314c399 
  src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 
f66383830140e5eaba436f35ebb5192eee65947a 
  src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
ead9d28100673440168a32d114ecaa15874978a6 
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
c2a342ce07bfb223193886038761f0da5230135d 
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
1cb56f19c331508a1585077e9c4a98f52aac343b 
  src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
e1c29747c9854cf75bf63f6f085cf40ca68989af 
  src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
4e7efb3c1214c3d193afd61f162713490eb8effb 
  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/filter/SchedulingFilterImplTest.java 
265c38d20136210e7639ac8ea915d307a4b72949 
  src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
411a55a8d85f60bb2703468f2d69b64b2736eee4 

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


Testing
---

./gradlew -Pq build

Tested in vagrant


File Attachments


NoStaticVetoFiltering.png
  
https://reviews.apache.org/media/uploaded/files/2014/12/03/7945c60b-4135-4016-a9bf-8d4815a4a573__NoStaticVetoFiltering.png
StaticVetoFiltering.png
  
https://reviews.apache.org/media/uploaded/files/2014/12/03/2f73b94a-5ba9-43b6-922e-e9e4ec18d0bb__StaticVetoFiltering.png


Thanks,

Maxim Khutornenko

Re: Review Request 28742: Simplify logging in the client.


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

Ship it!


Ship It!

- Maxim Khutornenko


On Dec. 8, 2014, 11:18 p.m., Zameer Manji wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28742/
 ---
 
 (Updated Dec. 8, 2014, 11:18 p.m.)
 
 
 Review request for Aurora, Maxim Khutornenko and Bill Farner.
 
 
 Bugs: AURORA-919
 https://issues.apache.org/jira/browse/AURORA-919
 
 
 Repository: aurora
 
 
 Description
 ---
 
 This patch makes multiple changes to simplify the logging done in the Aurora 
 client:
 1. Remove the TRANSCRIPT log level and replaced all instances with the 
 standard Python DEBUG level.
 2. Remove the custom aurora_client logger. This logger was designed to give 
 each invocation of the client a unique id and record the username of the user 
 with the intention that a hook could take this information and ship it to the 
 cluster administer. However a hook could capture logs by adding a handler to 
 the root log handler and generate a unique id itself.
 3. Remove the 'print_log' method of the context and replaced all callers with 
 the standard python logging facilities.
 4. Removed duplicate printing/logging messages by just printing the 
 information to the user.
 5. Removed the custom PlainFormatter implementation and replaced it with 
 Python's default formatter.
 6. Replaced the --verbose-logging and --logging-level flags with a single 
 --verbose/-v flag which enables DEBUG logging. Without this flag the user 
 sees INFO and up.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/BUILD 
 ebe681a0d1735b7cc695dc3b7a14c4292d87ae32 
   src/main/python/apache/aurora/client/cli/__init__.py 
 6e553d8af459e575b2d62282a3bc0d1e266203d8 
   src/main/python/apache/aurora/client/cli/client.py 
 0cb69448cd24372067ac845eca5862bc3d3a46a9 
   src/main/python/apache/aurora/client/cli/command_hooks.py 
 aa850bf941bede1d3bd8aae4811cb094ba77965f 
   src/main/python/apache/aurora/client/cli/context.py 
 51c7d24dca664e476e62f1864d095416dfab70e4 
   src/main/python/apache/aurora/client/cli/jobs.py 
 8f349c09637c16e2499e85f2dc96eb7ccffd0aaf 
   src/main/python/apache/aurora/client/cli/logsetup.py 
 55d99c42f643910db0bf3c24022596383e160276 
   src/main/python/apache/aurora/client/cli/task.py 
 91175facdc8c9fd59ab66781f86ee8b5940a 
   src/test/python/apache/aurora/client/cli/BUILD 
 e1f9ebf96774b8f5c75de8570c6ba87d953ab649 
   src/test/python/apache/aurora/client/cli/test_client.py PRE-CREATION 
   src/test/python/apache/aurora/client/cli/test_logging.py 
 6285fbb07442291c2dc4096e68eb285c98994097 
   src/test/python/apache/aurora/client/cli/test_plugins.py 
 7a0a31818cbc57de952d7817f8e7c8fa1e84b25a 
   src/test/python/apache/aurora/client/cli/test_task.py 
 c69a624ec7063973d365846f7df3516047ceeb68 
 
 Diff: https://reviews.apache.org/r/28742/diff/
 
 
 Testing
 ---
 
 ./pants ./src/test/python/apache/aurora::
 
 
 vagrant@vagrant-ubuntu-trusty-64:~$ aurora2 config list 
 ./aurora/examples/jobs/hello_world.aurora
 jobs=[devcluster/www-data/prod/hello]
 
 vagrant@vagrant-ubuntu-trusty-64:~$ aurora2 job create 
 devcluster/www-data/prod/hello ./aurora/examples/jobs/hello_world.aurora
  INFO] Creating job hello
  INFO] Checking status of devcluster/www-data/prod/hello
 job create succeeded: job 
 url=http://vagrant-ubuntu-trusty-64:8081/scheduler/www-data/prod/hello
 
 vagrant@vagrant-ubuntu-trusty-64:~$ aurora2 job list devcluster/*
  INFO] Retrieving jobs for role None
 devcluster/www-data/prod/hello
 
 vagrant@vagrant-ubuntu-trusty-64:~$ aurora2 job update 
 devcluster/www-data/prod/hello ./aurora/examples/jobs/hello_world.aurora
  INFO] Updating job: hello
  INFO] Instances to update: [0, 1]
  INFO] Processing in parallel with 1 worker thread(s)
  INFO] Examining instance: 0
  INFO] Skipping unchanged instance: 0
  INFO] Examining instance: 1
  INFO] Adding instance: 1
  INFO] Added: 1
  INFO] Watching instances: [1]
  INFO] Instance 1 was not reported healthy within 60 seconds
  INFO] Failed instances: set([1])
  WARN] Not restarting failed instances [1], which exceeded maximum allowed 
 instance failure limit of 0
 ERROR] 1 failed instances observed, maximum allowed is 0
 ERROR] 1 instance failures for instance 1, maximum allowed is 0
  INFO] Reverting update for: [1, 0]
  INFO] Processing in parallel with 1 worker thread(s)
  INFO] Reverting instance: 1
  INFO] Examining instance: 1
  INFO] Killing instance: 1
  INFO] Killed: 1
  INFO] Reverting instance: 0
  INFO] Examining instance: 0
  INFO] Skipping unchanged instance: 0
  WARN] Update failures threshold reached
 Update failed due to error:
   Update reverted
 Error executing

Re: Review Request 28742: Simplify logging in the client.


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


On master now.

- Maxim Khutornenko


On Dec. 8, 2014, 11:18 p.m., Zameer Manji wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28742/
 ---
 
 (Updated Dec. 8, 2014, 11:18 p.m.)
 
 
 Review request for Aurora, Maxim Khutornenko and Bill Farner.
 
 
 Bugs: AURORA-919
 https://issues.apache.org/jira/browse/AURORA-919
 
 
 Repository: aurora
 
 
 Description
 ---
 
 This patch makes multiple changes to simplify the logging done in the Aurora 
 client:
 1. Remove the TRANSCRIPT log level and replaced all instances with the 
 standard Python DEBUG level.
 2. Remove the custom aurora_client logger. This logger was designed to give 
 each invocation of the client a unique id and record the username of the user 
 with the intention that a hook could take this information and ship it to the 
 cluster administer. However a hook could capture logs by adding a handler to 
 the root log handler and generate a unique id itself.
 3. Remove the 'print_log' method of the context and replaced all callers with 
 the standard python logging facilities.
 4. Removed duplicate printing/logging messages by just printing the 
 information to the user.
 5. Removed the custom PlainFormatter implementation and replaced it with 
 Python's default formatter.
 6. Replaced the --verbose-logging and --logging-level flags with a single 
 --verbose/-v flag which enables DEBUG logging. Without this flag the user 
 sees INFO and up.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/BUILD 
 ebe681a0d1735b7cc695dc3b7a14c4292d87ae32 
   src/main/python/apache/aurora/client/cli/__init__.py 
 6e553d8af459e575b2d62282a3bc0d1e266203d8 
   src/main/python/apache/aurora/client/cli/client.py 
 0cb69448cd24372067ac845eca5862bc3d3a46a9 
   src/main/python/apache/aurora/client/cli/command_hooks.py 
 aa850bf941bede1d3bd8aae4811cb094ba77965f 
   src/main/python/apache/aurora/client/cli/context.py 
 51c7d24dca664e476e62f1864d095416dfab70e4 
   src/main/python/apache/aurora/client/cli/jobs.py 
 8f349c09637c16e2499e85f2dc96eb7ccffd0aaf 
   src/main/python/apache/aurora/client/cli/logsetup.py 
 55d99c42f643910db0bf3c24022596383e160276 
   src/main/python/apache/aurora/client/cli/task.py 
 91175facdc8c9fd59ab66781f86ee8b5940a 
   src/test/python/apache/aurora/client/cli/BUILD 
 e1f9ebf96774b8f5c75de8570c6ba87d953ab649 
   src/test/python/apache/aurora/client/cli/test_client.py PRE-CREATION 
   src/test/python/apache/aurora/client/cli/test_logging.py 
 6285fbb07442291c2dc4096e68eb285c98994097 
   src/test/python/apache/aurora/client/cli/test_plugins.py 
 7a0a31818cbc57de952d7817f8e7c8fa1e84b25a 
   src/test/python/apache/aurora/client/cli/test_task.py 
 c69a624ec7063973d365846f7df3516047ceeb68 
 
 Diff: https://reviews.apache.org/r/28742/diff/
 
 
 Testing
 ---
 
 ./pants ./src/test/python/apache/aurora::
 
 
 vagrant@vagrant-ubuntu-trusty-64:~$ aurora2 config list 
 ./aurora/examples/jobs/hello_world.aurora
 jobs=[devcluster/www-data/prod/hello]
 
 vagrant@vagrant-ubuntu-trusty-64:~$ aurora2 job create 
 devcluster/www-data/prod/hello ./aurora/examples/jobs/hello_world.aurora
  INFO] Creating job hello
  INFO] Checking status of devcluster/www-data/prod/hello
 job create succeeded: job 
 url=http://vagrant-ubuntu-trusty-64:8081/scheduler/www-data/prod/hello
 
 vagrant@vagrant-ubuntu-trusty-64:~$ aurora2 job list devcluster/*
  INFO] Retrieving jobs for role None
 devcluster/www-data/prod/hello
 
 vagrant@vagrant-ubuntu-trusty-64:~$ aurora2 job update 
 devcluster/www-data/prod/hello ./aurora/examples/jobs/hello_world.aurora
  INFO] Updating job: hello
  INFO] Instances to update: [0, 1]
  INFO] Processing in parallel with 1 worker thread(s)
  INFO] Examining instance: 0
  INFO] Skipping unchanged instance: 0
  INFO] Examining instance: 1
  INFO] Adding instance: 1
  INFO] Added: 1
  INFO] Watching instances: [1]
  INFO] Instance 1 was not reported healthy within 60 seconds
  INFO] Failed instances: set([1])
  WARN] Not restarting failed instances [1], which exceeded maximum allowed 
 instance failure limit of 0
 ERROR] 1 failed instances observed, maximum allowed is 0
 ERROR] 1 instance failures for instance 1, maximum allowed is 0
  INFO] Reverting update for: [1, 0]
  INFO] Processing in parallel with 1 worker thread(s)
  INFO] Reverting instance: 1
  INFO] Examining instance: 1
  INFO] Killing instance: 1
  INFO] Killed: 1
  INFO] Reverting instance: 0
  INFO] Examining instance: 0
  INFO] Skipping unchanged instance: 0
  WARN] Update failures threshold reached
 Update failed due to error:
   Update reverted
 Error executing

Review Request 28831: Changing the default --batch-size to 1.


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

Review request for Aurora and Bill Farner.


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


Repository: aurora


Description
---

Also, cleaned up kill command tests a bit.


Diffs
-

  src/main/python/apache/aurora/client/cli/options.py 
e844cf340583b631ce194352f403bbaec71655b7 
  src/test/python/apache/aurora/client/cli/test_kill.py 
1eda72af4c19831ae27733f506858e67772b2075 
  src/test/python/apache/aurora/client/cli/util.py 
67d7eaa6eff4e1dbaaa485166e084812a4f04074 

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


Testing
---

./pants src/test/python/apache/aurora/client/cli:all


Thanks,

Maxim Khutornenko

Re: Review Request 28738: Remove unused DefaultServlet subclass.


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

Ship it!


Ship It!

- Maxim Khutornenko


On Dec. 5, 2014, 4:06 a.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28738/
 ---
 
 (Updated Dec. 5, 2014, 4:06 a.m.)
 
 
 Review request for Aurora and Kevin Sweeney.
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Remove unused DefaultServlet subclass.
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
 424859867d7bd1aafefe406f455b831246c1cca5 
 
 Diff: https://reviews.apache.org/r/28738/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Bill Farner

Re: Review Request 28710: Adding JMH framework support for scheduler performance analysis.


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

(Updated Dec. 5, 2014, 6:32 p.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

Rebased.


Repository: aurora


Description
---

This RB is superseding the https://reviews.apache.org/r/28474/.


I have spent some time researching the available microbenchmark frameworks and 
JMH [1] came as a clear winner:
- Active development trail [2]
- Advanced featureset and built-in optimizations improving accuracy and 
consistency [3]
- Well documented set of examples [4]
- Large community experience and collective wisdom. 


This RB adds gradle support for running JMH benchmarks and is relying on JMH 
gradle plugin [5]. The benchmarks are run via `./gradlew jmh` command.

[1] - http://openjdk.java.net/projects/code-tools/jmh/
[2] - http://hg.openjdk.java.net/code-tools/jmh/ 
[3] - 
https://groups.google.com/forum/#!msg/mechanical-sympathy/m4opvy4xq3U/7lY8x8SvHgwJ
[4] - 
http://hg.openjdk.java.net/code-tools/jmh/file/adb6047266d8/jmh-samples/src/main/java/org/openjdk/jmh/samples
[5] - https://github.com/melix/jmh-gradle-plugin


Diffs (updated)
-

  build.gradle 152ba631e2dd07f0306e58e355274e10a4128140 
  config/findbugs/excludeFilter.xml d6c1b1681c2d8505a088f9fb082ce11ac400126f 
  src/jmh/java/org/apache/aurora/benchmark/SchedulerBenchmark.java PRE-CREATION 

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


Testing
---

$ ./gradlew jmh

Sample results generated:

```
# VM invoker: 
/Library/Java/JavaVirtualMachines/jdk1.7.0_25.jdk/Contents/Home/jre/bin/java
# VM options: -Dfile.encoding=UTF-8 -Duser.country=US -Duser.language=en 
-Duser.variant
# Warmup: 1 iterations, 1 s each
# Measurement: 3 iterations, 1 s each
# Timeout: 10 min per iteration
# Threads: 1 thread, will synchronize iterations
# Benchmark mode: Throughput, ops/time
# Benchmark: org.apache.aurora.benchmark.SchedulerBenchmark.example

# Run progress: 0.00% complete, ETA 00:00:04
# Fork: 1 of 1
# Warmup Iteration   1: 3156839103.911 ops/s
Iteration   1: 544897.411 ops/s
Iteration   2: 3357230627.218 ops/s
Iteration   3: 3461073727.560 ops/s


Result: 3383949750.729 ±(99.9%) 1237528915.517 ops/s [Average]
  Statistics: (min, avg, max) = (544897.411, 3383949750.729, 
3461073727.560), stdev = 67833135.714
  Confidence interval (99.9%): [2146420835.212, 4621478666.247]


# Run complete. Total time: 00:00:05

Benchmark  Mode  Samples   Score
Error  Units
o.a.a.b.SchedulerBenchmark.examplethrpt3  3383949750.729 ± 
1237528915.517  ops/s

```


Thanks,

Maxim Khutornenko

Re: Review Request 28710: Adding JMH framework support for scheduler performance analysis.



 On Dec. 5, 2014, 8:06 p.m., Bill Farner wrote:
  Not to bikeshed, but do either of you have a sense for what it would take 
  to put the benchmarking code into a gradle subproject?  My sense is that 
  would avoid further complicating the root build.gradle, but i'm not 
  familiar enough with gradle to know what, if any, hurdles that would 
  present.

There is not much to hide in a subproject as all you see here is configuring 
the plugin to run with our gradle. I don't see any sane way of hiding it in a 
subproject short of reimplementing the plugin ourselves.


- Maxim


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


On Dec. 5, 2014, 6:32 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28710/
 ---
 
 (Updated Dec. 5, 2014, 6:32 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Repository: aurora
 
 
 Description
 ---
 
 This RB is superseding the https://reviews.apache.org/r/28474/.
 
 
 I have spent some time researching the available microbenchmark frameworks 
 and JMH [1] came as a clear winner:
 - Active development trail [2]
 - Advanced featureset and built-in optimizations improving accuracy and 
 consistency [3]
 - Well documented set of examples [4]
 - Large community experience and collective wisdom. 
 
 
 This RB adds gradle support for running JMH benchmarks and is relying on JMH 
 gradle plugin [5]. The benchmarks are run via `./gradlew jmh` command.
 
 [1] - http://openjdk.java.net/projects/code-tools/jmh/
 [2] - http://hg.openjdk.java.net/code-tools/jmh/ 
 [3] - 
 https://groups.google.com/forum/#!msg/mechanical-sympathy/m4opvy4xq3U/7lY8x8SvHgwJ
 [4] - 
 http://hg.openjdk.java.net/code-tools/jmh/file/adb6047266d8/jmh-samples/src/main/java/org/openjdk/jmh/samples
 [5] - https://github.com/melix/jmh-gradle-plugin
 
 
 Diffs
 -
 
   build.gradle 152ba631e2dd07f0306e58e355274e10a4128140 
   config/findbugs/excludeFilter.xml d6c1b1681c2d8505a088f9fb082ce11ac400126f 
   src/jmh/java/org/apache/aurora/benchmark/SchedulerBenchmark.java 
 PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/28710/diff/
 
 
 Testing
 ---
 
 $ ./gradlew jmh
 
 Sample results generated:
 
 ```
 # VM invoker: 
 /Library/Java/JavaVirtualMachines/jdk1.7.0_25.jdk/Contents/Home/jre/bin/java
 # VM options: -Dfile.encoding=UTF-8 -Duser.country=US -Duser.language=en 
 -Duser.variant
 # Warmup: 1 iterations, 1 s each
 # Measurement: 3 iterations, 1 s each
 # Timeout: 10 min per iteration
 # Threads: 1 thread, will synchronize iterations
 # Benchmark mode: Throughput, ops/time
 # Benchmark: org.apache.aurora.benchmark.SchedulerBenchmark.example
 
 # Run progress: 0.00% complete, ETA 00:00:04
 # Fork: 1 of 1
 # Warmup Iteration   1: 3156839103.911 ops/s
 Iteration   1: 544897.411 ops/s
 Iteration   2: 3357230627.218 ops/s
 Iteration   3: 3461073727.560 ops/s
 
 
 Result: 3383949750.729 ±(99.9%) 1237528915.517 ops/s [Average]
   Statistics: (min, avg, max) = (544897.411, 3383949750.729, 
 3461073727.560), stdev = 67833135.714
   Confidence interval (99.9%): [2146420835.212, 4621478666.247]
 
 
 # Run complete. Total time: 00:00:05
 
 Benchmark  Mode  Samples   Score  
   Error  Units
 o.a.a.b.SchedulerBenchmark.examplethrpt3  3383949750.729 ± 
 1237528915.517  ops/s
 
 ```
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28771: Don't fall back to old command syntax in the new client.


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

Ship it!


Ship It!

- Maxim Khutornenko


On Dec. 5, 2014, 9:12 p.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28771/
 ---
 
 (Updated Dec. 5, 2014, 9:12 p.m.)
 
 
 Review request for Aurora, Maxim Khutornenko and Zameer Manji.
 
 
 Bugs: AURORA-782
 https://issues.apache.org/jira/browse/AURORA-782
 
 
 Repository: aurora
 
 
 Description
 ---
 
 The diff appears like a large delta to `cli/client.py`, but it is in fact a 
 pure `git mv` of standalone_client.py to client.py, since the qualification 
 no longer makes sense.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/BUILD 
 8d34bf71a225210f1fef8b5a69bc9f14b49d17bb 
   src/main/python/apache/aurora/client/cli/client.py 
 a1df788adf2c8066d207e6b70d7694f6975b8edd 
   src/main/python/apache/aurora/client/cli/standalone_client.py 
 93120a15f5e1967b4067d3c068b9bc564f3fb432 
 
 Diff: https://reviews.apache.org/r/28771/diff/
 
 
 Testing
 ---
 
 ```
 ./pants build src/test/python/apache/aurora/client:all -vxs
 ./src/test/sh/org/apache/aurora/e2e/test_end_to_end_v2.sh
 ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
 ```
 
 
 Thanks,
 
 Bill Farner

Re: Review Request 28617: Implemented offer filtering for tasks with static vetoes.


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

(Updated Dec. 5, 2014, 10:57 p.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

Improving banned offer lookup performance.


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


Repository: aurora


Description (updated)
---

Modified the task offer/task matching logic to skip offer matching for tasks 
previously vetoed statically.

Preliminary testing in vagrant (see pictures below) shows close to 50% 
improvement in task scheduling performance.

Update:
Testing with JMH (https://reviews.apache.org/r/28710/ and 
https://reviews.apache.org/r/28731/) shows over 97% better perf:

```
Master with cluster fillup 0.9:
Benchmark   
 Mode  SamplesScoreError  Units
o.a.a.b.SchedulingBenchmarks.ConstraintMismatchsSchedulingBenchmark.example 
 avgt  100  8291046.074 Â± 145251.995  ns/op
o.a.a.b.SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.example   
 avgt  100  7522269.050 Â± 142446.265  ns/op

This RB with cluster fillup 0.9:
Benchmark   
 Mode  Samples   Score  Error  Units
o.a.a.b.SchedulingBenchmarks.ConstraintMismatchsSchedulingBenchmark.example 
 avgt  100  204171.046 Â± 3800.124  ns/op
o.a.a.b.SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.example   
 avgt  100  215854.129 Â± 8959.851  ns/op
```


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/TaskVars.java 
f017cdd26ca40138a7e141f21613ed567314c399 
  src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 
f66383830140e5eaba436f35ebb5192eee65947a 
  src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
ead9d28100673440168a32d114ecaa15874978a6 
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
c2a342ce07bfb223193886038761f0da5230135d 
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
1cb56f19c331508a1585077e9c4a98f52aac343b 
  src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
e1c29747c9854cf75bf63f6f085cf40ca68989af 
  src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
4e7efb3c1214c3d193afd61f162713490eb8effb 
  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/filter/SchedulingFilterImplTest.java 
265c38d20136210e7639ac8ea915d307a4b72949 
  src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
411a55a8d85f60bb2703468f2d69b64b2736eee4 

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


Testing
---

./gradlew -Pq build

Tested in vagrant


File Attachments


NoStaticVetoFiltering.png
  
https://reviews.apache.org/media/uploaded/files/2014/12/03/7945c60b-4135-4016-a9bf-8d4815a4a573__NoStaticVetoFiltering.png
StaticVetoFiltering.png
  
https://reviews.apache.org/media/uploaded/files/2014/12/03/2f73b94a-5ba9-43b6-922e-e9e4ec18d0bb__StaticVetoFiltering.png


Thanks,

Maxim Khutornenko

Re: Review Request 28731: Implemented TaskScheduler benchmarks.

  93: 5985075.676 ns/op
Iteration  94: 4735798.283 ns/op
Iteration  95: 4893389.381 ns/op
Iteration  96: 6055005.495 ns/op
Iteration  97: 4684194.915 ns/op
Iteration  98: 4649130.252 ns/op
Iteration  99: 4619602.510 ns/op
Iteration 100: 4593595.833 ns/op


Result: 5011738.309 Â±(99.9%) 160249.620 ns/op [Average]
  Statistics: (min, avg, max) = (4527586.066, 5011738.309, 7052592.357), stdev 
= 472499.654
  Confidence interval (99.9%): [4851488.689, 5171987.930]


# Run complete. Total time: 00:04:29

Benchmark   
 Mode  SamplesScoreError  Units
o.a.a.b.SchedulingBenchmarks.ConstraintMismatchsSchedulingBenchmark.example 
 avgt  100  5165386.898 Â± 344576.928  ns/op
o.a.a.b.SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.example   
 avgt  100  5011738.309 Â± 160249.620  ns/op
```


Thanks,

Maxim Khutornenko

Re: Review Request 28710: Adding JMH framework support for scheduler performance analysis.


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

(Updated Dec. 6, 2014, 12:33 a.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

CR comments.


Repository: aurora


Description
---

This RB is superseding the https://reviews.apache.org/r/28474/.


I have spent some time researching the available microbenchmark frameworks and 
JMH [1] came as a clear winner:
- Active development trail [2]
- Advanced featureset and built-in optimizations improving accuracy and 
consistency [3]
- Well documented set of examples [4]
- Large community experience and collective wisdom. 


This RB adds gradle support for running JMH benchmarks and is relying on JMH 
gradle plugin [5]. The benchmarks are run via `./gradlew jmh` command.

[1] - http://openjdk.java.net/projects/code-tools/jmh/
[2] - http://hg.openjdk.java.net/code-tools/jmh/ 
[3] - 
https://groups.google.com/forum/#!msg/mechanical-sympathy/m4opvy4xq3U/7lY8x8SvHgwJ
[4] - 
http://hg.openjdk.java.net/code-tools/jmh/file/adb6047266d8/jmh-samples/src/main/java/org/openjdk/jmh/samples
[5] - https://github.com/melix/jmh-gradle-plugin


Diffs (updated)
-

  build.gradle 152ba631e2dd07f0306e58e355274e10a4128140 
  config/findbugs/excludeFilter.xml d6c1b1681c2d8505a088f9fb082ce11ac400126f 
  src/jmh/java/org/apache/aurora/benchmark/SchedulerBenchmark.java PRE-CREATION 

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


Testing
---

$ ./gradlew jmh

Sample results generated:

```
# VM invoker: 
/Library/Java/JavaVirtualMachines/jdk1.7.0_25.jdk/Contents/Home/jre/bin/java
# VM options: -Dfile.encoding=UTF-8 -Duser.country=US -Duser.language=en 
-Duser.variant
# Warmup: 1 iterations, 1 s each
# Measurement: 3 iterations, 1 s each
# Timeout: 10 min per iteration
# Threads: 1 thread, will synchronize iterations
# Benchmark mode: Throughput, ops/time
# Benchmark: org.apache.aurora.benchmark.SchedulerBenchmark.example

# Run progress: 0.00% complete, ETA 00:00:04
# Fork: 1 of 1
# Warmup Iteration   1: 3156839103.911 ops/s
Iteration   1: 544897.411 ops/s
Iteration   2: 3357230627.218 ops/s
Iteration   3: 3461073727.560 ops/s


Result: 3383949750.729 ±(99.9%) 1237528915.517 ops/s [Average]
  Statistics: (min, avg, max) = (544897.411, 3383949750.729, 
3461073727.560), stdev = 67833135.714
  Confidence interval (99.9%): [2146420835.212, 4621478666.247]


# Run complete. Total time: 00:00:05

Benchmark  Mode  Samples   Score
Error  Units
o.a.a.b.SchedulerBenchmark.examplethrpt3  3383949750.729 ± 
1237528915.517  ops/s

```


Thanks,

Maxim Khutornenko

Re: Review Request 28710: Adding JMH framework support for scheduler performance analysis.



 On Dec. 5, 2014, 7:42 p.m., Kevin Sweeney wrote:
  build.gradle, line 53
  https://reviews.apache.org/r/28710/diff/1/?file=782698#file782698line53
 
  This should be unneeded.

Dropped.


 On Dec. 5, 2014, 7:42 p.m., Kevin Sweeney wrote:
  build.gradle, line 103
  https://reviews.apache.org/r/28710/diff/3/?file=783833#file783833line103
 
  im not convinced you want to apply this to the api subproject.

This was only needed to make :api recognize .jmh configuration. However, played 
a bit more and found a way to reference parent configuration. Dropped.


 On Dec. 5, 2014, 7:42 p.m., Kevin Sweeney wrote:
  build.gradle, line 156
  https://reviews.apache.org/r/28710/diff/3/?file=783833#file783833line156
 
  Do we want to apply jmh to the 'api' project? I'd think we'd want to 
  add the root. That will give you the benefit of not having to disable this 
  task on the api project at all.

Gone.


 On Dec. 5, 2014, 7:42 p.m., Kevin Sweeney wrote:
  build.gradle, line 165
  https://reviews.apache.org/r/28710/diff/3/?file=783833#file783833line165
 
  should this be a test scope? also should this be in the root project 
  instead of the api project?

It's only needed to resolve /jmh deps. Test scope is irrelevant here.


 On Dec. 5, 2014, 7:42 p.m., Kevin Sweeney wrote:
  build.gradle, line 458
  https://reviews.apache.org/r/28710/diff/3/?file=783833#file783833line458
 
  Does
  
  ```groovy
  tasks('jmh')  {
//...
  }
  ```
  
  work?

It does not: ` Could not find method tasks() for arguments [jmh] on root 
project 'aurora'.`


- Maxim


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


On Dec. 5, 2014, 6:32 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28710/
 ---
 
 (Updated Dec. 5, 2014, 6:32 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Repository: aurora
 
 
 Description
 ---
 
 This RB is superseding the https://reviews.apache.org/r/28474/.
 
 
 I have spent some time researching the available microbenchmark frameworks 
 and JMH [1] came as a clear winner:
 - Active development trail [2]
 - Advanced featureset and built-in optimizations improving accuracy and 
 consistency [3]
 - Well documented set of examples [4]
 - Large community experience and collective wisdom. 
 
 
 This RB adds gradle support for running JMH benchmarks and is relying on JMH 
 gradle plugin [5]. The benchmarks are run via `./gradlew jmh` command.
 
 [1] - http://openjdk.java.net/projects/code-tools/jmh/
 [2] - http://hg.openjdk.java.net/code-tools/jmh/ 
 [3] - 
 https://groups.google.com/forum/#!msg/mechanical-sympathy/m4opvy4xq3U/7lY8x8SvHgwJ
 [4] - 
 http://hg.openjdk.java.net/code-tools/jmh/file/adb6047266d8/jmh-samples/src/main/java/org/openjdk/jmh/samples
 [5] - https://github.com/melix/jmh-gradle-plugin
 
 
 Diffs
 -
 
   build.gradle 152ba631e2dd07f0306e58e355274e10a4128140 
   config/findbugs/excludeFilter.xml d6c1b1681c2d8505a088f9fb082ce11ac400126f 
   src/jmh/java/org/apache/aurora/benchmark/SchedulerBenchmark.java 
 PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/28710/diff/
 
 
 Testing
 ---
 
 $ ./gradlew jmh
 
 Sample results generated:
 
 ```
 # VM invoker: 
 /Library/Java/JavaVirtualMachines/jdk1.7.0_25.jdk/Contents/Home/jre/bin/java
 # VM options: -Dfile.encoding=UTF-8 -Duser.country=US -Duser.language=en 
 -Duser.variant
 # Warmup: 1 iterations, 1 s each
 # Measurement: 3 iterations, 1 s each
 # Timeout: 10 min per iteration
 # Threads: 1 thread, will synchronize iterations
 # Benchmark mode: Throughput, ops/time
 # Benchmark: org.apache.aurora.benchmark.SchedulerBenchmark.example
 
 # Run progress: 0.00% complete, ETA 00:00:04
 # Fork: 1 of 1
 # Warmup Iteration   1: 3156839103.911 ops/s
 Iteration   1: 544897.411 ops/s
 Iteration   2: 3357230627.218 ops/s
 Iteration   3: 3461073727.560 ops/s
 
 
 Result: 3383949750.729 ±(99.9%) 1237528915.517 ops/s [Average]
   Statistics: (min, avg, max) = (544897.411, 3383949750.729, 
 3461073727.560), stdev = 67833135.714
   Confidence interval (99.9%): [2146420835.212, 4621478666.247]
 
 
 # Run complete. Total time: 00:00:05
 
 Benchmark  Mode  Samples   Score  
   Error  Units
 o.a.a.b.SchedulerBenchmark.examplethrpt3  3383949750.729 ± 
 1237528915.517  ops/s
 
 ```
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28660: Reduce minimum branch coverage requirement to avoid flakiness.


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

Ship it!


Ship It!

- Maxim Khutornenko


On Dec. 3, 2014, 7:02 p.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28660/
 ---
 
 (Updated Dec. 3, 2014, 7:02 p.m.)
 
 
 Review request for Aurora and Maxim Khutornenko.
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Reduce minimum branch coverage requirement to avoid flakiness.
 
 
 Diffs
 -
 
   build.gradle fb729c5096108c535229e266fa9649f997e6da37 
 
 Diff: https://reviews.apache.org/r/28660/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Bill Farner

Re: Review Request 28674: Remove Response.messageDEPRECATED field.


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



src/main/python/apache/aurora/client/base.py
https://reviews.apache.org/r/28674/#comment106024

This uses a different quoting style than the rest of the file.



src/main/python/apache/aurora/client/base.py
https://reviews.apache.org/r/28674/#comment106025

This will str() on a ResponseDetail object including the struct details 
that we don't need: 

```
$ ./pants py src/main/python/apache/aurora/client:base
 from gen.apache.aurora.api.ttypes import Response, ResponseDetail, 
ResponseCode
 resp = Response(responseCode=ResponseCode.OK, 
details=[ResponseDetail(message='Quota check successful.')])
 ', '.join(map(str, resp.details or []))
ResponseDetail(message='Quota check successful.')
```



src/test/python/apache/aurora/client/api/test_job_monitor.py
https://reviews.apache.org/r/28674/#comment106028

Use kvarg 'message=' for consistency?



src/test/python/apache/aurora/client/api/test_quota_check.py
https://reviews.apache.org/r/28674/#comment106029

same here



src/test/python/apache/aurora/client/api/test_task_util.py
https://reviews.apache.org/r/28674/#comment106032

same here


- Maxim Khutornenko


On Dec. 3, 2014, 8:34 p.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28674/
 ---
 
 (Updated Dec. 3, 2014, 8:34 p.m.)
 
 
 Review request for Aurora, Maxim Khutornenko and Zameer Manji.
 
 
 Bugs: AURORA-466
 https://issues.apache.org/jira/browse/AURORA-466
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Remove Response.message field.
 
 
 Diffs
 -
 
   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
 6b63f04a7113527e26d7f38e877b0ebd07822108 
   src/main/java/org/apache/aurora/scheduler/thrift/Util.java 
 d879db4157c7a2c782e3213974067d86b6184f04 
   src/main/python/apache/aurora/client/api/BUILD 
 8b0da6725362c6d9a3af6524a76a855a9bcbfd40 
   src/main/python/apache/aurora/client/api/__init__.py 
 d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59 
   src/main/python/apache/aurora/client/api/command_runner.py 
 14a316b6cda671764f2b2ac1ba5bbfef15eb1ab5 
   src/main/python/apache/aurora/client/api/quota_check.py 
 5877cba5dd06b2caa75ed0cab9786a80c2ae71b6 
   src/main/python/apache/aurora/client/api/restarter.py 
 43599e7ef7d17441f89f4a3a08b39b86d7d6fb5b 
   src/main/python/apache/aurora/client/api/updater.py 
 2092ff31141b6ccfedf0af673fe8dc2a74a7828e 
   src/main/python/apache/aurora/client/base.py 
 2c7d8160b23dbca0979cecf3bb44b904bf0d8de6 
   src/main/python/apache/aurora/client/cli/context.py 
 96c386e83db7b7c16419ca05b9155dd527bfb834 
   src/main/python/apache/aurora/client/cli/task.py 
 8a139db02ba6baf0dc558ccdba76d194fb0ebe88 
   src/main/python/apache/aurora/client/commands/admin.py 
 cb5ae88e3f39b7d7fbb80593be664809fbaa8958 
   src/main/python/apache/aurora/client/commands/core.py 
 ee227165d6f6b7c2a5c51d9e70b25b8cd0179381 
   src/main/python/apache/aurora/client/hooks/hooked_api.py 
 91efe5248144049d6a13b1ec81ffe08522df1ee9 
   
 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
  d687f572b467a76e79d55ea1d7eb0abf7ec61bbd 
   src/test/python/apache/aurora/client/api/test_api.py 
 1f4e9fe9111ac88726d7c45b699b3b91438448b6 
   src/test/python/apache/aurora/client/api/test_disambiguator.py 
 e9523ac67a67f83f55a7d79f38a5c13a9a90694c 
   src/test/python/apache/aurora/client/api/test_instance_watcher.py 
 abbbdbe953e3a81b64eb77ab096cef22c6ffc4c6 
   src/test/python/apache/aurora/client/api/test_job_monitor.py 
 27d8025bc80cff22c2f025302d1fe0519d8632e9 
   src/test/python/apache/aurora/client/api/test_quota_check.py 
 cb443c227589d69559c92444232eb6ba7d9259eb 
   src/test/python/apache/aurora/client/api/test_restarter.py 
 eb0af3bc588c088aa2aca8eb561cbd90d28209e1 
   src/test/python/apache/aurora/client/api/test_sla.py 
 50a6c47f00c77265328d6eacc835884e158b9e20 
   src/test/python/apache/aurora/client/api/test_task_util.py 
 3e772b949b0ec8b9cece62fc1ed46059a8310195 
   src/test/python/apache/aurora/client/api/test_updater.py 
 a32fc529cb1b23ab926a9180debb68bb826f66a8 
   src/test/python/apache/aurora/client/cli/util.py 
 0ec74e675aaabc7ac0cb28e02f5b8534570b7a49 
   src/test/python/apache/aurora/client/commands/test_admin.py 
 f9261affcc7d2f5391712fa0d0eb84e89a13bd70 
   src/test/python/apache/aurora/client/commands/test_kill.py 
 4ac742f4c7f3528cee0cdc25b9624ffde8384b11 
   src/test/python/apache/aurora/client/commands/util.py 
 c06de50e81be57cbf0480b1566f0efcec07f8a9d 
   src/test/python/apache/aurora/client/test_base.py 
 785784b3cb8e670111bb367363acc45772a8ea3e 
 
 Diff: https://reviews.apache.org/r

Re: Review Request 28693: Make abstract method annotations on ConfigurationPlugin effective.


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



src/main/python/apache/aurora/client/cli/__init__.py
https://reviews.apache.org/r/28693/#comment106171

Why not keeping @abstractmethod attributes and dropping the return 
statements instead? With your modification there is no need to keep this noop 
behavior as tests like EmptyPlugin below would not be possible anyway.


- Maxim Khutornenko


On Dec. 4, 2014, 6:36 a.m., Zameer Manji wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28693/
 ---
 
 (Updated Dec. 4, 2014, 6:36 a.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
 
 
 Repository: aurora
 
 
 Description
 ---
 
 This makes ConfigurationPlugin inherit from AbstractClass so the 
 @abstractmethod annotation is useful. This also removes the annotation for 
 the two methods with default values.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/__init__.py 
 6e553d8af459e575b2d62282a3bc0d1e266203d8 
   src/test/python/apache/aurora/client/cli/test_plugins.py 
 7a0a31818cbc57de952d7817f8e7c8fa1e84b25a 
 
 Diff: https://reviews.apache.org/r/28693/diff/
 
 
 Testing
 ---
 
 ./pants src/test/python/apache/aurora/client::
 
 
 Thanks,
 
 Zameer Manji

Re: Review Request 28693: Make abstract method annotations on ConfigurationPlugin effective.



 On Dec. 4, 2014, 5:21 p.m., Maxim Khutornenko wrote:
  src/main/python/apache/aurora/client/cli/__init__.py, lines 172-175
  https://reviews.apache.org/r/28693/diff/1/?file=782496#file782496line172
 
  Why not keeping @abstractmethod attributes and dropping the return 
  statements instead? With your modification there is no need to keep this 
  noop behavior as tests like EmptyPlugin below would not be possible 
  anyway.
 
 Zameer Manji wrote:
 The return statements provide default behaviour that plugins should have 
 to prevent AURORA-362. If get_options does not return an iterable the 
 argument handling code will crash and before_dispatch needs to return 
 raw_args to prevent the clobbering of user passed in commandline arguments.
 
 This reduces the work that plugins need to do if they are just interested 
 in implementing before_execution or after_execution.

I don't see how `before_dispatch` is semantically different from 
`before_execution` for the plugin implementor. The only difference is that one 
is expected to return `raw_args` and the other one is void. These noops provide 
no additional value besides questionable convenience on override. I would say 
drop the return statements, convert these methods back to abstract and properly 
document return type expectations. This way there is no second guessing what 
needs to be overriden and what isn't.


- Maxim


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


On Dec. 4, 2014, 6:36 a.m., Zameer Manji wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28693/
 ---
 
 (Updated Dec. 4, 2014, 6:36 a.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
 
 
 Repository: aurora
 
 
 Description
 ---
 
 This makes ConfigurationPlugin inherit from AbstractClass so the 
 @abstractmethod annotation is useful. This also removes the annotation for 
 the two methods with default values.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/__init__.py 
 6e553d8af459e575b2d62282a3bc0d1e266203d8 
   src/test/python/apache/aurora/client/cli/test_plugins.py 
 7a0a31818cbc57de952d7817f8e7c8fa1e84b25a 
 
 Diff: https://reviews.apache.org/r/28693/diff/
 
 
 Testing
 ---
 
 ./pants src/test/python/apache/aurora/client::
 
 
 Thanks,
 
 Zameer Manji

Re: Review Request 28692: Simplify logging in the Aurora client.


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


Can you paste a few examples from vagrant (preferrably job update) before and 
after your change? That would help reviewing.


src/main/python/apache/aurora/client/cli/context.py
https://reviews.apache.org/r/28692/#comment106184

Why is this jumping from TRANSCRIPT to INFO? Will every invocation now log 
the entire config?



src/main/python/apache/aurora/client/cli/context.py
https://reviews.apache.org/r/28692/#comment106185

same question here


- Maxim Khutornenko


On Dec. 4, 2014, 6:27 a.m., Zameer Manji wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28692/
 ---
 
 (Updated Dec. 4, 2014, 6:27 a.m.)
 
 
 Review request for Aurora, Maxim Khutornenko and Bill Farner.
 
 
 Bugs: AURORA-919
 https://issues.apache.org/jira/browse/AURORA-919
 
 
 Repository: aurora
 
 
 Description
 ---
 
 This patch removes a custom log level and adds a --verbose flag to the output.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/BUILD 
 ebe681a0d1735b7cc695dc3b7a14c4292d87ae32 
   src/main/python/apache/aurora/client/cli/__init__.py 
 6e553d8af459e575b2d62282a3bc0d1e266203d8 
   src/main/python/apache/aurora/client/cli/context.py 
 51c7d24dca664e476e62f1864d095416dfab70e4 
   src/main/python/apache/aurora/client/cli/logsetup.py 
 55d99c42f643910db0bf3c24022596383e160276 
   src/main/python/apache/aurora/client/cli/standalone_client.py 
 b7c8de66d6e4664b536911f826e36a984e8d0fef 
   src/test/python/apache/aurora/client/cli/test_plugins.py 
 7a0a31818cbc57de952d7817f8e7c8fa1e84b25a 
 
 Diff: https://reviews.apache.org/r/28692/diff/
 
 
 Testing
 ---
 
 ./pants src/test/python/apache/aurora/client::
 
 
 Thanks,
 
 Zameer Manji

Review Request 28710: Adding JMH framework support for scheduler performance analysis.


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

Review request for Aurora, Kevin Sweeney and Bill Farner.


Repository: aurora


Description
---

This RB is superseding the https://reviews.apache.org/r/28474/.


I have spent some time researching the available microbenchmark frameworks and 
JMH [1] came as a clear winner:
- Active development trail [2]
- Advanced featureset and built-in optimizations improving accuracy and 
consistency [3]
- Well documented set of examples [4]
- Large community experience and collective wisdom. 


This RB adds gradle support for running JMH benchmarks and is relying on JMH 
gradle plugin [5]. The benchmarks are run via `./gradlew jmh` command.

[1] - http://openjdk.java.net/projects/code-tools/jmh/
[2] - http://hg.openjdk.java.net/code-tools/jmh/ 
[3] - 
https://groups.google.com/forum/#!msg/mechanical-sympathy/m4opvy4xq3U/7lY8x8SvHgwJ
[4] - 
http://hg.openjdk.java.net/code-tools/jmh/file/adb6047266d8/jmh-samples/src/main/java/org/openjdk/jmh/samples
[5] - https://github.com/melix/jmh-gradle-plugin


Diffs
-

  build.gradle fb729c5096108c535229e266fa9649f997e6da37 
  config/findbugs/excludeFilter.xml d6c1b1681c2d8505a088f9fb082ce11ac400126f 
  src/jmh/java/org/apache/aurora/benchmark/SchedulerBenchmark.java PRE-CREATION 

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


Testing
---

$ ./gradlew jmh

Sample results generated:

```
# VM invoker: 
/Library/Java/JavaVirtualMachines/jdk1.7.0_25.jdk/Contents/Home/jre/bin/java
# VM options: -Dfile.encoding=UTF-8 -Duser.country=US -Duser.language=en 
-Duser.variant
# Warmup: 1 iterations, 1 s each
# Measurement: 3 iterations, 1 s each
# Timeout: 10 min per iteration
# Threads: 1 thread, will synchronize iterations
# Benchmark mode: Throughput, ops/time
# Benchmark: org.apache.aurora.benchmark.SchedulerBenchmark.example

# Run progress: 0.00% complete, ETA 00:00:04
# Fork: 1 of 1
# Warmup Iteration   1: 3156839103.911 ops/s
Iteration   1: 544897.411 ops/s
Iteration   2: 3357230627.218 ops/s
Iteration   3: 3461073727.560 ops/s


Result: 3383949750.729 ±(99.9%) 1237528915.517 ops/s [Average]
  Statistics: (min, avg, max) = (544897.411, 3383949750.729, 
3461073727.560), stdev = 67833135.714
  Confidence interval (99.9%): [2146420835.212, 4621478666.247]


# Run complete. Total time: 00:00:05

Benchmark  Mode  Samples   Score
Error  Units
o.a.a.b.SchedulerBenchmark.examplethrpt3  3383949750.729 ± 
1237528915.517  ops/s

```


Thanks,

Maxim Khutornenko

Re: Review Request 28474: Added manual perf tests for the scheduling pipeline.


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


Discarding this in favor of benchmark harness started in 
https://reviews.apache.org/r/28710/.

- Maxim Khutornenko


On Nov. 26, 2014, 6:15 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28474/
 ---
 
 (Updated Nov. 26, 2014, 6:15 p.m.)
 
 
 Review request for Aurora and Bill Farner.
 
 
 Bugs: AURORA-909
 https://issues.apache.org/jira/browse/AURORA-909
 
 
 Repository: aurora
 
 
 Description
 ---
 
 A manual testbed for the upcoming AURORA-909 work.
 
 
 Diffs
 -
 
   src/test/java/org/apache/aurora/scheduler/async/Offers.java 
 8293dd181b0d062e89776fdc1205c1c227d6bb6c 
   src/test/java/org/apache/aurora/scheduler/async/SchedulerPerfIT.java 
 PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/28474/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 IDE manual run:
 
 Nov 26, 2014 10:12:21 AM 
 org.apache.aurora.scheduler.async.preemptor.PreemptorModule configure
 INFO: Preemptor Enabled.
 Nov 26, 2014 10:12:21 AM com.twitter.common.util.BuildInfo fetchProperties
 INFO: Fetching build properties from build.properties
 Nov 26, 2014 10:12:21 AM com.twitter.common.util.BuildInfo fetchProperties
 WARNING: Failed to fetch build properties from build.properties
 Nov 26, 2014 10:12:21 AM 
 com.twitter.common.application.modules.StatsModule$StartStatPoller execute
 INFO: Build information: {}
 Nov 26, 2014 10:12:26 AM org.apache.aurora.scheduler.async.SchedulerPerfIT 
 runTest
 INFO: Results for: INSUFFICIENT_RESOURCES
 Nov 26, 2014 10:12:26 AM org.apache.aurora.scheduler.async.SchedulerPerfIT 
 logDuration
 INFO: Mean task_schedule_attempt duration: 737660.0796871068 ns
 Nov 26, 2014 10:12:26 AM org.apache.aurora.scheduler.async.SchedulerPerfIT 
 logDuration
 INFO: Mean offer_queue_launch_first duration: 332330.25185686833 ns
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28710: Adding JMH framework support for scheduler performance analysis.



 On Dec. 4, 2014, 8:12 p.m., Kevin Sweeney wrote:
  build.gradle, line 23
  https://reviews.apache.org/r/28710/diff/1/?file=782698#file782698line23
 
  Looks like you want the new-style block here instead:
  
  https://plugins.gradle.org/plugin/me.champeau.gradle.jmh

Great suggestion. Done.


- Maxim


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


On Dec. 4, 2014, 7:36 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28710/
 ---
 
 (Updated Dec. 4, 2014, 7:36 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Bill Farner.
 
 
 Repository: aurora
 
 
 Description
 ---
 
 This RB is superseding the https://reviews.apache.org/r/28474/.
 
 
 I have spent some time researching the available microbenchmark frameworks 
 and JMH [1] came as a clear winner:
 - Active development trail [2]
 - Advanced featureset and built-in optimizations improving accuracy and 
 consistency [3]
 - Well documented set of examples [4]
 - Large community experience and collective wisdom. 
 
 
 This RB adds gradle support for running JMH benchmarks and is relying on JMH 
 gradle plugin [5]. The benchmarks are run via `./gradlew jmh` command.
 
 [1] - http://openjdk.java.net/projects/code-tools/jmh/
 [2] - http://hg.openjdk.java.net/code-tools/jmh/ 
 [3] - 
 https://groups.google.com/forum/#!msg/mechanical-sympathy/m4opvy4xq3U/7lY8x8SvHgwJ
 [4] - 
 http://hg.openjdk.java.net/code-tools/jmh/file/adb6047266d8/jmh-samples/src/main/java/org/openjdk/jmh/samples
 [5] - https://github.com/melix/jmh-gradle-plugin
 
 
 Diffs
 -
 
   build.gradle fb729c5096108c535229e266fa9649f997e6da37 
   config/findbugs/excludeFilter.xml d6c1b1681c2d8505a088f9fb082ce11ac400126f 
   src/jmh/java/org/apache/aurora/benchmark/SchedulerBenchmark.java 
 PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/28710/diff/
 
 
 Testing
 ---
 
 $ ./gradlew jmh
 
 Sample results generated:
 
 ```
 # VM invoker: 
 /Library/Java/JavaVirtualMachines/jdk1.7.0_25.jdk/Contents/Home/jre/bin/java
 # VM options: -Dfile.encoding=UTF-8 -Duser.country=US -Duser.language=en 
 -Duser.variant
 # Warmup: 1 iterations, 1 s each
 # Measurement: 3 iterations, 1 s each
 # Timeout: 10 min per iteration
 # Threads: 1 thread, will synchronize iterations
 # Benchmark mode: Throughput, ops/time
 # Benchmark: org.apache.aurora.benchmark.SchedulerBenchmark.example
 
 # Run progress: 0.00% complete, ETA 00:00:04
 # Fork: 1 of 1
 # Warmup Iteration   1: 3156839103.911 ops/s
 Iteration   1: 544897.411 ops/s
 Iteration   2: 3357230627.218 ops/s
 Iteration   3: 3461073727.560 ops/s
 
 
 Result: 3383949750.729 ±(99.9%) 1237528915.517 ops/s [Average]
   Statistics: (min, avg, max) = (544897.411, 3383949750.729, 
 3461073727.560), stdev = 67833135.714
   Confidence interval (99.9%): [2146420835.212, 4621478666.247]
 
 
 # Run complete. Total time: 00:00:05
 
 Benchmark  Mode  Samples   Score  
   Error  Units
 o.a.a.b.SchedulerBenchmark.examplethrpt3  3383949750.729 ± 
 1237528915.517  ops/s
 
 ```
 
 
 Thanks,
 
 Maxim Khutornenko

Review Request 28731: Implemented TaskScheduler benchmarks.

  97: 4684194.915 ns/op
Iteration  98: 4649130.252 ns/op
Iteration  99: 4619602.510 ns/op
Iteration 100: 4593595.833 ns/op


Result: 5011738.309 Â±(99.9%) 160249.620 ns/op [Average]
  Statistics: (min, avg, max) = (4527586.066, 5011738.309, 7052592.357), stdev 
= 472499.654
  Confidence interval (99.9%): [4851488.689, 5171987.930]


# Run complete. Total time: 00:04:29

Benchmark   
 Mode  SamplesScoreError  Units
o.a.a.b.SchedulingBenchmarks.ConstraintMismatchsSchedulingBenchmark.example 
 avgt  100  5165386.898 Â± 344576.928  ns/op
o.a.a.b.SchedulingBenchmarks.InsufficientResourcesSchedulingBenchmark.example   
 avgt  100  5011738.309 Â± 160249.620  ns/op
```


Thanks,

Maxim Khutornenko

Re: Review Request 28674: Remove Response.messageDEPRECATED field.


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

Ship it!


Ship It!

- Maxim Khutornenko


On Dec. 5, 2014, 1:09 a.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28674/
 ---
 
 (Updated Dec. 5, 2014, 1:09 a.m.)
 
 
 Review request for Aurora, Maxim Khutornenko and Zameer Manji.
 
 
 Bugs: AURORA-466
 https://issues.apache.org/jira/browse/AURORA-466
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Remove Response.message field.
 
 
 Diffs
 -
 
   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
 7d55dce06c77b17b2f895834e88e5c8543462b31 
   src/main/java/org/apache/aurora/scheduler/thrift/Util.java 
 d879db4157c7a2c782e3213974067d86b6184f04 
   src/main/python/apache/aurora/client/api/BUILD 
 8b0da6725362c6d9a3af6524a76a855a9bcbfd40 
   src/main/python/apache/aurora/client/api/__init__.py 
 d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59 
   src/main/python/apache/aurora/client/api/command_runner.py 
 14a316b6cda671764f2b2ac1ba5bbfef15eb1ab5 
   src/main/python/apache/aurora/client/api/quota_check.py 
 5877cba5dd06b2caa75ed0cab9786a80c2ae71b6 
   src/main/python/apache/aurora/client/api/restarter.py 
 43599e7ef7d17441f89f4a3a08b39b86d7d6fb5b 
   src/main/python/apache/aurora/client/api/updater.py 
 2092ff31141b6ccfedf0af673fe8dc2a74a7828e 
   src/main/python/apache/aurora/client/base.py 
 2c7d8160b23dbca0979cecf3bb44b904bf0d8de6 
   src/main/python/apache/aurora/client/cli/context.py 
 96c386e83db7b7c16419ca05b9155dd527bfb834 
   src/main/python/apache/aurora/client/cli/task.py 
 8a139db02ba6baf0dc558ccdba76d194fb0ebe88 
   src/main/python/apache/aurora/client/commands/admin.py 
 cb5ae88e3f39b7d7fbb80593be664809fbaa8958 
   src/main/python/apache/aurora/client/commands/core.py 
 ee227165d6f6b7c2a5c51d9e70b25b8cd0179381 
   src/main/python/apache/aurora/client/hooks/hooked_api.py 
 91efe5248144049d6a13b1ec81ffe08522df1ee9 
   
 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
  072ea2b916d9d7d01cd7ba75c79b96896dccca7f 
   src/test/python/apache/aurora/client/api/test_api.py 
 1f4e9fe9111ac88726d7c45b699b3b91438448b6 
   src/test/python/apache/aurora/client/api/test_disambiguator.py 
 e9523ac67a67f83f55a7d79f38a5c13a9a90694c 
   src/test/python/apache/aurora/client/api/test_instance_watcher.py 
 abbbdbe953e3a81b64eb77ab096cef22c6ffc4c6 
   src/test/python/apache/aurora/client/api/test_job_monitor.py 
 27d8025bc80cff22c2f025302d1fe0519d8632e9 
   src/test/python/apache/aurora/client/api/test_quota_check.py 
 cb443c227589d69559c92444232eb6ba7d9259eb 
   src/test/python/apache/aurora/client/api/test_restarter.py 
 eb0af3bc588c088aa2aca8eb561cbd90d28209e1 
   src/test/python/apache/aurora/client/api/test_sla.py 
 50a6c47f00c77265328d6eacc835884e158b9e20 
   src/test/python/apache/aurora/client/api/test_task_util.py 
 3e772b949b0ec8b9cece62fc1ed46059a8310195 
   src/test/python/apache/aurora/client/api/test_updater.py 
 a32fc529cb1b23ab926a9180debb68bb826f66a8 
   src/test/python/apache/aurora/client/cli/util.py 
 0ec74e675aaabc7ac0cb28e02f5b8534570b7a49 
   src/test/python/apache/aurora/client/commands/test_admin.py 
 f9261affcc7d2f5391712fa0d0eb84e89a13bd70 
   src/test/python/apache/aurora/client/commands/test_kill.py 
 4ac742f4c7f3528cee0cdc25b9624ffde8384b11 
   src/test/python/apache/aurora/client/commands/util.py 
 c06de50e81be57cbf0480b1566f0efcec07f8a9d 
   src/test/python/apache/aurora/client/test_base.py 
 785784b3cb8e670111bb367363acc45772a8ea3e 
 
 Diff: https://reviews.apache.org/r/28674/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Bill Farner

Re: Review Request 28607: Add a caching ClusterState implementation.


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



src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java
https://reviews.apache.org/r/28607/#comment105873

minor nit: you might want to have it outside the synchronized block to 
further reduce lock scope.



src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java
https://reviews.apache.org/r/28607/#comment105877

Would it make sense to do it conditionally, i.e.:
```java
if (!Tasks.SLAVE_ASSIGNED_STATES.contains(stateChange.getNewState()) {
  Iterables.remove...
} else {
  victims.put...
}
```


- Maxim Khutornenko


On Dec. 2, 2014, 10:03 p.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28607/
 ---
 
 (Updated Dec. 2, 2014, 10:03 p.m.)
 
 
 Review request for Aurora, Maxim Khutornenko and Zameer Manji.
 
 
 Bugs: AURORA-121
 https://issues.apache.org/jira/browse/AURORA-121
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Add a caching ClusterState implementation.
 
 
 Diffs
 -
 
   
 src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java
  PRE-CREATION 
   src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java 
 3524dc595e7b61a531912843f90b01a87bc57cc4 
   
 src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
  489c0bfa22aae14fbd3fd2ee9e5bd32b7c3aafa5 
   
 src/test/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterStateTest.java
  PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/28607/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Bill Farner

Review Request 28617: Implemented offer filtering for tasks with static vetoes.


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

Review request for Aurora, Kevin Sweeney and Bill Farner.


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


Repository: aurora


Description
---

Modified the task offer/task matching logic to skip offer matching for tasks 
previously vetoed statically.

I will follow up with more accurate measurements 
(https://reviews.apache.org/r/28474/) but preliminary testing in vagrant (see 
pictures below) shows close to 50% improvement in task scheduling performance.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/TaskVars.java 
f017cdd26ca40138a7e141f21613ed567314c399 
  src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 
f66383830140e5eaba436f35ebb5192eee65947a 
  src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
ead9d28100673440168a32d114ecaa15874978a6 
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
c2a342ce07bfb223193886038761f0da5230135d 
  src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
1cb56f19c331508a1585077e9c4a98f52aac343b 
  src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
e1c29747c9854cf75bf63f6f085cf40ca68989af 
  src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
4e7efb3c1214c3d193afd61f162713490eb8effb 
  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/filter/SchedulingFilterImplTest.java 
265c38d20136210e7639ac8ea915d307a4b72949 
  src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
411a55a8d85f60bb2703468f2d69b64b2736eee4 

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


Testing
---

./gradlew -Pq build

Tested in vagrant


File Attachments


NoStaticVetoFiltering.png
  
https://reviews.apache.org/media/uploaded/files/2014/12/03/7945c60b-4135-4016-a9bf-8d4815a4a573__NoStaticVetoFiltering.png
StaticVetoFiltering.png
  
https://reviews.apache.org/media/uploaded/files/2014/12/03/2f73b94a-5ba9-43b6-922e-e9e4ec18d0bb__StaticVetoFiltering.png


Thanks,

Maxim Khutornenko

Re: Review Request 28607: Add a caching ClusterState implementation.



 On Dec. 2, 2014, 11:09 p.m., Maxim Khutornenko wrote:
  src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java,
   line 58
  https://reviews.apache.org/r/28607/diff/1/?file=780416#file780416line58
 
  Would it make sense to do it conditionally, i.e.:
  ```java
  if (!Tasks.SLAVE_ASSIGNED_STATES.contains(stateChange.getNewState()) {
Iterables.remove...
  } else {
victims.put...
  }
  ```
 
 Bill Farner wrote:
 That would introduce duplicates, e.g. PENDING - ASSIGNED - RUNNING 
 would result in 2 entries for the task.

Why not use a HashMultimap to store victims then? I don't see anything in the 
PreemptionVictim that would be different between task state transitions.


- Maxim


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


On Dec. 2, 2014, 10:03 p.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28607/
 ---
 
 (Updated Dec. 2, 2014, 10:03 p.m.)
 
 
 Review request for Aurora, Maxim Khutornenko and Zameer Manji.
 
 
 Bugs: AURORA-121
 https://issues.apache.org/jira/browse/AURORA-121
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Add a caching ClusterState implementation.
 
 
 Diffs
 -
 
   
 src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java
  PRE-CREATION 
   src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java 
 3524dc595e7b61a531912843f90b01a87bc57cc4 
   
 src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
  489c0bfa22aae14fbd3fd2ee9e5bd32b7c3aafa5 
   
 src/test/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterStateTest.java
  PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/28607/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Bill Farner

Re: Review Request 28607: Add a caching ClusterState implementation.


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

Ship it!



src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java
https://reviews.apache.org/r/28607/#comment105910

This can be further simplified (unless you are concerned about the heap 
churn):
```java
victims.remove(slaveId, 
PreemptionVictim.fromTask(stateChange.getTask().getAssignedTask());
```


- Maxim Khutornenko


On Dec. 3, 2014, 1:39 a.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28607/
 ---
 
 (Updated Dec. 3, 2014, 1:39 a.m.)
 
 
 Review request for Aurora, Maxim Khutornenko and Zameer Manji.
 
 
 Bugs: AURORA-121
 https://issues.apache.org/jira/browse/AURORA-121
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Add a caching ClusterState implementation.
 
 
 Diffs
 -
 
   
 src/main/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterState.java
  PRE-CREATION 
   src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java 
 3524dc595e7b61a531912843f90b01a87bc57cc4 
   
 src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
  489c0bfa22aae14fbd3fd2ee9e5bd32b7c3aafa5 
   
 src/test/java/org/apache/aurora/scheduler/async/preemptor/CachedClusterStateTest.java
  PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/28607/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Bill Farner

Re: Review Request 28623: Remove getVersion RPC and DEPRECATEDversion Response field.


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

Ship it!


Ship It!

- Maxim Khutornenko


On Dec. 3, 2014, 2:13 a.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28623/
 ---
 
 (Updated Dec. 3, 2014, 2:13 a.m.)
 
 
 Review request for Aurora, David McLaughlin and Kevin Sweeney.
 
 
 Bugs: AURORA-143 and AURORA-467
 https://issues.apache.org/jira/browse/AURORA-143
 https://issues.apache.org/jira/browse/AURORA-467
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Remove getVersion RPC and DEPRECATEDversion Response field.
 
 
 Diffs
 -
 
   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
 6b63f04a7113527e26d7f38e877b0ebd07822108 
   
 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
  0898c62315c5a47628ad629182c3177c86a00bce 
   
 src/main/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptor.java
  3406722067af40a91fe39340d94ee03d20d7ddbd 
   
 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
  d687f572b467a76e79d55ea1d7eb0abf7ec61bbd 
   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
 04979084b5352d3044bd3c2ba7071e10d9992765 
   
 src/test/java/org/apache/aurora/scheduler/thrift/aop/ServerInfoInterceptorTest.java
  840b3f88e7de306fa0af73593b5bac6cc00528da 
 
 Diff: https://reviews.apache.org/r/28623/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Bill Farner

Re: Review Request 28571: Reject jobs containing an empty cron schedule.

2014-12-01 Thread Maxim Khutornenko


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

Ship it!


Ship It!

- Maxim Khutornenko


On Dec. 1, 2014, 8:07 p.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28571/
 ---
 
 (Updated Dec. 1, 2014, 8:07 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
 
 
 Bugs: AURORA-424
 https://issues.apache.org/jira/browse/AURORA-424
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Reject jobs containing an empty cron schedule.
 
 
 Diffs
 -
 
   
 src/main/java/org/apache/aurora/scheduler/configuration/SanitizedConfiguration.java
  fe3ad4ca973f5ae494250ffe50d419032be0d984 
   src/main/java/org/apache/aurora/scheduler/cron/SanitizedCronJob.java 
 0e35cfbe6519050d86c154cc4ee545400757503e 
   
 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
  b28b3ae9010bd4bae652bbf1b2ceb45e01e23784 
 
 Diff: https://reviews.apache.org/r/28571/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Bill Farner

Re: Review Request 28572: Minimize the state consumed when collecting preemption victims.

2014-12-01 Thread Maxim Khutornenko


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

Ship it!



src/main/java/org/apache/aurora/scheduler/async/preemptor/LiveClusterState.java
https://reviews.apache.org/r/28572/#comment105758

Please, drop the now unused TASK_TO_SLAVE_ID.


- Maxim Khutornenko


On Dec. 1, 2014, 8:32 p.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28572/
 ---
 
 (Updated Dec. 1, 2014, 8:32 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Zameer Manji.
 
 
 Bugs: AURORA-121
 https://issues.apache.org/jira/browse/AURORA-121
 
 
 Repository: aurora
 
 
 Description
 ---
 
 This should be the last step before creating a caching `ClusterState` 
 implementation.
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java 
 4f0019a1a1272e27e600c47641a05d320035 
   
 src/main/java/org/apache/aurora/scheduler/async/preemptor/LiveClusterState.java
  9d83acc4177076411c5ce6c8304fcf75cb029597 
   
 src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptionVictim.java
  PRE-CREATION 
   
 src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImpl.java 
 f013383b095968b0de6e917d2311c85d6afe53f7 
   
 src/test/java/org/apache/aurora/scheduler/async/preemptor/LiveClusterStateTest.java
  8f91ff684dddc0591e71503afc0f999e508b7dbe 
 
 Diff: https://reviews.apache.org/r/28572/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Bill Farner

Re: Review Request 28411: Adding quota check into replaceCronTemplate rpc.

2014-11-26 Thread Maxim Khutornenko


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


@ReviewBot retry

- Maxim Khutornenko


On Nov. 24, 2014, 9:27 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28411/
 ---
 
 (Updated Nov. 24, 2014, 9:27 p.m.)
 
 
 Review request for Aurora and Bill Farner.
 
 
 Bugs: AURORA-946
 https://issues.apache.org/jira/browse/AURORA-946
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Merged scheduleCronJob and replaceCronTemplate implementations. 
 
 
 Diffs
 -
 
   
 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
  a5e869fc52ab6c4c28e6965585b015440f448f59 
   src/main/thrift/org/apache/aurora/gen/api.thrift 
 b91fca9383891af15477a6f6ef7c407bfa125303 
   
 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
  de5f21a084109e5f31a7e5fca1b8ee265e30b893 
 
 Diff: https://reviews.apache.org/r/28411/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 vagrant@vagrant-ubuntu-trusty-64:~$ aurora update  
 devcluster/vagrant/test/cron_hello_world2 
 aurora/examples/jobs/cron_hello_world.aurora 
 WARNING: update is an aurora clientv1 command which will be deprecated soon
 To run this command using clientv2, use 'aurora job update 
 devcluster/vagrant/test/cron_hello_world2 
 aurora/examples/jobs/cron_hello_world.aurora 
 --health-check-interval-seconds=3'
  INFO] Updating job: cron_hello_world2
  INFO] Response from scheduler: ERROR (message: Aborting update without 
 rollback! Fatal error: Response from scheduler: INVALID_REQUEST (message: 
 Insufficient resource quota: CPU quota exceeded by 1.00 core(s); RAM quota 
 exceeded by 1024.00 MB; DISK quota exceeded by 1024.00 MB))
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28411: Adding quota check into replaceCronTemplate rpc.

2014-11-26 Thread Maxim Khutornenko


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

(Updated Nov. 27, 2014, 12:36 a.m.)


Review request for Aurora and Bill Farner.


Changes
---

Rebased.


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


Repository: aurora


Description
---

Merged scheduleCronJob and replaceCronTemplate implementations. 


Diffs (updated)
-

  api/src/main/thrift/org/apache/aurora/gen/api.thrift 
b91fca9383891af15477a6f6ef7c407bfa125303 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
9087eb2a5b305ceb1c5d50c75b9ca2ea2143a10f 
  
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 c602b304fc7d8abbcf397f30dd364d6aa1b4de9b 

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


Testing
---

./gradlew -Pq build

vagrant@vagrant-ubuntu-trusty-64:~$ aurora update  
devcluster/vagrant/test/cron_hello_world2 
aurora/examples/jobs/cron_hello_world.aurora 
WARNING: update is an aurora clientv1 command which will be deprecated soon
To run this command using clientv2, use 'aurora job update 
devcluster/vagrant/test/cron_hello_world2 
aurora/examples/jobs/cron_hello_world.aurora --health-check-interval-seconds=3'
 INFO] Updating job: cron_hello_world2
 INFO] Response from scheduler: ERROR (message: Aborting update without 
rollback! Fatal error: Response from scheduler: INVALID_REQUEST (message: 
Insufficient resource quota: CPU quota exceeded by 1.00 core(s); RAM quota 
exceeded by 1024.00 MB; DISK quota exceeded by 1024.00 MB))


Thanks,

Maxim Khutornenko

Re: Review Request 28398: Adding cron check into aurora beta-update start.


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

(Updated Nov. 25, 2014, 6 p.m.)


Review request for Aurora, David McLaughlin, Kevin Sweeney, and Bill Farner.


Changes
---

CR comments.


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


Repository: aurora


Description
---

Failing fast aurora beta-update start when a cron schedule is specified.

Also, updated cron command help messages and docs.


Diffs (updated)
-

  docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7 
  docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
a5e869fc52ab6c4c28e6965585b015440f448f59 
  src/main/python/apache/aurora/client/cli/cron.py 
cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2 
  src/main/python/apache/aurora/client/cli/update.py 
12774af8bcd1c953fdbc799b0a142c27407d69f5 
  
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 de5f21a084109e5f31a7e5fca1b8ee265e30b893 
  src/test/python/apache/aurora/client/cli/test_supdate.py 
7637352feea6b07408256158814c05bc17ec14f3 

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


Testing
---

./pants src/test/python:all

Tested in vagrant.


Thanks,

Maxim Khutornenko

Re: Review Request 28398: Adding cron check into aurora beta-update start.



 On Nov. 24, 2014, 7:48 p.m., David McLaughlin wrote:
  src/main/python/apache/aurora/client/cli/cron.py, line 79
  https://reviews.apache.org/r/28398/diff/1/?file=774538#file774538line79
 
  s/existing //

Done.


- Maxim


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


On Nov. 24, 2014, 7:19 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28398/
 ---
 
 (Updated Nov. 24, 2014, 7:19 p.m.)
 
 
 Review request for Aurora, David McLaughlin, Kevin Sweeney, and Bill Farner.
 
 
 Bugs: AURORA-921
 https://issues.apache.org/jira/browse/AURORA-921
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Failing fast aurora beta-update start when a cron schedule is specified.
 
 Also, updated cron command help messages and docs.
 
 
 Diffs
 -
 
   docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7 
   docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72 
   
 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
  a5e869fc52ab6c4c28e6965585b015440f448f59 
   src/main/python/apache/aurora/client/cli/cron.py 
 cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2 
   src/main/python/apache/aurora/client/cli/update.py 
 12774af8bcd1c953fdbc799b0a142c27407d69f5 
   
 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
  de5f21a084109e5f31a7e5fca1b8ee265e30b893 
   src/test/python/apache/aurora/client/cli/test_supdate.py 
 7637352feea6b07408256158814c05bc17ec14f3 
 
 Diff: https://reviews.apache.org/r/28398/diff/
 
 
 Testing
 ---
 
 ./pants src/test/python:all
 
 Tested in vagrant.
 
 
 Thanks,
 
 Maxim Khutornenko

Review Request 28445: Improving messages in CronJobManager.


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

Review request for Aurora and Kevin Sweeney.


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


Repository: aurora


Description
---

Fixed checkCronExists() error message.

Also, refactored message handling and added tests to bring the branch coverage 
to 100%.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java 
28f1ae72aec392d6b2666e8993920106b8e3138f 
  
src/test/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImplTest.java
 934e9bb669e6647dfbc2b43f00d036bad19932e5 

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


Testing
---

./graldew -Pq build


Thanks,

Maxim Khutornenko

Re: Review Request 28361: Extract thrift into an API subproject.


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

Ship it!


Ship It!

- Maxim Khutornenko


On Nov. 25, 2014, 7:39 p.m., Kevin Sweeney wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28361/
 ---
 
 (Updated Nov. 25, 2014, 7:39 p.m.)
 
 
 Review request for Aurora, Joshua Cohen, Bill Farner, and Zameer Manji.
 
 
 Bugs: AURORA-925
 https://issues.apache.org/jira/browse/AURORA-925
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Extract thrift generation into an api subproject.
 
 
 Diffs
 -
 
   .gitignore ec072d2cf7d9678c59d2f67ec3ab1fda3efd88b7 
   BUILD b6d7ce983dfbb99dd7e26e547a4992a27a48ccdf 
   build-support/python/make-pycharm-virtualenv 
 8f58d4df650892aff987ccfe47a9580023b8cf63 
   build-support/release/make-python-sdists 
 e0d20a19ef1fbc129a882b6368515a3ea41bab3f 
   build-support/thrift/thriftw PRE-CREATION 
   build.gradle 7b35dedb19143b27955c12c4df94ba98ff5f6608 
   buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy 
 f233153fd093cae255c1cb6807cfec6590ba36f9 
   
 buildSrc/src/main/groovy/org/apache/aurora/build/ThriftEntitiesPlugin.groovy 
 PRE-CREATION 
   buildSrc/src/main/groovy/org/apache/aurora/build/ThriftPlugin.groovy 
 PRE-CREATION 
   gradle/wrapper/gradle-wrapper.properties 
 b04300260fd3975ec98a1a1d87b57025e7904c2f 
   settings.gradle 3e9cbfe6e6836f70fb9164f80973f3ebb813ef98 
   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
 4b9281590a9eeb8a8b571b909fd507259abfac44 
   src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
 c072010ed5a1856a05fa7e6c87b25c073059 
   src/main/python/apache/aurora/admin/BUILD 
 9441e121bf4ea88a7ae286f8cd1cbd0c0ac89e06 
   src/main/python/apache/aurora/client/BUILD 
 48566d99f432e6eb94d1bfdb18277205fd7bb9d8 
   src/main/python/apache/aurora/client/api/BUILD 
 6d2a1bfe8531d800be651ec00518fc5b07a53474 
   src/main/python/apache/aurora/client/cli/BUILD 
 e6627a8a3c501292fdd31ec384320870db702bc2 
   src/main/python/apache/aurora/client/commands/BUILD 
 d146015d70715142618f2653538aca6beb83c1fc 
   src/main/python/apache/aurora/client/hooks/BUILD 
 f46cf650d9b471d08ed2b76652bca5d429f955ee 
   src/main/python/apache/aurora/common/BUILD 
 02ec17ce35c6632df204ea4d1d12e61daf30dd1f 
   src/main/python/apache/aurora/common/auth/BUILD 
 c26d117122d17b2228b637441ce0aa564f5a8a3e 
   src/main/python/apache/aurora/config/BUILD 
 fa40ebdfdecae3eb13878d6f172a591c16507530 
   src/main/python/apache/aurora/config/schema/BUILD 
 157c141bdf968666f31452784967de2a640d1815 
   src/main/python/apache/aurora/executor/BUILD 
 ca4193d31e3e3a71fa45f918058fba40fc911487 
   src/main/python/apache/aurora/executor/common/BUILD 
 d33e14b1cf7be7d115c7fe7741d52896f02f3aed 
   src/main/python/apache/aurora/tools/java/thrift_wrapper_codegen.py 
 d6bb5a120bb6368305ab52d253d25ad2bcfb3e8b 
   src/main/python/apache/thermos/bin/BUILD 
 34e2b3fe1d1a49cc51db8f61339a21ff8e03e0f4 
   src/main/python/apache/thermos/common/BUILD 
 918800b4ccb7bef61249d488ca8a4255168311bb 
   src/main/python/apache/thermos/core/BUILD 
 f362acdc395f1e479bcaf4bc063456abb1fdb0e2 
   src/main/python/apache/thermos/monitoring/BUILD 
 0dad47e6337b7db33dca793f0324892d61e3433a 
   src/main/python/apache/thermos/observer/BUILD 
 b07db90906a06779f5369158469651425fefa1a3 
   src/main/python/apache/thermos/testing/BUILD 
 b96c166e9bc529b783071b5d37b78779a36d06c3 
   src/main/resources/scheduler/assets/scheduler/index.html 
 f4ca07166aee21b8e6b7d3da82454e1b9b132f59 
   src/main/thrift/org/apache/aurora/gen/BUILD  
   src/main/thrift/org/apache/aurora/gen/api.thrift  
   src/main/thrift/org/apache/aurora/gen/internal_rpc.thrift  
   src/main/thrift/org/apache/aurora/gen/storage.thrift  
   src/main/thrift/org/apache/aurora/gen/test.thrift  
   src/main/thrift/org/apache/thermos/BUILD  
   src/main/thrift/org/apache/thermos/thermos_internal.thrift  
   src/test/python/apache/aurora/admin/BUILD 
 101aaa068f2a134f0e846359dc11c7a0ccc28dbb 
   src/test/python/apache/aurora/client/api/BUILD 
 f46ef695decd9b91112b9c933f6220efaa5a0bd3 
   src/test/python/apache/aurora/client/commands/BUILD 
 000eafa95802d6cc835cae7c6f68016645dfe0f1 
   src/test/python/apache/aurora/common/BUILD 
 3933c4616e1df667e7b2ce79e1a6df4c62ca73cf 
   src/test/python/apache/aurora/config/BUILD 
 551595ec6268046817cb1f183d8b0c0af03f5cba 
   src/test/python/apache/aurora/executor/BUILD 
 23a93fdb50c2492d4d8f4613796ff1723e3659fa 
   src/test/python/apache/aurora/executor/common/BUILD 
 318e66d477bbf75d5e36ffe4bc70294da34b4965 
 
 Diff: https://reviews.apache.org/r/28361/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 ./pants src/test/python:all
 
 
 Thanks

Review Request 28450: Fixing admin query command


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

Review request for Aurora and Bill Farner.


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


Repository: aurora


Description
---

Changing the way query is constructed to avoid Invalid JobKey error.


Diffs
-

  src/main/python/apache/aurora/client/commands/admin.py 
9719a581d69e75dc72e8f56ff12758cf9702271b 
  src/test/python/apache/aurora/client/commands/test_admin.py 
7dd61cdd6735bd0f274722249276d60084b4dd93 

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


Testing
---

./pants src/test/python:all


Thanks,

Maxim Khutornenko

Re: Review Request 28350: Add cron replace command.

2014-11-24 Thread Maxim Khutornenko


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


Discarding this in favor of an upserting cron schedule command. Will send a 
new diff to fix help/messaging.

- Maxim Khutornenko


On Nov. 22, 2014, 12:54 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28350/
 ---
 
 (Updated Nov. 22, 2014, 12:54 a.m.)
 
 
 Review request for Aurora, David McLaughlin and Bill Farner.
 
 
 Bugs: AURORA-921
 https://issues.apache.org/jira/browse/AURORA-921
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Implementing aurora cron replace command to close the functionality gap 
 that will be created with client updater removal.
 
 
 Diffs
 -
 
   docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7 
   docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72 
   src/main/python/apache/aurora/client/api/__init__.py 
 d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59 
   src/main/python/apache/aurora/client/cli/cron.py 
 cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2 
   src/main/python/apache/aurora/client/cli/update.py 
 12774af8bcd1c953fdbc799b0a142c27407d69f5 
   src/test/python/apache/aurora/client/api/test_api.py 
 1f4e9fe9111ac88726d7c45b699b3b91438448b6 
   src/test/python/apache/aurora/client/cli/test_cron.py 
 c748212febf5867f5f7cc54e34bf91a8890d 
   src/test/python/apache/aurora/client/cli/test_supdate.py 
 7637352feea6b07408256158814c05bc17ec14f3 
   src/test/python/apache/aurora/client/cli/util.py 
 0ec74e675aaabc7ac0cb28e02f5b8534570b7a49 
 
 Diff: https://reviews.apache.org/r/28350/diff/
 
 
 Testing
 ---
 
 ./pants src/test/python:all
 
 tested in vagrant as well
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28193: Prevent Aurora from creating zero sized Executor tasks.


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



src/main/java/org/apache/aurora/scheduler/ResourceSlot.java
https://reviews.apache.org/r/28193/#comment104640

tabbing is off



src/main/java/org/apache/aurora/scheduler/configuration/Resources.java
https://reviews.apache.org/r/28193/#comment104644

This is only used in tests outside of this class. Consider reverting to 
private.



src/main/java/org/apache/aurora/scheduler/configuration/Resources.java
https://reviews.apache.org/r/28193/#comment104647

+1 on moving it closer to its only consumer. That's a general guideline we 
follow everywhere.



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
https://reviews.apache.org/r/28193/#comment104649

Not related to your change but consider renaming it to something different 
(e.g. ExecutorSettings) to avoid naming collision with the thrift object.



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
https://reviews.apache.org/r/28193/#comment104651

-100MB looks like negative resource and is confusing.



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
https://reviews.apache.org/r/28193/#comment104660

Why not MIN_EXECUTOR_RESOURCES? We normally abstract out from the process 
framework concept in the scheduler code.



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
https://reviews.apache.org/r/28193/#comment104654

Any justification for the min resources chosen similar to the above?



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
https://reviews.apache.org/r/28193/#comment104655

Convert to // for inline comments.



src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java
https://reviews.apache.org/r/28193/#comment104665

Unless I am missing something, all you need to do here is to make sure 
neither containerResources nor executorResources is less than min. Can you do 
something like:
finalTaskResources = Resources.maxElements(containerResources, 
MIN_TASK_RESOURCES);

and replace .addAllResources(MIN_THERMOS_RESOURCES.toResourceList()) with
.addAllResources(Resources.maxElements(executorOverhead, 
MIN_THERMOS_RESOURCES))?


- Maxim Khutornenko


On Nov. 21, 2014, 5:01 a.m., Zameer Manji wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28193/
 ---
 
 (Updated Nov. 21, 2014, 5:01 a.m.)
 
 
 Review request for Aurora, Maxim Khutornenko and Bill Farner.
 
 
 Bugs: AURORA-928
 https://issues.apache.org/jira/browse/AURORA-928
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Mesos rejects tasks and executors that are zero sized. This patch 
 reconfigures Aurora to ensure no zero sized tasks and executors are created.
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
 ed60447c798a97daceda4a3bba6ee9bcdcaedd0f 
   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
 40b652c679d8e340f585e28cbed066335d9d760d 
   src/main/java/org/apache/aurora/scheduler/configuration/Resources.java 
 65c4b526c89a4d5607af4424ebe49bb48e296ae9 
   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
 bb227fd86f7c4c692f6ae2aef1c15a94913354b7 
   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
 899416fceae498353880012b8a93491cff461064 
   src/test/java/org/apache/aurora/scheduler/configuration/ResourcesTest.java 
 d6febb8998e05257cabe8d193cefa0b6c79f197e 
   
 src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
 953c1edb6802d8983ab324aa56361e5c8fbe2e68 
 
 Diff: https://reviews.apache.org/r/28193/diff/
 
 
 Testing
 ---
 
 ./gradlew clean build -Pq
 
 
 Thanks,
 
 Zameer Manji

Re: Review Request 28310: Extract a cluster state abstraction from PreemptorImpl.


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

Ship it!



src/main/java/org/apache/aurora/scheduler/async/Preemptor.java
https://reviews.apache.org/r/28310/#comment104668

newline?



src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java
https://reviews.apache.org/r/28310/#comment104669

Reads awkward. Was this supposed to be a TODO?


- Maxim Khutornenko


On Nov. 21, 2014, 6:28 p.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28310/
 ---
 
 (Updated Nov. 21, 2014, 6:28 p.m.)
 
 
 Review request for Aurora, Maxim Khutornenko and Zameer Manji.
 
 
 Bugs: AURORA-121
 https://issues.apache.org/jira/browse/AURORA-121
 
 
 Repository: aurora
 
 
 Description
 ---
 
 This is to pull out the bits that will be cached for performance improvements.
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
 19bf162586b90329e764cacf992df37ca68b0dc0 
   src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 
 4e37f4c9c8d4cde477a96a9b8cca7a075f170919 
   src/main/java/org/apache/aurora/scheduler/async/Preemptor.java 
 97d5d13fcb686c7199ccf92ddab04931d6d5ab57 
   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
 f247ccfb115d4daa10fd4ca46750f708a093791b 
   src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java 
 PRE-CREATION 
   
 src/main/java/org/apache/aurora/scheduler/async/preemptor/LiveClusterState.java
  PRE-CREATION 
   
 src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
  PRE-CREATION 
   src/test/java/org/apache/aurora/scheduler/async/AsyncModuleTest.java 
 962aff8f4fa590935773c9fe90b1a6f59bc1c51f 
   src/test/java/org/apache/aurora/scheduler/async/PreemptorImplTest.java 
 69108cf93e8478b54b0292c27ff56074448ec793 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
 17f2d77bd1a143a9388ff1f52fa8edfcdfe08d91 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
 012804a72fddb3e05f01d7e556cd577c07614ede 
   
 src/test/java/org/apache/aurora/scheduler/async/preemptor/LiveClusterStateTest.java
  PRE-CREATION 
   
 src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModuleTest.java
  PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/28310/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Bill Farner

Re: Review Request 28310: Extract a cluster state abstraction from PreemptorImpl.



 On Nov. 21, 2014, 6:48 p.m., Maxim Khutornenko wrote:
  src/main/java/org/apache/aurora/scheduler/async/Preemptor.java, line 301
  https://reviews.apache.org/r/28310/diff/3/?file=772285#file772285line301
 
  newline?
 
 Bill Farner wrote:
 Not sure what you're requesting here.  Where would you like to see a 
 newline?

This line is a continuation, so I expected a newline after it as per our 
guidelines.


- Maxim


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


On Nov. 21, 2014, 7:42 p.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28310/
 ---
 
 (Updated Nov. 21, 2014, 7:42 p.m.)
 
 
 Review request for Aurora, Maxim Khutornenko and Zameer Manji.
 
 
 Bugs: AURORA-121
 https://issues.apache.org/jira/browse/AURORA-121
 
 
 Repository: aurora
 
 
 Description
 ---
 
 This is to pull out the bits that will be cached for performance improvements.
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
 19bf162586b90329e764cacf992df37ca68b0dc0 
   src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java 
 4e37f4c9c8d4cde477a96a9b8cca7a075f170919 
   src/main/java/org/apache/aurora/scheduler/async/Preemptor.java 
 97d5d13fcb686c7199ccf92ddab04931d6d5ab57 
   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
 f247ccfb115d4daa10fd4ca46750f708a093791b 
   src/main/java/org/apache/aurora/scheduler/async/preemptor/ClusterState.java 
 PRE-CREATION 
   
 src/main/java/org/apache/aurora/scheduler/async/preemptor/LiveClusterState.java
  PRE-CREATION 
   
 src/main/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModule.java
  PRE-CREATION 
   src/test/java/org/apache/aurora/scheduler/async/AsyncModuleTest.java 
 962aff8f4fa590935773c9fe90b1a6f59bc1c51f 
   src/test/java/org/apache/aurora/scheduler/async/PreemptorImplTest.java 
 69108cf93e8478b54b0292c27ff56074448ec793 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 
 17f2d77bd1a143a9388ff1f52fa8edfcdfe08d91 
   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
 012804a72fddb3e05f01d7e556cd577c07614ede 
   
 src/test/java/org/apache/aurora/scheduler/async/preemptor/LiveClusterStateTest.java
  PRE-CREATION 
   
 src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorModuleTest.java
  PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/28310/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Bill Farner

Re: Review Request 28193: Prevent Aurora from creating zero sized Executor tasks.



 On Nov. 21, 2014, 6:41 p.m., Maxim Khutornenko wrote:
  src/main/java/org/apache/aurora/scheduler/configuration/Resources.java, 
  line 196
  https://reviews.apache.org/r/28193/diff/2/?file=772028#file772028line196
 
  This is only used in tests outside of this class. Consider reverting to 
  private.
 
 Zameer Manji wrote:
 I think using this constant in tests makes the tests a bit simplier. I 
 have added a '@VisibleForTesting' annotation to signifiy this.

Using @VisibleForTesting is rather an exception when you want to reuse the 
complex definition. You already re-define SOME_EXECUTOR_OVERHEAD for test 
purposes, why not do the same for NO_EXECUTOR_OVERHEAD?


 On Nov. 21, 2014, 6:41 p.m., Maxim Khutornenko wrote:
  src/main/java/org/apache/aurora/scheduler/configuration/Resources.java, 
  line 406
  https://reviews.apache.org/r/28193/diff/2/?file=772028#file772028line406
 
  +1 on moving it closer to its only consumer. That's a general guideline 
  we follow everywhere.
 
 Zameer Manji wrote:
 I really think it should belong with the Resources class because it is 
 equally as useful as .sum in my opinion. If you disagree I will move it 
 closer to the consumer.

You can always move it there when there is a use case. Until then, it's better 
follow our style any open up only those things that are used in more than one 
place.


 On Nov. 21, 2014, 6:41 p.m., Maxim Khutornenko wrote:
  src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line 
  94
  https://reviews.apache.org/r/28193/diff/2/?file=772029#file772029line94
 
  Why not MIN_EXECUTOR_RESOURCES? We normally abstract out from the 
  process framework concept in the scheduler code.
 
 Zameer Manji wrote:
 These minimum values are for thermos. Another executor might require more 
 resources to function.

Did not we want to eliminate it completely though but Mesos did not let us do 
that? I suggest we just use a default and abstract MIN_EXECUTOR_RESOURCES and 
address the real need to differentiate when/if it comes up. Also, when 
https://reviews.apache.org/r/28345/ lands, the 100MB will become more like 0.5 
MB, so it clearly feels like an arbitrary Mesos workaround rather than a true 
MIN enforcement.


 On Nov. 21, 2014, 6:41 p.m., Maxim Khutornenko wrote:
  src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, 
  lines 166-173
  https://reviews.apache.org/r/28193/diff/2/?file=772029#file772029line166
 
  Unless I am missing something, all you need to do here is to make sure 
  neither containerResources nor executorResources is less than min. Can you 
  do something like:
  finalTaskResources = Resources.maxElements(containerResources, 
  MIN_TASK_RESOURCES);
  
  and replace .addAllResources(MIN_THERMOS_RESOURCES.toResourceList()) 
  with
  .addAllResources(Resources.maxElements(executorOverhead, 
  MIN_THERMOS_RESOURCES))?
 
 Zameer Manji wrote:
 I would always like to allocate MIN_THERMOS_RESOURCES for the executor. 
 What you are proposing will make it possible to allocate more CPU or RAM. 
 This is a change in behaviour from before where we were always allocated a 
 fixed amount for the executor.
 
 I can change it to this if you insist but I prefer to allocate a fixed 
 amount for the executor.

Valid point. Though given the randomness of the applied MIN requirement I am 
not sure how important it is. I would go with a more readable and simple 
approach here. Your call.


- Maxim


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


On Nov. 21, 2014, 8:34 p.m., Zameer Manji wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28193/
 ---
 
 (Updated Nov. 21, 2014, 8:34 p.m.)
 
 
 Review request for Aurora, Maxim Khutornenko and Bill Farner.
 
 
 Bugs: AURORA-928
 https://issues.apache.org/jira/browse/AURORA-928
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Mesos rejects tasks and executors that are zero sized. This patch 
 reconfigures Aurora to ensure no zero sized tasks and executors are created.
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
 ed60447c798a97daceda4a3bba6ee9bcdcaedd0f 
   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
 40b652c679d8e340f585e28cbed066335d9d760d 
   src/main/java/org/apache/aurora/scheduler/configuration/Resources.java 
 65c4b526c89a4d5607af4424ebe49bb48e296ae9 
   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
 bb227fd86f7c4c692f6ae2aef1c15a94913354b7 
   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
 899416fceae498353880012b8a93491cff461064 
   src/test/java/org/apache

Re: Review Request 28345: Move thermos_runner out of the apache.aurora.executor package.


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

Ship it!


it also makes thermos_runner.pex 566k instead of 70MB - awesome!

- Maxim Khutornenko


On Nov. 21, 2014, 7:52 p.m., Brian Wickman wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28345/
 ---
 
 (Updated Nov. 21, 2014, 7:52 p.m.)
 
 
 Review request for Aurora, Bhuvan Arumugam, Joshua Cohen, and Maxim 
 Khutornenko.
 
 
 Bugs: AURORA-823
 https://issues.apache.org/jira/browse/AURORA-823
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Move thermos_runner out of the apache.aurora.executor package.
 
 The drawback of having it in the apache.aurora.executor package is that if we 
 want to build a thermos_runner binary from the apache.aurora.executor 
 distribution, it transitively pulls in mesos.native, making the 
 thermos_runner pex like 70MB.  thermos_runner.pex really only has 
 dependencies within apache.thermos and the thermos_runner exit-code contract 
 is a bit more clear now, so putting it into apache.thermos makes a bit more 
 sense now.  it also makes thermos_runner.pex 566k instead of 70MB.  it also 
 fixes AURORA-823.
 
 
 Diffs
 -
 
   examples/vagrant/aurorabuild.sh 4d51cee1c19ed677e08000f03de1c9050e85832b 
   src/main/python/apache/aurora/executor/BUILD 
 ca4193d31e3e3a71fa45f918058fba40fc911487 
   src/main/python/apache/aurora/executor/bin/BUILD 
 a41cf950b707ac47486b7f869edba1fc9e42 
   src/main/python/apache/aurora/executor/bin/thermos_runner_main.py 
 f61caf038db0be4decc4acfe4a2a378de28be50b 
   src/main/python/apache/aurora/executor/thermos_runner.py 
 e10f43896bda4c3ed3c879168e42e104aff89483 
   src/main/python/apache/aurora/executor/thermos_statuses.py  
   src/main/python/apache/aurora/executor/thermos_task_runner.py 
 9a2faa06325c23c41b64f3d0a1422d849c03f278 
   src/main/python/apache/thermos/bin/BUILD 
 34e2b3fe1d1a49cc51db8f61339a21ff8e03e0f4 
   src/main/python/apache/thermos/common/BUILD 
 918800b4ccb7bef61249d488ca8a4255168311bb 
   src/test/python/apache/aurora/executor/test_thermos_executor.py 
 16a40111da055268f7ab396a3a4ea38176c526a3 
   src/test/python/apache/aurora/executor/test_thermos_task_runner.py 
 4fc9d4b7eced0a5d65d09be6c66e30f555d8734b 
 
 Diff: https://reviews.apache.org/r/28345/diff/
 
 
 Testing
 ---
 
 build-support/jenkins/build.sh
 
 firing off an integration test run right now.
 
 
 Thanks,
 
 Brian Wickman

Review Request 28350: Add cron replace command.


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

Review request for Aurora, David McLaughlin and Bill Farner.


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


Repository: aurora


Description
---

Implementing aurora cron replace command to close the functionality gap that 
will be created with client updater removal.


Diffs
-

  docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7 
  docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72 
  src/main/python/apache/aurora/client/api/__init__.py 
d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59 
  src/main/python/apache/aurora/client/cli/cron.py 
cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2 
  src/main/python/apache/aurora/client/cli/update.py 
12774af8bcd1c953fdbc799b0a142c27407d69f5 
  src/test/python/apache/aurora/client/api/test_api.py 
1f4e9fe9111ac88726d7c45b699b3b91438448b6 
  src/test/python/apache/aurora/client/cli/test_cron.py 
c748212febf5867f5f7cc54e34bf91a8890d 
  src/test/python/apache/aurora/client/cli/test_supdate.py 
7637352feea6b07408256158814c05bc17ec14f3 

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


Testing
---

./pants src/test/python:all

tested in vagrant as well


Thanks,

Maxim Khutornenko

Re: Review Request 28350: Add cron replace command.



 On Nov. 21, 2014, 11:26 p.m., Kevin Sweeney wrote:
  docs/cron-jobs.md, line 89
  https://reviews.apache.org/r/28350/diff/1/?file=772861#file772861line89
 
  Hold on a sec - why do we have this and cron schedule (see above)?

The cron replace is an atomic convenience command for cron deschedule + 
cron schedule. It finalizes the cron/service split:
job create  cron schedule
job killall  cron deschedule (with caveat that active tasks are not killed)
job update  cron replace


- Maxim


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


On Nov. 21, 2014, 11:14 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28350/
 ---
 
 (Updated Nov. 21, 2014, 11:14 p.m.)
 
 
 Review request for Aurora, David McLaughlin and Bill Farner.
 
 
 Bugs: AURORA-921
 https://issues.apache.org/jira/browse/AURORA-921
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Implementing aurora cron replace command to close the functionality gap 
 that will be created with client updater removal.
 
 
 Diffs
 -
 
   docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7 
   docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72 
   src/main/python/apache/aurora/client/api/__init__.py 
 d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59 
   src/main/python/apache/aurora/client/cli/cron.py 
 cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2 
   src/main/python/apache/aurora/client/cli/update.py 
 12774af8bcd1c953fdbc799b0a142c27407d69f5 
   src/test/python/apache/aurora/client/api/test_api.py 
 1f4e9fe9111ac88726d7c45b699b3b91438448b6 
   src/test/python/apache/aurora/client/cli/test_cron.py 
 c748212febf5867f5f7cc54e34bf91a8890d 
   src/test/python/apache/aurora/client/cli/test_supdate.py 
 7637352feea6b07408256158814c05bc17ec14f3 
 
 Diff: https://reviews.apache.org/r/28350/diff/
 
 
 Testing
 ---
 
 ./pants src/test/python:all
 
 tested in vagrant as well
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28350: Add cron replace command.



 On Nov. 21, 2014, 11:34 p.m., David McLaughlin wrote:
  src/test/python/apache/aurora/client/cli/test_cron.py, lines 199-215
  https://reviews.apache.org/r/28350/diff/1/?file=772866#file772866line199
 
  This is an integration test. Can we replace this with a more 
  fine-grained unit test that only tests against the Replace verb you added? 
  There is an example of this for beta-update start in your review. You'll be 
  able to remove the use of mock.patch and the creation of a temporary file 
  if you do.

I am a bit concerned about this spread of different approaches in command 
testing but in this particular case it clearly made it simpler. Done.


- Maxim


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


On Nov. 21, 2014, 11:14 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28350/
 ---
 
 (Updated Nov. 21, 2014, 11:14 p.m.)
 
 
 Review request for Aurora, David McLaughlin and Bill Farner.
 
 
 Bugs: AURORA-921
 https://issues.apache.org/jira/browse/AURORA-921
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Implementing aurora cron replace command to close the functionality gap 
 that will be created with client updater removal.
 
 
 Diffs
 -
 
   docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7 
   docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72 
   src/main/python/apache/aurora/client/api/__init__.py 
 d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59 
   src/main/python/apache/aurora/client/cli/cron.py 
 cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2 
   src/main/python/apache/aurora/client/cli/update.py 
 12774af8bcd1c953fdbc799b0a142c27407d69f5 
   src/test/python/apache/aurora/client/api/test_api.py 
 1f4e9fe9111ac88726d7c45b699b3b91438448b6 
   src/test/python/apache/aurora/client/cli/test_cron.py 
 c748212febf5867f5f7cc54e34bf91a8890d 
   src/test/python/apache/aurora/client/cli/test_supdate.py 
 7637352feea6b07408256158814c05bc17ec14f3 
 
 Diff: https://reviews.apache.org/r/28350/diff/
 
 
 Testing
 ---
 
 ./pants src/test/python:all
 
 tested in vagrant as well
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28350: Add cron replace command.



 On Nov. 21, 2014, 11:32 p.m., Bill Farner wrote:
  src/test/python/apache/aurora/client/cli/test_cron.py, lines 218-227
  https://reviews.apache.org/r/28350/diff/1/?file=772866#file772866line218
 
  This block is repeated ~verbatim 4x in this file.  Cleaning up the 
  underlying code to improve testing is probably too high of a bar, but you 
  could at least extract a helper function to minimize the spread.

Obviated by refactoring.


 On Nov. 21, 2014, 11:32 p.m., Bill Farner wrote:
  docs/cron-jobs.md, line 90
  https://reviews.apache.org/r/28350/diff/1/?file=772861#file772861line90
 
  This makes it seem like only the _execution schedule_ is changed by 
  this command, not the template.  Suggest rewording to mkae it more obvious 
  that this will update subsequent cron executions.

Good point. Reworded.


- Maxim


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


On Nov. 21, 2014, 11:14 p.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28350/
 ---
 
 (Updated Nov. 21, 2014, 11:14 p.m.)
 
 
 Review request for Aurora, David McLaughlin and Bill Farner.
 
 
 Bugs: AURORA-921
 https://issues.apache.org/jira/browse/AURORA-921
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Implementing aurora cron replace command to close the functionality gap 
 that will be created with client updater removal.
 
 
 Diffs
 -
 
   docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7 
   docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72 
   src/main/python/apache/aurora/client/api/__init__.py 
 d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59 
   src/main/python/apache/aurora/client/cli/cron.py 
 cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2 
   src/main/python/apache/aurora/client/cli/update.py 
 12774af8bcd1c953fdbc799b0a142c27407d69f5 
   src/test/python/apache/aurora/client/api/test_api.py 
 1f4e9fe9111ac88726d7c45b699b3b91438448b6 
   src/test/python/apache/aurora/client/cli/test_cron.py 
 c748212febf5867f5f7cc54e34bf91a8890d 
   src/test/python/apache/aurora/client/cli/test_supdate.py 
 7637352feea6b07408256158814c05bc17ec14f3 
 
 Diff: https://reviews.apache.org/r/28350/diff/
 
 
 Testing
 ---
 
 ./pants src/test/python:all
 
 tested in vagrant as well
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28350: Add cron replace command.


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

(Updated Nov. 22, 2014, 12:54 a.m.)


Review request for Aurora, David McLaughlin and Bill Farner.


Changes
---

CR comments.


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


Repository: aurora


Description
---

Implementing aurora cron replace command to close the functionality gap that 
will be created with client updater removal.


Diffs (updated)
-

  docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7 
  docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72 
  src/main/python/apache/aurora/client/api/__init__.py 
d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59 
  src/main/python/apache/aurora/client/cli/cron.py 
cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2 
  src/main/python/apache/aurora/client/cli/update.py 
12774af8bcd1c953fdbc799b0a142c27407d69f5 
  src/test/python/apache/aurora/client/api/test_api.py 
1f4e9fe9111ac88726d7c45b699b3b91438448b6 
  src/test/python/apache/aurora/client/cli/test_cron.py 
c748212febf5867f5f7cc54e34bf91a8890d 
  src/test/python/apache/aurora/client/cli/test_supdate.py 
7637352feea6b07408256158814c05bc17ec14f3 
  src/test/python/apache/aurora/client/cli/util.py 
0ec74e675aaabc7ac0cb28e02f5b8534570b7a49 

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


Testing
---

./pants src/test/python:all

tested in vagrant as well


Thanks,

Maxim Khutornenko

Re: Review Request 28193: Prevent Aurora from creating zero sized Executor tasks.


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

Ship it!


Ship It!

- Maxim Khutornenko


On Nov. 21, 2014, 10:50 p.m., Zameer Manji wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28193/
 ---
 
 (Updated Nov. 21, 2014, 10:50 p.m.)
 
 
 Review request for Aurora, Maxim Khutornenko and Bill Farner.
 
 
 Bugs: AURORA-928
 https://issues.apache.org/jira/browse/AURORA-928
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Mesos rejects tasks and executors that are zero sized. This patch 
 reconfigures Aurora to ensure no zero sized tasks and executors are created.
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
 ed60447c798a97daceda4a3bba6ee9bcdcaedd0f 
   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
 40b652c679d8e340f585e28cbed066335d9d760d 
   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
 bb227fd86f7c4c692f6ae2aef1c15a94913354b7 
   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
 899416fceae498353880012b8a93491cff461064 
   
 src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
 953c1edb6802d8983ab324aa56361e5c8fbe2e68 
 
 Diff: https://reviews.apache.org/r/28193/diff/
 
 
 Testing
 ---
 
 ./gradlew clean build -Pq
 
 
 Thanks,
 
 Zameer Manji

Re: Review Request 28193: Prevent Aurora from creating zero sized Executor tasks.


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


On master now.

- Maxim Khutornenko


On Nov. 21, 2014, 10:50 p.m., Zameer Manji wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28193/
 ---
 
 (Updated Nov. 21, 2014, 10:50 p.m.)
 
 
 Review request for Aurora, Maxim Khutornenko and Bill Farner.
 
 
 Bugs: AURORA-928
 https://issues.apache.org/jira/browse/AURORA-928
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Mesos rejects tasks and executors that are zero sized. This patch 
 reconfigures Aurora to ensure no zero sized tasks and executors are created.
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
 ed60447c798a97daceda4a3bba6ee9bcdcaedd0f 
   src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 
 40b652c679d8e340f585e28cbed066335d9d760d 
   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
 bb227fd86f7c4c692f6ae2aef1c15a94913354b7 
   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
 899416fceae498353880012b8a93491cff461064 
   
 src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
 953c1edb6802d8983ab324aa56361e5c8fbe2e68 
 
 Diff: https://reviews.apache.org/r/28193/diff/
 
 
 Testing
 ---
 
 ./gradlew clean build -Pq
 
 
 Thanks,
 
 Zameer Manji

Re: Review Request 28350: Add cron replace command.



 On Nov. 21, 2014, 11:26 p.m., Kevin Sweeney wrote:
  docs/cron-jobs.md, line 89
  https://reviews.apache.org/r/28350/diff/1/?file=772861#file772861line89
 
  Hold on a sec - why do we have this and cron schedule (see above)?
 
 Maxim Khutornenko wrote:
 The cron replace is an atomic convenience command for cron deschedule 
 + cron schedule. It finalizes the cron/service split:
 job create  cron schedule
 job killall  cron deschedule (with caveat that active tasks are not 
 killed)
 job update  cron replace
 
 Kevin Sweeney wrote:
 cron schedule does both job create and job update

What..? I completely forgot that scheduleCronJob actually acts as upsert. 
Should we move the update functionality from it in favor of 
replaceCronTemplate and have our client `cron` commands consistent with `job` 
commands though? I can argue both ways. Any thoughts?


- Maxim


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


On Nov. 22, 2014, 12:54 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28350/
 ---
 
 (Updated Nov. 22, 2014, 12:54 a.m.)
 
 
 Review request for Aurora, David McLaughlin and Bill Farner.
 
 
 Bugs: AURORA-921
 https://issues.apache.org/jira/browse/AURORA-921
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Implementing aurora cron replace command to close the functionality gap 
 that will be created with client updater removal.
 
 
 Diffs
 -
 
   docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7 
   docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72 
   src/main/python/apache/aurora/client/api/__init__.py 
 d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59 
   src/main/python/apache/aurora/client/cli/cron.py 
 cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2 
   src/main/python/apache/aurora/client/cli/update.py 
 12774af8bcd1c953fdbc799b0a142c27407d69f5 
   src/test/python/apache/aurora/client/api/test_api.py 
 1f4e9fe9111ac88726d7c45b699b3b91438448b6 
   src/test/python/apache/aurora/client/cli/test_cron.py 
 c748212febf5867f5f7cc54e34bf91a8890d 
   src/test/python/apache/aurora/client/cli/test_supdate.py 
 7637352feea6b07408256158814c05bc17ec14f3 
   src/test/python/apache/aurora/client/cli/util.py 
 0ec74e675aaabc7ac0cb28e02f5b8534570b7a49 
 
 Diff: https://reviews.apache.org/r/28350/diff/
 
 
 Testing
 ---
 
 ./pants src/test/python:all
 
 tested in vagrant as well
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28350: Add cron replace command.



 On Nov. 21, 2014, 11:26 p.m., Kevin Sweeney wrote:
  docs/cron-jobs.md, line 89
  https://reviews.apache.org/r/28350/diff/1/?file=772861#file772861line89
 
  Hold on a sec - why do we have this and cron schedule (see above)?
 
 Maxim Khutornenko wrote:
 The cron replace is an atomic convenience command for cron deschedule 
 + cron schedule. It finalizes the cron/service split:
 job create  cron schedule
 job killall  cron deschedule (with caveat that active tasks are not 
 killed)
 job update  cron replace
 
 Kevin Sweeney wrote:
 cron schedule does both job create and job update
 
 Maxim Khutornenko wrote:
 What..? I completely forgot that scheduleCronJob actually acts as 
 upsert. Should we move the update functionality from it in favor of 
 replaceCronTemplate and have our client `cron` commands consistent with `job` 
 commands though? I can argue both ways. Any thoughts?
 
 Kevin Sweeney wrote:
 I was completely surprised to learn that when I was writing this document 
 as well.
 
 I'm in favor of leaving the existing behavior as-is and removing any 
 unused endpoints.

There are no unused endpoints yet. The replaceCronTemplate is still used by the 
client updater.


- Maxim


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


On Nov. 22, 2014, 12:54 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28350/
 ---
 
 (Updated Nov. 22, 2014, 12:54 a.m.)
 
 
 Review request for Aurora, David McLaughlin and Bill Farner.
 
 
 Bugs: AURORA-921
 https://issues.apache.org/jira/browse/AURORA-921
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Implementing aurora cron replace command to close the functionality gap 
 that will be created with client updater removal.
 
 
 Diffs
 -
 
   docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7 
   docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72 
   src/main/python/apache/aurora/client/api/__init__.py 
 d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59 
   src/main/python/apache/aurora/client/cli/cron.py 
 cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2 
   src/main/python/apache/aurora/client/cli/update.py 
 12774af8bcd1c953fdbc799b0a142c27407d69f5 
   src/test/python/apache/aurora/client/api/test_api.py 
 1f4e9fe9111ac88726d7c45b699b3b91438448b6 
   src/test/python/apache/aurora/client/cli/test_cron.py 
 c748212febf5867f5f7cc54e34bf91a8890d 
   src/test/python/apache/aurora/client/cli/test_supdate.py 
 7637352feea6b07408256158814c05bc17ec14f3 
   src/test/python/apache/aurora/client/cli/util.py 
 0ec74e675aaabc7ac0cb28e02f5b8534570b7a49 
 
 Diff: https://reviews.apache.org/r/28350/diff/
 
 
 Testing
 ---
 
 ./pants src/test/python:all
 
 tested in vagrant as well
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28361: Extract thrift into an API subproject.


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



build-support/thrift/thriftw
https://reviews.apache.org/r/28361/#comment104795

License header?



build-support/thrift/thriftw
https://reviews.apache.org/r/28361/#comment104796

I don't see where EXPECTED_THRIFT_VERSION is used.



build.gradle
https://reviews.apache.org/r/28361/#comment104793

whitespaces?



build.gradle
https://reviews.apache.org/r/28361/#comment104794

revert or remove



buildSrc/src/main/groovy/org/apache/aurora/build/ThriftEntitiesPlugin.groovy
https://reviews.apache.org/r/28361/#comment104797

This is also defined in build.gradle. Any reason it has to be in both 
places?



buildSrc/src/main/groovy/org/apache/aurora/build/ThriftEntitiesPlugin.groovy
https://reviews.apache.org/r/28361/#comment104798

same here


- Maxim Khutornenko


On Nov. 22, 2014, 1:35 a.m., Kevin Sweeney wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28361/
 ---
 
 (Updated Nov. 22, 2014, 1:35 a.m.)
 
 
 Review request for Aurora, Joshua Cohen, Bill Farner, and Zameer Manji.
 
 
 Bugs: AURORA-925
 https://issues.apache.org/jira/browse/AURORA-925
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Extract thrift generation into an api subproject.
 
 
 Diffs
 -
 
   .gitignore ec072d2cf7d9678c59d2f67ec3ab1fda3efd88b7 
   BUILD b6d7ce983dfbb99dd7e26e547a4992a27a48ccdf 
   build-support/python/make-pycharm-virtualenv 
 8f58d4df650892aff987ccfe47a9580023b8cf63 
   build-support/release/make-python-sdists 
 e0d20a19ef1fbc129a882b6368515a3ea41bab3f 
   build-support/thrift/thriftw PRE-CREATION 
   build.gradle 6894ceb7055c0491dce1c869fad0371d432f6540 
   buildSrc/src/main/groovy/org/apache/aurora/CoverageReportCheck.groovy 
 f233153fd093cae255c1cb6807cfec6590ba36f9 
   
 buildSrc/src/main/groovy/org/apache/aurora/build/ThriftEntitiesPlugin.groovy 
 PRE-CREATION 
   buildSrc/src/main/groovy/org/apache/aurora/build/ThriftPlugin.groovy 
 PRE-CREATION 
   gradle/wrapper/gradle-wrapper.properties 
 b04300260fd3975ec98a1a1d87b57025e7904c2f 
   settings.gradle PRE-CREATION 
   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
 4b9281590a9eeb8a8b571b909fd507259abfac44 
   src/main/java/org/apache/aurora/scheduler/http/api/ApiBeta.java 
 c072010ed5a1856a05fa7e6c87b25c073059 
   src/main/python/apache/aurora/admin/BUILD 
 9441e121bf4ea88a7ae286f8cd1cbd0c0ac89e06 
   src/main/python/apache/aurora/client/BUILD 
 48566d99f432e6eb94d1bfdb18277205fd7bb9d8 
   src/main/python/apache/aurora/client/api/BUILD 
 6d2a1bfe8531d800be651ec00518fc5b07a53474 
   src/main/python/apache/aurora/client/cli/BUILD 
 e6627a8a3c501292fdd31ec384320870db702bc2 
   src/main/python/apache/aurora/client/commands/BUILD 
 d146015d70715142618f2653538aca6beb83c1fc 
   src/main/python/apache/aurora/client/hooks/BUILD 
 f46cf650d9b471d08ed2b76652bca5d429f955ee 
   src/main/python/apache/aurora/common/BUILD 
 02ec17ce35c6632df204ea4d1d12e61daf30dd1f 
   src/main/python/apache/aurora/common/auth/BUILD 
 c26d117122d17b2228b637441ce0aa564f5a8a3e 
   src/main/python/apache/aurora/config/BUILD 
 fa40ebdfdecae3eb13878d6f172a591c16507530 
   src/main/python/apache/aurora/config/schema/BUILD 
 157c141bdf968666f31452784967de2a640d1815 
   src/main/python/apache/aurora/executor/BUILD 
 ca4193d31e3e3a71fa45f918058fba40fc911487 
   src/main/python/apache/aurora/executor/common/BUILD 
 d33e14b1cf7be7d115c7fe7741d52896f02f3aed 
   src/main/python/apache/aurora/tools/java/thrift_wrapper_codegen.py 
 d6bb5a120bb6368305ab52d253d25ad2bcfb3e8b 
   src/main/python/apache/thermos/bin/BUILD 
 34e2b3fe1d1a49cc51db8f61339a21ff8e03e0f4 
   src/main/python/apache/thermos/common/BUILD 
 918800b4ccb7bef61249d488ca8a4255168311bb 
   src/main/python/apache/thermos/core/BUILD 
 f362acdc395f1e479bcaf4bc063456abb1fdb0e2 
   src/main/python/apache/thermos/monitoring/BUILD 
 0dad47e6337b7db33dca793f0324892d61e3433a 
   src/main/python/apache/thermos/observer/BUILD 
 b07db90906a06779f5369158469651425fefa1a3 
   src/main/python/apache/thermos/testing/BUILD 
 b96c166e9bc529b783071b5d37b78779a36d06c3 
   src/main/thrift/org/apache/aurora/gen/BUILD  
   src/main/thrift/org/apache/aurora/gen/api.thrift  
   src/main/thrift/org/apache/aurora/gen/internal_rpc.thrift  
   src/main/thrift/org/apache/aurora/gen/storage.thrift  
   src/main/thrift/org/apache/aurora/gen/test.thrift  
   src/main/thrift/org/apache/thermos/BUILD  
   src/main/thrift/org/apache/thermos/thermos_internal.thrift  
   src/test/python/apache/aurora/admin/BUILD 
 101aaa068f2a134f0e846359dc11c7a0ccc28dbb 
   src/test/python/apache/aurora/client/api/BUILD

Re: Review Request 27705: Adding instrumentation into the scheduling pipeline.


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


@ReviewBot retry

- Maxim Khutornenko


On Nov. 20, 2014, 3:01 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/27705/
 ---
 
 (Updated Nov. 20, 2014, 3:01 a.m.)
 
 
 Review request for Aurora, Bill Farner and Zameer Manji.
 
 
 Bugs: AURORA-914
 https://issues.apache.org/jira/browse/AURORA-914
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Adding @Timed to trace scheduling latencies and Veto counters per type.
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/TaskVars.java 
 cf8f7584afee758c527798914181049051aef0d8 
   src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 
 d2682cd910d248c897e691bcb4c8a3a6f1aec2d2 
   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
 f247ccfb115d4daa10fd4ca46750f708a093791b 
   src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 
 4821a7890b77ccb04c10bee6d8b4b9e7216940cc 
   src/main/java/org/apache/aurora/scheduler/filter/ConstraintMatcher.java 
 cc8c5b81baf0cf05d7ac04f69ad2834fa9438aac 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
 723e7ab70b8bfd930154dcce385cd1e1d599cf9f 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
 cf3bb644008274672c77753b94f4d50c99a1a49b 
   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
 87203690f09456ac1ca5e9da2b82826d60cbd723 
   src/main/java/org/apache/aurora/scheduler/stats/CachedCounters.java 
 aaedb3b5ec2cb27550449435efa8f335c6a9baad 
   src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
 12ea4c67350c2992f59bacd21a99d1413b60b757 
   
 src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java
  d408ec0f7781f65d5b5f215eced5d6255e53b0dd 
   
 src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
  d1a70661122802ecfdd8efa2ced567685c08995f 
   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
 b60b004adbd6753ec6fef125fd70286be5071c56 
   
 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
  168290af9f84750e66dca69cf056dacb5f38aaa3 
 
 Diff: https://reviews.apache.org/r/27705/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 Verified new stats in vagrant.
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 27705: Adding instrumentation into the scheduling pipeline.


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


@ReviewBot retry - last attempt

- Maxim Khutornenko


On Nov. 20, 2014, 3:01 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/27705/
 ---
 
 (Updated Nov. 20, 2014, 3:01 a.m.)
 
 
 Review request for Aurora, Bill Farner and Zameer Manji.
 
 
 Bugs: AURORA-914
 https://issues.apache.org/jira/browse/AURORA-914
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Adding @Timed to trace scheduling latencies and Veto counters per type.
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/TaskVars.java 
 cf8f7584afee758c527798914181049051aef0d8 
   src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 
 d2682cd910d248c897e691bcb4c8a3a6f1aec2d2 
   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
 f247ccfb115d4daa10fd4ca46750f708a093791b 
   src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 
 4821a7890b77ccb04c10bee6d8b4b9e7216940cc 
   src/main/java/org/apache/aurora/scheduler/filter/ConstraintMatcher.java 
 cc8c5b81baf0cf05d7ac04f69ad2834fa9438aac 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
 723e7ab70b8bfd930154dcce385cd1e1d599cf9f 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
 cf3bb644008274672c77753b94f4d50c99a1a49b 
   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
 87203690f09456ac1ca5e9da2b82826d60cbd723 
   src/main/java/org/apache/aurora/scheduler/stats/CachedCounters.java 
 aaedb3b5ec2cb27550449435efa8f335c6a9baad 
   src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
 12ea4c67350c2992f59bacd21a99d1413b60b757 
   
 src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java
  d408ec0f7781f65d5b5f215eced5d6255e53b0dd 
   
 src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
  d1a70661122802ecfdd8efa2ced567685c08995f 
   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
 b60b004adbd6753ec6fef125fd70286be5071c56 
   
 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
  168290af9f84750e66dca69cf056dacb5f38aaa3 
 
 Diff: https://reviews.apache.org/r/27705/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 Verified new stats in vagrant.
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 28306: Return an iterable of frame chunks.


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



src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java
https://reviews.apache.org/r/28306/#comment104522

Mind commenting on the algorithm here? Why the magic -2?


- Maxim Khutornenko


On Nov. 21, 2014, 12:54 a.m., Kevin Sweeney wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28306/
 ---
 
 (Updated Nov. 21, 2014, 12:54 a.m.)
 
 
 Review request for Aurora, David McLaughlin and Bill Farner.
 
 
 Bugs: AURORA-930
 https://issues.apache.org/jira/browse/AURORA-930
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Stream frame chunks instead of preallocating them.
 
 This avoids allocating an entire additional snapshot worth of heap during 
 entry serialization, reducing overall heap impact of the serializer from 
 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize.
 
 Further optimizations out-of-scope for this change:
 
 * Make the returned iterator mutate a fixed-size buffer (for GC pressure 
 avoidance).
 * Change the log format so that FrameHeader doesn't need to know the size and 
 checksum of the serialized data ahead-of-time (maybe write it as a trailer).
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java 
 f4fa1cb740633ced529c1b5fd9f18abba8944571 
   
 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java 
 cb95d8996a934751745f423b79279266d73b7722 
   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
 c90389433d81dd72756c659736e38fd9f66fcb35 
 
 Diff: https://reviews.apache.org/r/28306/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Kevin Sweeney

Re: Review Request 28306: Return an iterable of frame chunks.



 On Nov. 21, 2014, 1:09 a.m., Maxim Khutornenko wrote:
  src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java, 
  line 92
  https://reviews.apache.org/r/28306/diff/1/?file=771713#file771713line92
 
  Mind commenting on the algorithm here? Why the magic -2?
 
 Kevin Sweeney wrote:
 Not sure what I'd say there that doesn't risk contradicting the code 
 below - we don't usually comment loop variables and the old version had the 
 same amount of commenting.

I just find it harder to follow when I see anything less than -1 as the initial 
condition. Why not starting with -1 (to account for the header) and then 
increment i at the end of computeNext()?


- Maxim


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


On Nov. 21, 2014, 1:10 a.m., Kevin Sweeney wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28306/
 ---
 
 (Updated Nov. 21, 2014, 1:10 a.m.)
 
 
 Review request for Aurora, David McLaughlin and Bill Farner.
 
 
 Bugs: AURORA-930
 https://issues.apache.org/jira/browse/AURORA-930
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Stream frame chunks instead of preallocating them.
 
 This avoids allocating an entire additional snapshot worth of heap during 
 entry serialization, reducing overall heap impact of the serializer from 
 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize.
 
 Further optimizations out-of-scope for this change:
 
 * Make the returned iterator mutate a fixed-size buffer (for GC pressure 
 avoidance).
 * Change the log format so that FrameHeader doesn't need to know the size and 
 checksum of the serialized data ahead-of-time (maybe write it as a trailer).
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java 
 f4fa1cb740633ced529c1b5fd9f18abba8944571 
   
 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java 
 cb95d8996a934751745f423b79279266d73b7722 
   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
 c90389433d81dd72756c659736e38fd9f66fcb35 
 
 Diff: https://reviews.apache.org/r/28306/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Kevin Sweeney

Re: Review Request 28306: Return an iterable of frame chunks.



 On Nov. 21, 2014, 1:09 a.m., Maxim Khutornenko wrote:
  src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java, 
  line 92
  https://reviews.apache.org/r/28306/diff/1/?file=771713#file771713line92
 
  Mind commenting on the algorithm here? Why the magic -2?
 
 Kevin Sweeney wrote:
 Not sure what I'd say there that doesn't risk contradicting the code 
 below - we don't usually comment loop variables and the old version had the 
 same amount of commenting.
 
 Maxim Khutornenko wrote:
 I just find it harder to follow when I see anything less than -1 as the 
 initial condition. Why not starting with -1 (to account for the header) and 
 then increment i at the end of computeNext()?
 
 Kevin Sweeney wrote:
 Each branch short-circuits so I'd have to have a result intermediate 
 value. I don't find that more readable
 
 ```java
 byte[] result;
 if (i == -1) {
   result = header;
 } else if (...) {
   result = encode(...);
 } else {
   return endOfFile();
 }
 
 i++;
 return result;
 ```

I actually find it more readable as your if-else-if-else eliminates the need to 
short-circuit. Anyway, just a nit that forced me to pause for awhile.


- Maxim


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


On Nov. 21, 2014, 1:10 a.m., Kevin Sweeney wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28306/
 ---
 
 (Updated Nov. 21, 2014, 1:10 a.m.)
 
 
 Review request for Aurora, David McLaughlin and Bill Farner.
 
 
 Bugs: AURORA-930
 https://issues.apache.org/jira/browse/AURORA-930
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Stream frame chunks instead of preallocating them.
 
 This avoids allocating an entire additional snapshot worth of heap during 
 entry serialization, reducing overall heap impact of the serializer from 
 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize.
 
 Further optimizations out-of-scope for this change:
 
 * Make the returned iterator mutate a fixed-size buffer (for GC pressure 
 avoidance).
 * Change the log format so that FrameHeader doesn't need to know the size and 
 checksum of the serialized data ahead-of-time (maybe write it as a trailer).
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java 
 f4fa1cb740633ced529c1b5fd9f18abba8944571 
   
 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java 
 cb95d8996a934751745f423b79279266d73b7722 
   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
 c90389433d81dd72756c659736e38fd9f66fcb35 
 
 Diff: https://reviews.apache.org/r/28306/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Kevin Sweeney

Re: Review Request 28306: Return an iterable of frame chunks.


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

Ship it!


Ship It!

- Maxim Khutornenko


On Nov. 21, 2014, 1:10 a.m., Kevin Sweeney wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28306/
 ---
 
 (Updated Nov. 21, 2014, 1:10 a.m.)
 
 
 Review request for Aurora, David McLaughlin and Bill Farner.
 
 
 Bugs: AURORA-930
 https://issues.apache.org/jira/browse/AURORA-930
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Stream frame chunks instead of preallocating them.
 
 This avoids allocating an entire additional snapshot worth of heap during 
 entry serialization, reducing overall heap impact of the serializer from 
 2*sizeof(serialized-entry) to sizeof(serialized-entry)+chunkSize.
 
 Further optimizations out-of-scope for this change:
 
 * Make the returned iterator mutate a fixed-size buffer (for GC pressure 
 avoidance).
 * Change the log format so that FrameHeader doesn't need to know the size and 
 checksum of the serialized data ahead-of-time (maybe write it as a trailer).
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/storage/log/EntrySerializer.java 
 f4fa1cb740633ced529c1b5fd9f18abba8944571 
   
 src/main/java/org/apache/aurora/scheduler/storage/log/StreamManagerImpl.java 
 cb95d8996a934751745f423b79279266d73b7722 
   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
 c90389433d81dd72756c659736e38fd9f66fcb35 
 
 Diff: https://reviews.apache.org/r/28306/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 
 
 Thanks,
 
 Kevin Sweeney

Re: Review Request 28097: Remove ReadWriteLock from MemStorage, remove Storage#weaklyConsistentRead.


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

Ship it!


Ship It!

- Maxim Khutornenko


On Nov. 19, 2014, 11:41 p.m., Bill Farner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/28097/
 ---
 
 (Updated Nov. 19, 2014, 11:41 p.m.)
 
 
 Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
 
 
 Bugs: AURORA-929
 https://issues.apache.org/jira/browse/AURORA-929
 
 
 Repository: aurora
 
 
 Description
 ---
 
 See updated class javadoc in `Storage` for new behavior description.  
 Ultimately we're accepting the risk of reading uncommitted writes 
 (specifically in `TaskStore` and `JobStore`).  In practice this should 
 generally be exceedingly rare, as most writes are atomic to one store anyway, 
 and for those that are not (such as rescheduling a task), the window should 
 be O(µs) long.
 
 Once `DbStorage` owns all of the stores, we have a global knob for 
 transaction isolation: 
 http://mybatis.github.io/mybatis-3/java-api.html#sqlSessions (search that 
 page for `TransactionIsolationLevel`).
 
 Without the ReadWriteLock in play, there is no longer a need for 
 `weaklyConsistentRead`, so i've removed that.
 
 
 Diffs
 -
 
   3rdparty/python/BUILD 63ae77f4632db158109e8d562488ce4e84da0438 
   build.gradle f4b4e0d28962b6a1d5802ab9fcd6b4d49afbf360 
   src/main/java/org/apache/aurora/auth/SessionValidator.java 
 eeebb78901a6c33e08ceb8e675c91f0b5f44bcbc 
   src/main/java/org/apache/aurora/scheduler/TaskVars.java 
 cf8f7584afee758c527798914181049051aef0d8 
   src/main/java/org/apache/aurora/scheduler/async/GcExecutorLauncher.java 
 e02921df3a217aa2678e91a8bebe6a3708dbc9c3 
   src/main/java/org/apache/aurora/scheduler/async/KillRetry.java 
 d6b7fabe6642944e9fda87c5588029c5bb32c025 
   src/main/java/org/apache/aurora/scheduler/async/Preemptor.java 
 ff26c49729646ffe052cb0a993b9984ae96a89ac 
   src/main/java/org/apache/aurora/scheduler/async/RescheduleCalculator.java 
 ca54c9aa321c831abdbdb8bc1f8c06a0bff95ee2 
   src/main/java/org/apache/aurora/scheduler/async/TaskHistoryPruner.java 
 58d074b491dab2e2e0ce8a8a57e4ebdaf0984e73 
   
 src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
  8cf845f3622392a65216e0c29084965c7c64075d 
   
 src/main/java/org/apache/aurora/scheduler/configuration/SanitizedConfiguration.java
  4eb4437b91906ae191dd105474e84ba5f40cf52e 
   src/main/java/org/apache/aurora/scheduler/cron/SanitizedCronJob.java 
 babbf4203af6f405d4193d6feaa749232e553ae9 
   src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 
 84e37e49fabc7ac6bfa2967787cb6abb6ce4af5d 
   
 src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java 
 8e2d3d92c3404f9b24b802687c2d7faeeb27d318 
   src/main/java/org/apache/aurora/scheduler/http/Maintenance.java 
 be8a1fff2db77414dd04637af4a8183810b66845 
   src/main/java/org/apache/aurora/scheduler/http/Mname.java 
 883f954e5dc99311fb701d52dd5d737a50f23276 
   src/main/java/org/apache/aurora/scheduler/http/Quotas.java 
 5f3cce330a9ec7eb3bdfb3512791d918d4c9 
   src/main/java/org/apache/aurora/scheduler/http/Slaves.java 
 0ea462f5d1abbb9eec457205e1b9acca9976027a 
   src/main/java/org/apache/aurora/scheduler/http/StructDump.java 
 8147d545dce3082d63f693c9620e3c769258d9e4 
   src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 
 88150e564e70bf02b62c1c7477d126e98dd91437 
   src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 
 e38407ec487d511fb05142cbeda955ec5a6ba4ec 
   src/main/java/org/apache/aurora/scheduler/sla/MetricCalculator.java 
 149bb33568d67fffdee944bf676199e7108b0c0d 
   src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 
 f167290bf99b76ccc049eb51fe95ccfce940d078 
   src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java 
 a835eaabb047ede5833ca80f43a0cb1bee01d142 
   src/main/java/org/apache/aurora/scheduler/stats/ResourceCounter.java 
 79d12b0dd7959b5443ffce43d9ebdb79135718bb 
   
 src/main/java/org/apache/aurora/scheduler/storage/CallOrderEnforcingStorage.java
  0d02207d7bda46bcc84d2e7328a8c500ad9a5384 
   src/main/java/org/apache/aurora/scheduler/storage/ReadWriteLockManager.java 
 4e6d68b039bb140509b1261f25e7b49457bfd2be 
   src/main/java/org/apache/aurora/scheduler/storage/Storage.java 
 682bca881969d24ece40ab83356f1bebb77feabd 
   src/main/java/org/apache/aurora/scheduler/storage/backup/Recovery.java 
 4744dc9f202969906113ccb610bf17c94d188c43 
   
 src/main/java/org/apache/aurora/scheduler/storage/backup/TemporaryStorage.java
  503bdfeb2cefccfdeb151190b04ce9ce445f47b1 
   src/main/java/org/apache/aurora/scheduler/storage/db/DbStorage.java

Re: Review Request 27705: Adding instrumentation into the scheduling pipeline.



 On Nov. 19, 2014, 10:51 p.m., Bill Farner wrote:
  src/main/java/org/apache/aurora/scheduler/TaskVars.java, line 89
  https://reviews.apache.org/r/27705/diff/4/?file=764617#file764617line89
 
  This should be the only dynamic one, right?  Rack/host limit?  Value 
  constraints can be considered static.

Ah, good catch, reodered names to go alphabetically but did not change the 
values.


- Maxim


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


On Nov. 15, 2014, 12:15 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/27705/
 ---
 
 (Updated Nov. 15, 2014, 12:15 a.m.)
 
 
 Review request for Aurora, Bill Farner and Zameer Manji.
 
 
 Bugs: AURORA-914
 https://issues.apache.org/jira/browse/AURORA-914
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Adding @Timed to trace scheduling latencies and Veto counters per type.
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/TaskVars.java 
 cf8f7584afee758c527798914181049051aef0d8 
   src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 
 d2682cd910d248c897e691bcb4c8a3a6f1aec2d2 
   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
 6bfa3ac425ed3045fa60d1b0ca547e9bf3cde37a 
   src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 
 4821a7890b77ccb04c10bee6d8b4b9e7216940cc 
   src/main/java/org/apache/aurora/scheduler/filter/ConstraintFilter.java 
 3839083f27ca5d4b93406152559b58b04e912a10 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
 c1c5f26723f1eac3000e09e061b4582f922fded6 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
 cc6b53b3265253f76c1e954c0108aa5936f5cc36 
   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
 87203690f09456ac1ca5e9da2b82826d60cbd723 
   src/main/java/org/apache/aurora/scheduler/stats/CachedCounters.java 
 aaedb3b5ec2cb27550449435efa8f335c6a9baad 
   src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
 12ea4c67350c2992f59bacd21a99d1413b60b757 
   
 src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java
  94f0a179b786649775899f855f7c1a0caab7290f 
   
 src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
  e113eba1f304279b5ee3d70db1d1ea558efd63ac 
   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
 b60b004adbd6753ec6fef125fd70286be5071c56 
   
 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
  608903268a0a0d67711bfdc81d2e5b29c335ead2 
 
 Diff: https://reviews.apache.org/r/27705/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 Verified new stats in vagrant.
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 27705: Adding instrumentation into the scheduling pipeline.



 On Nov. 19, 2014, 10:51 p.m., Bill Farner wrote:
  src/main/java/org/apache/aurora/scheduler/TaskVars.java, line 252
  https://reviews.apache.org/r/27705/diff/4/?file=764617#file764617line252
 
  I don't have strong data to back this up, but i'm concerned about the 
  performance impact here.  In a large/busy cluster, this could be invoked 
  O(100k) times per second.  Can you avoid the Set creation?  Maybe instead 
  just loop and use two flags?
  
  To combat this, we really need to move the nearest fit/miss calculation 
  to the publisher end (which would, unfortunately, break these stats).  For 
  now, i think it makes sense to merely be cognizant of the call frequency.

Refactored to not use any new object creation.


- Maxim


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


On Nov. 15, 2014, 12:15 a.m., Maxim Khutornenko wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/27705/
 ---
 
 (Updated Nov. 15, 2014, 12:15 a.m.)
 
 
 Review request for Aurora, Bill Farner and Zameer Manji.
 
 
 Bugs: AURORA-914
 https://issues.apache.org/jira/browse/AURORA-914
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Adding @Timed to trace scheduling latencies and Veto counters per type.
 
 
 Diffs
 -
 
   src/main/java/org/apache/aurora/scheduler/TaskVars.java 
 cf8f7584afee758c527798914181049051aef0d8 
   src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 
 d2682cd910d248c897e691bcb4c8a3a6f1aec2d2 
   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
 6bfa3ac425ed3045fa60d1b0ca547e9bf3cde37a 
   src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 
 4821a7890b77ccb04c10bee6d8b4b9e7216940cc 
   src/main/java/org/apache/aurora/scheduler/filter/ConstraintFilter.java 
 3839083f27ca5d4b93406152559b58b04e912a10 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java 
 c1c5f26723f1eac3000e09e061b4582f922fded6 
   src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 
 cc6b53b3265253f76c1e954c0108aa5936f5cc36 
   src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 
 87203690f09456ac1ca5e9da2b82826d60cbd723 
   src/main/java/org/apache/aurora/scheduler/stats/CachedCounters.java 
 aaedb3b5ec2cb27550449435efa8f335c6a9baad 
   src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 
 12ea4c67350c2992f59bacd21a99d1413b60b757 
   
 src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java
  94f0a179b786649775899f855f7c1a0caab7290f 
   
 src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
  e113eba1f304279b5ee3d70db1d1ea558efd63ac 
   src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java 
 b60b004adbd6753ec6fef125fd70286be5071c56 
   
 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
  608903268a0a0d67711bfdc81d2e5b29c335ead2 
 
 Diff: https://reviews.apache.org/r/27705/diff/
 
 
 Testing
 ---
 
 ./gradlew -Pq build
 Verified new stats in vagrant.
 
 
 Thanks,
 
 Maxim Khutornenko

Re: Review Request 27705: Adding instrumentation into the scheduling pipeline.