Re: Review Request 18334: Move and unit test Maintenance module and commands

[Joe Smith]

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

(Updated March 3, 2014, 6:50 p.m.)


Review request for Aurora, Kevin Sweeney, Mark Chu-Carroll, Maxim Khutornenko, 
and Brian Wickman.


Changes
---

Updated, and also adding maxim as we've been dabbling in aurora_admin together 
(and I'm moving parse_hosts to base)


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


Repository: aurora


Description
---

As always, feel free to tear it apart.

I plan to add tests for the other commands as well, but wanted to get this out 
sooner than later to ensure others agreed with my approach.


Diffs (updated)
-

  src/main/python/apache/aurora/admin/BUILD 
480dad69d8122168a6b7222ef9df0eb689fae386 
  src/main/python/apache/aurora/admin/mesos_maintenance.py 
d8ffdec062db07ce82556fb38ff4a5eea7921953 
  src/main/python/apache/aurora/client/base.py 
c5c969f9641c8e0e1604648b1f9ed542ab59b7d0 
  src/main/python/apache/aurora/client/bin/BUILD 
dbabfd0e56288d04c399e20976ea6e0287112d91 
  src/main/python/apache/aurora/client/commands/BUILD 
14bbdd4d0cb0e8e5102e75baef4e8b34d98967c0 
  src/main/python/apache/aurora/client/commands/admin.py 
40588e23f5082604af9f474e9cf6ab84663b8d67 
  src/main/python/apache/aurora/client/commands/maintenance.py PRE-CREATION 
  src/test/python/apache/aurora/admin/BUILD 
258b1eb58a4eec1650bfd317823f76a30889f912 
  src/test/python/apache/aurora/admin/test_mesos_maintenance.py 
000fbe5a873669bf55a9110f1aa6f1d7c8350ef8 
  src/test/python/apache/aurora/client/commands/BUILD 
6d448d7320a78bf140873b743805846c179281a7 
  src/test/python/apache/aurora/client/commands/test_maintenance.py 
PRE-CREATION 

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


Testing
---

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

particularly:

src.test.python.apache.aurora.client.commands.maintenance   
.   SUCCESS


Thanks,

Joe Smith

Re: Review Request 18334: Move and unit test Maintenance module and commands

[Joe Smith]

> On Feb. 24, 2014, 6:36 p.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/admin/mesos_maintenance.py, line 73
> > 
> >
> > Inclined to revert this - better to explicitly call out a dependency on 
> > system time IMO.

done


> On Feb. 24, 2014, 6:36 p.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/commands/maintenance.py, lines 53-56
> > 
> >
> > DRY - these options are used multiple times. Consider factoring them as 
> > constants.

fixed


> On Feb. 24, 2014, 6:36 p.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/commands/test_maintenance.py, line 43
> > 
> >
> > make_mock_options would be more descriptive. also docstring is not 
> > informative, consider dropping it.

fixed


> On Feb. 24, 2014, 6:36 p.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/commands/test_maintenance.py, line 88
> > 
> >
> > parens aren't necessary

fixed


> On Feb. 24, 2014, 6:36 p.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/commands/test_maintenance.py, line 103
> > 
> >
> > parens not necessary

fixed


> On Feb. 24, 2014, 6:36 p.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/commands/test_maintenance.py, line 129
> > 
> >
> > no parens needed.

fixed


> On Feb. 24, 2014, 6:36 p.m., Kevin Sweeney wrote:
> > src/test/python/apache/aurora/client/commands/test_maintenance.py, line 136
> > 
> >
> > Not sure how I feel about this style of mock - I'd prefer the class 
> > under test to explicitly call out "I'm using system time" to avoid missed 
> > mock coverage causing the test suite to slow down.

hm.. any chance you have suggestions on how to pass this through? I'd prefer 
not to add it to the command line itself, I think


- Joe


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


On Feb. 24, 2014, 10:19 a.m., Joe Smith wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18334/
> ---
> 
> (Updated Feb. 24, 2014, 10:19 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Mark Chu-Carroll, and Brian Wickman.
> 
> 
> Bugs: AURORA-223
> https://issues.apache.org/jira/browse/AURORA-223
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> As always, feel free to tear it apart.
> 
> I plan to add tests for the other commands as well, but wanted to get this 
> out sooner than later to ensure others agreed with my approach.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/admin/BUILD 
> 480dad69d8122168a6b7222ef9df0eb689fae386 
>   src/main/python/apache/aurora/admin/mesos_maintenance.py 
> d8ffdec062db07ce82556fb38ff4a5eea7921953 
>   src/main/python/apache/aurora/client/bin/BUILD 
> dbabfd0e56288d04c399e20976ea6e0287112d91 
>   src/main/python/apache/aurora/client/commands/BUILD 
> 14bbdd4d0cb0e8e5102e75baef4e8b34d98967c0 
>   src/main/python/apache/aurora/client/commands/admin.py 
> 989c5b625b48fe67ef1297ceda8d7e35cb8ead7e 
>   src/main/python/apache/aurora/client/commands/maintenance.py PRE-CREATION 
>   src/test/python/apache/aurora/admin/BUILD 
> 258b1eb58a4eec1650bfd317823f76a30889f912 
>   src/test/python/apache/aurora/admin/test_mesos_maintenance.py 
> 000fbe5a873669bf55a9110f1aa6f1d7c8350ef8 
>   src/test/python/apache/aurora/client/commands/BUILD 
> 6d448d7320a78bf140873b743805846c179281a7 
>   src/test/python/apache/aurora/client/commands/test_maintenance.py 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/18334/diff/
> 
> 
> Testing
> ---
> 
> ./pants ./src/test/python/apache/aurora:all
> 
> particularly:
> 
> src.test.python.apache.aurora.client.commands.maintenance 
>   .   SUCCESS
> 
> 
> Thanks,
> 
> Joe Smith
> 
>

Re: Review Request 18713: Initiate a teardown of scheduler lifecycle upon application exit.

[Maxim Khutornenko]

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



src/main/java/org/apache/aurora/scheduler/SchedulerLifecycle.java


Not sure how shutdownRegistry.addAction() fits into this specific Closure. 
It there anything forcing it here? If not shouldn't be one level up within the 
constructor body?


- Maxim Khutornenko


On March 3, 2014, 11:31 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18713/
> ---
> 
> (Updated March 3, 2014, 11:31 p.m.)
> 
> 
> Review request for Aurora, Suman Karumuri and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> AFAICT there currently isn't wiring for this, so application exit not 
> initiated by scheduler lifecycle will not gracefully tear down components 
> handled by SchedulerLifecycle.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/SchedulerLifecycle.java 
> 1cdbff62b5348db4565388651032bf88533d9d91 
>   src/test/java/org/apache/aurora/scheduler/SchedulerLifecycleTest.java 
> 1b1e948d95387fe6aab0a78c2b1b1744b990ba55 
> 
> Diff: https://reviews.apache.org/r/18713/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Review Request 18720: Refactored admin client commands to take advantage of new TaskQuery fields.

[Maxim Khutornenko]

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

Review request for Aurora, Mark Chu-Carroll and Brian Wickman.


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


Repository: aurora


Description
---

Refactored admin client commands to take advantage of new TaskQuery fields.

The jobKeys thrift field is tracked by https://reviews.apache.org/r/18655/


Diffs
-

  src/main/python/apache/aurora/client/api/__init__.py 
9d9ff668496c7369c3abcf3ab3feb9d4090de6a8 
  src/main/python/apache/aurora/client/api/sla.py 
71013cdab23f1d0990169555a60fcc370cc0b85c 
  src/main/python/apache/aurora/client/commands/admin.py 
40588e23f5082604af9f474e9cf6ab84663b8d67 
  src/test/python/apache/aurora/client/api/test_sla.py 
c5ef100d42cde586e8d183d0f338d6b72b8b5dbd 
  src/test/python/apache/aurora/client/commands/test_admin_sla.py 
6e56bc48157efb6839e79d15cb1ba8de53ae1b7b 

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


Testing
---

/build-support/jenkins/build.sh


Thanks,

Maxim Khutornenko

Re: Review Request 18655: Added JobKey set into TaskQuery.

[Bill Farner]

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

Ship it!


Ship It!

- Bill Farner


On March 4, 2014, 1:03 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18655/
> ---
> 
> (Updated March 4, 2014, 1:03 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-235
> https://issues.apache.org/jira/browse/AURORA-235
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added an optional JobKey set filter into the TaskQuery.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> 5f684bc1f19470a8df4df167aa5c15ad6b9c9f95 
>   src/main/java/org/apache/aurora/scheduler/base/Query.java 
> d6f27fd6b8029401c918f942253beb59b6a71ddf 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
> 336e91b10dc25d761dfc4389ac27d9ac324c52c0 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
> 421b33023e3b443d579685f434a4e09957b1c6e0 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> 2afbb4ce98e49332495d264218a7112bce8d450b 
>   
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java
>  095cc4a881b16ab4d9e66b1e1f907fd781f8b0cb 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 
> 0c1b144bf22f1bf400da04bd5826a04891dfada2 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 5cfa330f54c0297c84b8f41ee550a7bc6f163010 
> 
> Diff: https://reviews.apache.org/r/18655/diff/
> 
> 
> Testing
> ---
> 
> /build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 18655: Added JobKey set into TaskQuery.

[Maxim Khutornenko]

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

(Updated March 4, 2014, 1:03 a.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

CR comments.


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


Repository: aurora


Description
---

Added an optional JobKey set filter into the TaskQuery.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
5f684bc1f19470a8df4df167aa5c15ad6b9c9f95 
  src/main/java/org/apache/aurora/scheduler/base/Query.java 
d6f27fd6b8029401c918f942253beb59b6a71ddf 
  src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
336e91b10dc25d761dfc4389ac27d9ac324c52c0 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
421b33023e3b443d579685f434a4e09957b1c6e0 
  src/main/thrift/org/apache/aurora/gen/api.thrift 
2afbb4ce98e49332495d264218a7112bce8d450b 
  
src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java 
095cc4a881b16ab4d9e66b1e1f907fd781f8b0cb 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 
0c1b144bf22f1bf400da04bd5826a04891dfada2 
  src/test/resources/org/apache/aurora/gen/api.thrift.md5 
5cfa330f54c0297c84b8f41ee550a7bc6f163010 

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


Testing
---

/build-support/jenkins/build.sh


Thanks,

Maxim Khutornenko

Re: Review Request 18655: Added JobKey set into TaskQuery.

[Maxim Khutornenko]

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



src/main/java/org/apache/aurora/scheduler/base/JobKeys.java


Good point. I initially used it in Query but that changed. Done.



src/main/java/org/apache/aurora/scheduler/base/Query.java


Not afraid. Done.



src/main/java/org/apache/aurora/scheduler/base/Query.java


Thanks, I see what you meant now. Done.



src/main/java/org/apache/aurora/scheduler/base/Query.java


Reverted.



src/main/java/org/apache/aurora/scheduler/base/Query.java


Done.


- Maxim Khutornenko


On March 3, 2014, 11:26 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18655/
> ---
> 
> (Updated March 3, 2014, 11:26 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-235
> https://issues.apache.org/jira/browse/AURORA-235
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added an optional JobKey set filter into the TaskQuery.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> 5f684bc1f19470a8df4df167aa5c15ad6b9c9f95 
>   src/main/java/org/apache/aurora/scheduler/base/Query.java 
> d6f27fd6b8029401c918f942253beb59b6a71ddf 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
> 336e91b10dc25d761dfc4389ac27d9ac324c52c0 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
> 421b33023e3b443d579685f434a4e09957b1c6e0 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> f9fc6bcb7fdc2380043cf673a4002886192c20c1 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 
> 0c1b144bf22f1bf400da04bd5826a04891dfada2 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 08c4c2a4717a1c827f4cef6405d78e7db5046a00 
> 
> Diff: https://reviews.apache.org/r/18655/diff/
> 
> 
> Testing
> ---
> 
> /build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 18655: Added JobKey set into TaskQuery.

[Bill Farner]

> On March 4, 2014, 12:02 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Query.java, line 78
> > 
> >
> > Iterables.getOnlyELement is a bit of a land mine.  An unsuspecting 
> > caller can hit IllegalArgumentException or NoSuchElementException.
> > 
> > SchedulerCoreImpl.killTasks is currently victim to this.
> 
> Maxim Khutornenko wrote:
> That's the unfortunate outcome of merging the extraction side, which I 
> have commented in the killTasks. Are you proposing to take on that 
> refactoring now?
> 
> Bill Farner wrote:
> AFAICT you can do this one locally to this method.  The safest behavior 
> is to continue the semantic of "is this job scoped by job only?", except you 
> now have to add "and exactly one job".  This might warrant a method rename.  
> Internally, you should be able to get away with another check for 
> jobKey.get().size() == 1.

s/is this job/is this query/


- Bill


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


On March 3, 2014, 11:26 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18655/
> ---
> 
> (Updated March 3, 2014, 11:26 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-235
> https://issues.apache.org/jira/browse/AURORA-235
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added an optional JobKey set filter into the TaskQuery.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> 5f684bc1f19470a8df4df167aa5c15ad6b9c9f95 
>   src/main/java/org/apache/aurora/scheduler/base/Query.java 
> d6f27fd6b8029401c918f942253beb59b6a71ddf 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
> 336e91b10dc25d761dfc4389ac27d9ac324c52c0 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
> 421b33023e3b443d579685f434a4e09957b1c6e0 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> f9fc6bcb7fdc2380043cf673a4002886192c20c1 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 
> 0c1b144bf22f1bf400da04bd5826a04891dfada2 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 08c4c2a4717a1c827f4cef6405d78e7db5046a00 
> 
> Diff: https://reviews.apache.org/r/18655/diff/
> 
> 
> Testing
> ---
> 
> /build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 18655: Added JobKey set into TaskQuery.

[Bill Farner]

> On March 4, 2014, 12:02 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Query.java, line 78
> > 
> >
> > Iterables.getOnlyELement is a bit of a land mine.  An unsuspecting 
> > caller can hit IllegalArgumentException or NoSuchElementException.
> > 
> > SchedulerCoreImpl.killTasks is currently victim to this.
> 
> Maxim Khutornenko wrote:
> That's the unfortunate outcome of merging the extraction side, which I 
> have commented in the killTasks. Are you proposing to take on that 
> refactoring now?

AFAICT you can do this one locally to this method.  The safest behavior is to 
continue the semantic of "is this job scoped by job only?", except you now have 
to add "and exactly one job".  This might warrant a method rename.  Internally, 
you should be able to get away with another check for jobKey.get().size() == 1.


- Bill


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


On March 3, 2014, 11:26 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18655/
> ---
> 
> (Updated March 3, 2014, 11:26 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-235
> https://issues.apache.org/jira/browse/AURORA-235
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added an optional JobKey set filter into the TaskQuery.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> 5f684bc1f19470a8df4df167aa5c15ad6b9c9f95 
>   src/main/java/org/apache/aurora/scheduler/base/Query.java 
> d6f27fd6b8029401c918f942253beb59b6a71ddf 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
> 336e91b10dc25d761dfc4389ac27d9ac324c52c0 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
> 421b33023e3b443d579685f434a4e09957b1c6e0 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> f9fc6bcb7fdc2380043cf673a4002886192c20c1 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 
> 0c1b144bf22f1bf400da04bd5826a04891dfada2 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 08c4c2a4717a1c827f4cef6405d78e7db5046a00 
> 
> Diff: https://reviews.apache.org/r/18655/diff/
> 
> 
> Testing
> ---
> 
> /build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 18655: Added JobKey set into TaskQuery.

[Maxim Khutornenko]

> On March 4, 2014, 12:02 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Query.java, line 78
> > 
> >
> > Iterables.getOnlyELement is a bit of a land mine.  An unsuspecting 
> > caller can hit IllegalArgumentException or NoSuchElementException.
> > 
> > SchedulerCoreImpl.killTasks is currently victim to this.

That's the unfortunate outcome of merging the extraction side, which I have 
commented in the killTasks. Are you proposing to take on that refactoring now?


- Maxim


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


On March 3, 2014, 11:26 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18655/
> ---
> 
> (Updated March 3, 2014, 11:26 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-235
> https://issues.apache.org/jira/browse/AURORA-235
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added an optional JobKey set filter into the TaskQuery.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> 5f684bc1f19470a8df4df167aa5c15ad6b9c9f95 
>   src/main/java/org/apache/aurora/scheduler/base/Query.java 
> d6f27fd6b8029401c918f942253beb59b6a71ddf 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
> 336e91b10dc25d761dfc4389ac27d9ac324c52c0 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
> 421b33023e3b443d579685f434a4e09957b1c6e0 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> f9fc6bcb7fdc2380043cf673a4002886192c20c1 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 
> 0c1b144bf22f1bf400da04bd5826a04891dfada2 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 08c4c2a4717a1c827f4cef6405d78e7db5046a00 
> 
> Diff: https://reviews.apache.org/r/18655/diff/
> 
> 
> Testing
> ---
> 
> /build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 18655: Added JobKey set into TaskQuery.

[Bill Farner]

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



src/main/java/org/apache/aurora/scheduler/base/JobKeys.java


s/public/private/ (only used in this class), kill doc, and move closer to 
call site



src/main/java/org/apache/aurora/scheduler/base/Query.java


remove extra parens around query.isSetJobKeys().  Also, if you're not 
scared off by the single-letter variable name, you can reshuffle to make the 
precedence more clear:

return (q.isSetOwner() && q.getOwner().isSetRole() && 
q.isSetEnvironment() && q.isSetJobName())
|| q.isSetJobKeys();



src/main/java/org/apache/aurora/scheduler/base/Query.java


Iterables.getOnlyELement is a bit of a land mine.  An unsuspecting caller 
can hit IllegalArgumentException or NoSuchElementException.

SchedulerCoreImpl.killTasks is currently victim to this.



src/main/java/org/apache/aurora/scheduler/base/Query.java


revert



src/main/java/org/apache/aurora/scheduler/base/Query.java


"scoped to the jobs" rather than "scoped to the job keys"


- Bill Farner


On March 3, 2014, 11:26 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18655/
> ---
> 
> (Updated March 3, 2014, 11:26 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-235
> https://issues.apache.org/jira/browse/AURORA-235
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added an optional JobKey set filter into the TaskQuery.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> 5f684bc1f19470a8df4df167aa5c15ad6b9c9f95 
>   src/main/java/org/apache/aurora/scheduler/base/Query.java 
> d6f27fd6b8029401c918f942253beb59b6a71ddf 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
> 336e91b10dc25d761dfc4389ac27d9ac324c52c0 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
> 421b33023e3b443d579685f434a4e09957b1c6e0 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> f9fc6bcb7fdc2380043cf673a4002886192c20c1 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 
> 0c1b144bf22f1bf400da04bd5826a04891dfada2 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 08c4c2a4717a1c827f4cef6405d78e7db5046a00 
> 
> Diff: https://reviews.apache.org/r/18655/diff/
> 
> 
> Testing
> ---
> 
> /build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Review Request 18714: Disable log and initiate shutdown upon log storage failures.

[Bill Farner]

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

Review request for Aurora, Kevin Sweeney and Suman Karumuri.


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


Repository: aurora


Description
---

Disable log and initiate shutdown upon log storage failures.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLog.java 
071c4fcf96099819aac6276306034434e3eec879 
  src/test/java/org/apache/aurora/scheduler/log/mesos/MesosLogTest.java 
359f5d44e030a53cedd612a350b161b392d929c4 

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


Testing
---

./gradlew build


Thanks,

Bill Farner

Review Request 18713: Initiate a teardown of scheduler lifecycle upon application exit.

[Bill Farner]

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

Review request for Aurora, Suman Karumuri and Maxim Khutornenko.


Repository: aurora


Description
---

AFAICT there currently isn't wiring for this, so application exit not initiated 
by scheduler lifecycle will not gracefully tear down components handled by 
SchedulerLifecycle.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/SchedulerLifecycle.java 
1cdbff62b5348db4565388651032bf88533d9d91 
  src/test/java/org/apache/aurora/scheduler/SchedulerLifecycleTest.java 
1b1e948d95387fe6aab0a78c2b1b1744b990ba55 

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


Testing
---

./gradlew build


Thanks,

Bill Farner

Re: Review Request 18655: Added JobKey set into TaskQuery.

[Maxim Khutornenko]

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

(Updated March 3, 2014, 11:26 p.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

Addressed CR comments.


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


Repository: aurora


Description
---

Added an optional JobKey set filter into the TaskQuery.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
5f684bc1f19470a8df4df167aa5c15ad6b9c9f95 
  src/main/java/org/apache/aurora/scheduler/base/Query.java 
d6f27fd6b8029401c918f942253beb59b6a71ddf 
  src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
336e91b10dc25d761dfc4389ac27d9ac324c52c0 
  src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
421b33023e3b443d579685f434a4e09957b1c6e0 
  src/main/thrift/org/apache/aurora/gen/api.thrift 
f9fc6bcb7fdc2380043cf673a4002886192c20c1 
  src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 
0c1b144bf22f1bf400da04bd5826a04891dfada2 
  src/test/resources/org/apache/aurora/gen/api.thrift.md5 
08c4c2a4717a1c827f4cef6405d78e7db5046a00 

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


Testing
---

/build-support/jenkins/build.sh


Thanks,

Maxim Khutornenko

Re: Review Request 18580: Upgrade mesos to 0.17.0

[Brian Wickman]

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

Ship it!


Ship It!

- Brian Wickman


On Feb. 28, 2014, 11:21 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18580/
> ---
> 
> (Updated Feb. 28, 2014, 11:21 p.m.)
> 
> 
> Review request for Aurora, Bill Farner and Brian Wickman.
> 
> 
> Bugs: AURORA-240
> https://issues.apache.org/jira/browse/AURORA-240
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Upgrade mesos to 0.17.0
> 
> Moves to using non-deprecated methods and calls the new mesos-log 
> --initialize command before starting up the scheduler.
> 
> 
> Diffs
> -
> 
>   3rdparty/python/BUILD 2c7646ced42a4215267291f566726a46e41575d9 
>   build.gradle e5cd831f7e742a7911994c6cbe23122f90aae313 
>   docs/deploying-aurora-scheduler.md 1718f8923ae33e3cac6f88fa54cef582e2f2342c 
>   examples/scheduler/scheduler-local.sh 
> e1e394e5259686f30dc14856a455abf6197a26b0 
>   examples/vagrant/provision-aurora-scheduler.sh 
> 894a4d20a9084c7eb6bc3acf70d7c45d0052ca8d 
>   examples/vagrant/provision-dev-environment.sh 
> 7fc11bf523a054f6d068cf676e87bb043d854a25 
>   examples/vagrant/provision-mesos-master.sh 
> 6e8a923b829b6665751cf81353d89467fdd28da5 
>   examples/vagrant/provision-mesos-slave.sh 
> ef9180e40ac714d12d412c3e705b2e4dbad42ca2 
>   src/main/java/org/apache/aurora/scheduler/Driver.java 
> 7af8721cc3d9a743605909eba3ada66c0dca592a 
>   src/main/java/org/apache/aurora/scheduler/local/FakeDriverFactory.java 
> e5d13e0b2cbcb89cfb24d934e27515fa9a4c885e 
>   src/test/java/org/apache/aurora/scheduler/MesosSchedulerImplTest.java 
> 7a91ab8d9437ba465018fb9dc221d68174178534 
> 
> Diff: https://reviews.apache.org/r/18580/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> vagrant destroy -f
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> vagrant ssh devtools -c 'cd aurora; ./pants 
> src/test/python/apache/aurora/executor:executor-large'
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 18562: Simplfiy TOC and anchors in remaining docs, among other niceties.

[Tom Galloway]

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

Ship it!


Ship It!

- Tom Galloway


On Feb. 27, 2014, 4:06 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18562/
> ---
> 
> (Updated Feb. 27, 2014, 4:06 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Tom Galloway.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> - Simplify TOC and anchors
> - Enable syntax highlighting
> - Minor cleanups, make formatting more consistent
> 
> 
> Diffs
> -
> 
>   docs/clientcommands.md d87f6292eef31e5a919cc6dd0e4ef8455345522d 
>   docs/configurationtutorial.md 7fe8948424dbd37ae698f1f74a9c48c3c9323cc5 
>   docs/hooks.md 8bc00c8c0d672e8eb4eef4a41f7b03c27fc602f3 
>   docs/resourceisolation.md 0e3335d62af5e50bc7bbf9a5a563d9cbe2e97fc7 
>   docs/tutorial.md ec60e70d7f31581db736ffa454c901bb5f9172ea 
>   docs/userguide.md 2e5af23d3c2fba2b22d7c7dbd4e6d383f3d2bb6c 
> 
> Diff: https://reviews.apache.org/r/18562/diff/
> 
> 
> Testing
> ---
> 
> Rendered at 
> https://github.com/wfarner/incubator-aurora/blob/wfarner/doc_cleanup2/docs
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 18655: Added JobKey set into TaskQuery.

[Maxim Khutornenko]

> On March 1, 2014, 1:32 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/base/JobKeys.java, line 178
> > 
> >
> > "from" is currently intended to unambiguously give a single IJobKey 
> > instance derived from role/env/name. I am not sure exposing a Set 
> > would be a good idea. 
> >
> 
> Bill Farner wrote:
> It gets weird though if i call from() with a query that has a singleton 
> set of job keys and get back Optional.absent().

Agree. That comment is superseded by a later one. The current plan is to merge 
extraction side (from) and keep the builder side (byJob/byJobKeys) separated to 
allow proper filter construction.  


- Maxim


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


On March 1, 2014, 12:54 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18655/
> ---
> 
> (Updated March 1, 2014, 12:54 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-235
> https://issues.apache.org/jira/browse/AURORA-235
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added an optional JobKey set filter into the TaskQuery.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> 5f684bc1f19470a8df4df167aa5c15ad6b9c9f95 
>   src/main/java/org/apache/aurora/scheduler/base/Query.java 
> d6f27fd6b8029401c918f942253beb59b6a71ddf 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
> 421b33023e3b443d579685f434a4e09957b1c6e0 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> f9fc6bcb7fdc2380043cf673a4002886192c20c1 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 
> 0c1b144bf22f1bf400da04bd5826a04891dfada2 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 08c4c2a4717a1c827f4cef6405d78e7db5046a00 
> 
> Diff: https://reviews.apache.org/r/18655/diff/
> 
> 
> Testing
> ---
> 
> /build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 18655: Added JobKey set into TaskQuery.

[Bill Farner]

> On March 1, 2014, 1:32 a.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/base/JobKeys.java, line 178
> > 
> >
> > "from" is currently intended to unambiguously give a single IJobKey 
> > instance derived from role/env/name. I am not sure exposing a Set 
> > would be a good idea. 
> >

It gets weird though if i call from() with a query that has a singleton set of 
job keys and get back Optional.absent().


- Bill


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


On March 1, 2014, 12:54 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18655/
> ---
> 
> (Updated March 1, 2014, 12:54 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-235
> https://issues.apache.org/jira/browse/AURORA-235
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added an optional JobKey set filter into the TaskQuery.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> 5f684bc1f19470a8df4df167aa5c15ad6b9c9f95 
>   src/main/java/org/apache/aurora/scheduler/base/Query.java 
> d6f27fd6b8029401c918f942253beb59b6a71ddf 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
> 421b33023e3b443d579685f434a4e09957b1c6e0 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> f9fc6bcb7fdc2380043cf673a4002886192c20c1 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 
> 0c1b144bf22f1bf400da04bd5826a04891dfada2 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 08c4c2a4717a1c827f4cef6405d78e7db5046a00 
> 
> Diff: https://reviews.apache.org/r/18655/diff/
> 
> 
> Testing
> ---
> 
> /build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 18655: Added JobKey set into TaskQuery.

[Bill Farner]

> On March 3, 2014, 6:06 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Query.java, line 387
> > 
> >
> > To continue this conversation, I don't quite feel merging job 
> > (role/env/name) and jobKey extraction/building points would make much 
> > logical sense. 
> > 
> > Given the current filter implementation, the jobKeys from the set would 
> > be ORed between each other but ANDed with the job key derived from 
> > role/env/name fields. Unionizing the two would just send the wrong message 
> > as the job built out of role/env/name will not be treated the same way as 
> > the set of keys. 
> > 
> > Combining the two would make for mutually exclusive combinations. 
> > Example: byJobs(IJobKey jobKey, IJobKey... jobKeys) would have to shred the 
> > first key to role/env/name fields and send the rest to the Set 
> > (see the test case below). 
> > 
> > Given the above, I can see some benefits in merging the two on the 
> > extraction side (i.e. from(), isJobKeyScoped()) but I feel that the 
> > construction side should remain separate (byJob, byJobKeys). Thoughts?

I'll let you make the call from here.  The semantic probably isn't too 
important given that we want to get rid of the overlap in short order.


- Bill


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


On March 1, 2014, 12:54 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18655/
> ---
> 
> (Updated March 1, 2014, 12:54 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-235
> https://issues.apache.org/jira/browse/AURORA-235
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added an optional JobKey set filter into the TaskQuery.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> 5f684bc1f19470a8df4df167aa5c15ad6b9c9f95 
>   src/main/java/org/apache/aurora/scheduler/base/Query.java 
> d6f27fd6b8029401c918f942253beb59b6a71ddf 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
> 421b33023e3b443d579685f434a4e09957b1c6e0 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> f9fc6bcb7fdc2380043cf673a4002886192c20c1 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 
> 0c1b144bf22f1bf400da04bd5826a04891dfada2 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 08c4c2a4717a1c827f4cef6405d78e7db5046a00 
> 
> Diff: https://reviews.apache.org/r/18655/diff/
> 
> 
> Testing
> ---
> 
> /build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 18658: GZIP HTTP thrift API responses

[Bill Farner]

> On March 1, 2014, 5:23 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java, line 40
> > 
> >
> > I can't think of a good reason to not apply this to /, can you?  If so, 
> > the filter should probably be applied in a different module.
> 
> Kevin Sweeney wrote:
> From the docs:
> 
>  * This filter will gzip the content of a response iff: 
>  * The filter is mapped to a matching path
>  * The response status code is >=200 and <300
>  * The content length is unknown or more than the 
> minGzipSize initParameter or the minGzipSize is 0(default)
>  * The content-type is in the comma separated list of mimeTypes set 
> in the mimeTypes initParameter or
>  * if no mimeTypes are defined the content-type is not 
> "application/gzip"
>  * No content-encoding is specified by the resource
>  * 
>  
> I'm not certain that generally applies to the whole application (/health 
> for example probably shouldn't be gzipped).

That doc defines the server-dependent details needed to enable gzip.  The 
client still needs to set 'accept-encoding', which i think does make it apply 
universally.

>From org.mortbay.servlet.GzipFilter:

String ae = request.getHeader("accept-encoding");
Boolean gzip=(Boolean)request.getAttribute("GzipFilter");
if (ae != null && ae.indexOf("gzip")>=0 && 
!response.containsHeader("Content-Encoding") &&
(gzip==null || gzip.booleanValue()) && 
!"HEAD".equalsIgnoreCase(request.getMethod()))
{
  // snip
}
else
{
super.doFilter(request,response,chain);
}


- Bill


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


On March 1, 2014, 1:27 a.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18658/
> ---
> 
> (Updated March 1, 2014, 1:27 a.m.)
> 
> 
> Review request for Aurora, Suman Karumuri and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> GZIP HTTP thrift API responses
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
> 12113532eb062308af3c43458661c7b43b23237f 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js 
> 94189642a4f35aa0cad8176589cfbd84964c2e14 
> 
> Diff: https://reviews.apache.org/r/18658/diff/
> 
> 
> Testing
> ---
> 
> Verified API responses are gzipped.
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 17303: Added getJobSummary API

[Bill Farner]

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


Looking pretty good now, found a handful of places where comments from previous 
rounds weren't fixed throughout.


src/main/java/org/apache/aurora/scheduler/base/Jobs.java


s/Collection/Iterable/



src/main/java/org/apache/aurora/scheduler/base/Tasks.java


I actually find this more confusing than "roughly".  You're better off 
saying "inactive tasks are ordered before active tasks", otherwise i start 
wondering why FAILED is less active than FINISHED.



src/main/java/org/apache/aurora/scheduler/base/Tasks.java


I still think it's best to directly reference ACTIVE_STATES and 
TERMINAL_STATES here.  You noted that those are not exhaustive of all states, 
but the unrepresented states are INIT and UNKNOWN.  If consuming code reads 
tasks in those states, it's a  bug (they're currently only there assist the 
state machine).



src/main/java/org/apache/aurora/scheduler/base/Tasks.java


How about getLatestActiveTask()?  getLatestTransitionedTask() seems like it 
should be skipping the LATEST_ACTIVITY order.



src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java


Barring naming conflict, can you remove the "ByRole" qualifier from these 
method names?  It reads unnaturally to "get by role, maybe without a role", 
while "get, with optional role" is more obvious from the call site.



src/main/thrift/org/apache/aurora/gen/api.thrift


still planning to move this closer to JobSummary?



src/main/thrift/org/apache/aurora/gen/api.thrift


"optionally only those owned by a specific role"



src/test/java/org/apache/aurora/scheduler/base/JobsTest.java


2014



src/test/java/org/apache/aurora/scheduler/base/TaskTestUtil.java


2014



src/test/java/org/apache/aurora/scheduler/base/TasksTest.java


2014



src/test/java/org/apache/aurora/scheduler/base/TasksTest.java


s/final //

throughout



src/test/java/org/apache/aurora/scheduler/base/TasksTest.java


What's the expected behavior when an empty iterable is provided?



src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java


It would be really nice to see all of these assertEquals looking more like:

  assertEquals(expected, actual);

Where 'actual' is the top-level response rather than the result of peeking 
into fields.  This gives confidence that the response code and message are also 
set appropriately.


- Bill Farner


On March 1, 2014, 1:26 a.m., Suman Karumuri wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/17303/
> ---
> 
> (Updated March 1, 2014, 1:26 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-64
> https://issues.apache.org/jira/browse/AURORA-64
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added getJobSummary API so it can be used by the role and role/environment 
> page in the UI.
> Refactored code from SchedulerzRole and SchedulerzRoleTest into relevant 
> classes so it can be used by the UI and the thrift API.
> Added tests for new code.
> Moved populateJobConfig call into ReadOnlyScheduler.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/base/Jobs.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/base/Tasks.java 
> d9cb886ef333e108d5d5f86043ac80e450689894 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerzRole.java 
> 25ba7da5f8bbe5416f41bb0b14850beb84392cc7 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  7b9f185cea77825e46ecfc588c72e146cd864a32 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> 3ee24c75f961af61048a78ec6c3f244361bed5bd 
>   src/test/java/org/apache/aurora/scheduler/base/JobsTest.java PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/base/TasksTest.java PRE-CREATION 
>   src/test/java/org/apache/aurora/schedu

Re: Review Request 18562: Simplfiy TOC and anchors in remaining docs, among other niceties.

[Tom Galloway]

Ship it!

Tom


On Mon, Mar 3, 2014 at 9:52 AM, Maxim Khutornenko  wrote:

>This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18562/
>
> Ship it!
>
> I would delegate this to Tom. LGTM AFAICT.
>
>
> - Maxim Khutornenko
>
> On February 27th, 2014, 4:06 a.m. UTC, Bill Farner wrote:
>   Review request for Aurora, Maxim Khutornenko and Tom Galloway.
> By Bill Farner.
>
> *Updated Feb. 27, 2014, 4:06 a.m.*
>  *Repository: * aurora
> Description
>
> - Simplify TOC and anchors
> - Enable syntax highlighting
> - Minor cleanups, make formatting more consistent
>
>   Testing
>
> Rendered at 
> https://github.com/wfarner/incubator-aurora/blob/wfarner/doc_cleanup2/docs
>
>   Diffs
>
>- docs/clientcommands.md (d87f6292eef31e5a919cc6dd0e4ef8455345522d)
>- docs/configurationtutorial.md
>(7fe8948424dbd37ae698f1f74a9c48c3c9323cc5)
>- docs/hooks.md (8bc00c8c0d672e8eb4eef4a41f7b03c27fc602f3)
>- docs/resourceisolation.md (0e3335d62af5e50bc7bbf9a5a563d9cbe2e97fc7)
>- docs/tutorial.md (ec60e70d7f31581db736ffa454c901bb5f9172ea)
>- docs/userguide.md (2e5af23d3c2fba2b22d7c7dbd4e6d383f3d2bb6c)
>
> View Diff 
>

Re: Review Request 18704: Remove LogStream.close, which was a no-op everywhere.

[Kevin Sweeney]

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

Ship it!


Ship It!

- Kevin Sweeney


On March 3, 2014, 11:05 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18704/
> ---
> 
> (Updated March 3, 2014, 11:05 a.m.)
> 
> 
> Review request for Aurora and Kevin Sweeney.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Drive-by cleanup.  Noticed only no-ops in implementations of this method, so 
> it makes sense to remove.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/log/Log.java 
> 35403f33ce5583a01b8c85fd9968113cb6e153d1 
>   src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLog.java 
> a8e6c83e77aae894ed36e79f8cc85dcd85f75d6c 
>   src/main/java/org/apache/aurora/scheduler/log/testing/FileLog.java 
> 5b69e4e12bb3c2ea797e9b32495699c2e243df40 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogManager.java 
> b59b9d092b0101093621d809aea760dca2f71d42 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> 6cfaf39ee41862d088ab9747b7820da3f5014a0c 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogManagerTest.java 
> 040125e5f60329646b1d6356a00f5a8f1fd93b72 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> d6624f470eadaf0be658db1d87d5863ea6701d94 
> 
> Diff: https://reviews.apache.org/r/18704/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Review Request 18704: Remove LogStream.close, which was a no-op everywhere.

[Bill Farner]

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

Review request for Aurora and Kevin Sweeney.


Repository: aurora


Description
---

Drive-by cleanup.  Noticed only no-ops in implementations of this method, so it 
makes sense to remove.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/log/Log.java 
35403f33ce5583a01b8c85fd9968113cb6e153d1 
  src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLog.java 
a8e6c83e77aae894ed36e79f8cc85dcd85f75d6c 
  src/main/java/org/apache/aurora/scheduler/log/testing/FileLog.java 
5b69e4e12bb3c2ea797e9b32495699c2e243df40 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogManager.java 
b59b9d092b0101093621d809aea760dca2f71d42 
  src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
6cfaf39ee41862d088ab9747b7820da3f5014a0c 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogManagerTest.java 
040125e5f60329646b1d6356a00f5a8f1fd93b72 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
d6624f470eadaf0be658db1d87d5863ea6701d94 

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


Testing
---

./gradlew build


Thanks,

Bill Farner

Re: Review Request 18487: Refactor LogStorage to more cleanly separate replay and write-behind modes, and require explicit implementation of mutate operations.

[Bill Farner]

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

(Updated March 3, 2014, 6:29 p.m.)


Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.


Repository: aurora


Description
---

There are a few nice things falling out of this refactor:
- New methods on mutable store interfaces are not implicitly forwarded
  (ForwardingStore does not implement mutable stores).
- Write ahead/behind behavior is more obvious in LogStorage
  (i found the delegation by calling super tough to catch mistakes.)
- Callers with a handle on LogStorage don't have a means to invoke mutate calls 
outside of a transaction
  (they only get access to mutable stores in write(), which obviated 
testMutateRequiresWriteOperation)


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 
34d39f2e9064b67ca226c13bb7e330f4daa2a2aa 
  src/main/java/org/apache/aurora/scheduler/storage/TaskStore.java 
3d0ff2dfc01404c890dc17d23c3d15732915c438 
  src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
1842210c91ee3e153f5d142266bc2696960535af 
  src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
d6624f470eadaf0be658db1d87d5863ea6701d94 

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


Testing
---

./gradlew build


Thanks,

Bill Farner

Re: Review Request 18487: Refactor LogStorage to more cleanly separate replay and write-behind modes, and require explicit implementation of mutate operations.

[Bill Farner]

> On Feb. 26, 2014, 7:30 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java, line 
> > 335
> > 
> >
> > How about extracting a private transactionManager field initialized 
> > inline? Small price for a big improvement in readability.

Done.


> On Feb. 26, 2014, 7:30 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java,
> >  line 87
> > 
> >
> > Extra space.

Fixed.


> On Feb. 26, 2014, 7:30 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java,
> >  line 91
> > 
> >
> > Typo.

Fixed.


> On Feb. 26, 2014, 7:30 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java,
> >  line 158
> > 
> >
> > This seems to be a prevailing pattern: assertWriting() followed by 
> > log(). Why not retain the previous approach of asserting inside of a log()? 
> > You could still assert earlier when needed with the assertion in log() 
> > being a catch-all case.

Thanks for catching, this was a regression due to a sequence of dependent 
reviews.


> On Feb. 26, 2014, 7:30 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java,
> >  line 280
> > 
> >
> > Mind converting this comment into a message arg?

Done.  Also added a TODO in TaskStore to give some insight into where i'd like 
to see these methods go.


- Bill


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


On Feb. 26, 2014, 12:12 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18487/
> ---
> 
> (Updated Feb. 26, 2014, 12:12 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> There are a few nice things falling out of this refactor:
> - New methods on mutable store interfaces are not implicitly forwarded
>   (ForwardingStore does not implement mutable stores).
> - Write ahead/behind behavior is more obvious in LogStorage
>   (i found the delegation by calling super tough to catch mistakes.)
> - Callers with a handle on LogStorage don't have a means to invoke mutate 
> calls outside of a transaction
>   (they only get access to mutable stores in write(), which obviated 
> testMutateRequiresWriteOperation)
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 
> 34d39f2e9064b67ca226c13bb7e330f4daa2a2aa 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> 1842210c91ee3e153f5d142266bc2696960535af 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 
> d6624f470eadaf0be658db1d87d5863ea6701d94 
> 
> Diff: https://reviews.apache.org/r/18487/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 18655: Added JobKey set into TaskQuery.

[Maxim Khutornenko]

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



src/main/java/org/apache/aurora/scheduler/base/Query.java


To continue this conversation, I don't quite feel merging job 
(role/env/name) and jobKey extraction/building points would make much logical 
sense. 

Given the current filter implementation, the jobKeys from the set would be 
ORed between each other but ANDed with the job key derived from role/env/name 
fields. Unionizing the two would just send the wrong message as the job built 
out of role/env/name will not be treated the same way as the set of keys. 

Combining the two would make for mutually exclusive combinations. Example: 
byJobs(IJobKey jobKey, IJobKey... jobKeys) would have to shred the first key to 
role/env/name fields and send the rest to the Set (see the test case 
below). 

Given the above, I can see some benefits in merging the two on the 
extraction side (i.e. from(), isJobKeyScoped()) but I feel that the 
construction side should remain separate (byJob, byJobKeys). Thoughts?


- Maxim Khutornenko


On March 1, 2014, 12:54 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18655/
> ---
> 
> (Updated March 1, 2014, 12:54 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-235
> https://issues.apache.org/jira/browse/AURORA-235
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Added an optional JobKey set filter into the TaskQuery.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 
> 5f684bc1f19470a8df4df167aa5c15ad6b9c9f95 
>   src/main/java/org/apache/aurora/scheduler/base/Query.java 
> d6f27fd6b8029401c918f942253beb59b6a71ddf 
>   src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 
> 421b33023e3b443d579685f434a4e09957b1c6e0 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> f9fc6bcb7fdc2380043cf673a4002886192c20c1 
>   src/test/java/org/apache/aurora/scheduler/storage/mem/MemTaskStoreTest.java 
> 0c1b144bf22f1bf400da04bd5826a04891dfada2 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 08c4c2a4717a1c827f4cef6405d78e7db5046a00 
> 
> Diff: https://reviews.apache.org/r/18655/diff/
> 
> 
> Testing
> ---
> 
> /build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 18658: GZIP HTTP thrift API responses

[Kevin Sweeney]

> On March 1, 2014, 9:23 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java, line 40
> > 
> >
> > I can't think of a good reason to not apply this to /, can you?  If so, 
> > the filter should probably be applied in a different module.

>From the docs:

 * This filter will gzip the content of a response iff: 
 * The filter is mapped to a matching path
 * The response status code is >=200 and <300
 * The content length is unknown or more than the minGzipSize 
initParameter or the minGzipSize is 0(default)
 * The content-type is in the comma separated list of mimeTypes set in the 
mimeTypes initParameter or
 * if no mimeTypes are defined the content-type is not "application/gzip"
 * No content-encoding is specified by the resource
 * 
 
I'm not certain that generally applies to the whole application (/health for 
example probably shouldn't be gzipped).


- Kevin


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


On Feb. 28, 2014, 5:27 p.m., Kevin Sweeney wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18658/
> ---
> 
> (Updated Feb. 28, 2014, 5:27 p.m.)
> 
> 
> Review request for Aurora, Suman Karumuri and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> GZIP HTTP thrift API responses
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/thrift/ThriftModule.java 
> 12113532eb062308af3c43458661c7b43b23237f 
>   src/main/resources/org/apache/aurora/scheduler/http/ui/js/services.js 
> 94189642a4f35aa0cad8176589cfbd84964c2e14 
> 
> Diff: https://reviews.apache.org/r/18658/diff/
> 
> 
> Testing
> ---
> 
> Verified API responses are gzipped.
> 
> 
> Thanks,
> 
> Kevin Sweeney
> 
>

Re: Review Request 18562: Simplfiy TOC and anchors in remaining docs, among other niceties.

[Maxim Khutornenko]

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

Ship it!


I would delegate this to Tom. LGTM AFAICT.

- Maxim Khutornenko


On Feb. 27, 2014, 4:06 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18562/
> ---
> 
> (Updated Feb. 27, 2014, 4:06 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Tom Galloway.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> - Simplify TOC and anchors
> - Enable syntax highlighting
> - Minor cleanups, make formatting more consistent
> 
> 
> Diffs
> -
> 
>   docs/clientcommands.md d87f6292eef31e5a919cc6dd0e4ef8455345522d 
>   docs/configurationtutorial.md 7fe8948424dbd37ae698f1f74a9c48c3c9323cc5 
>   docs/hooks.md 8bc00c8c0d672e8eb4eef4a41f7b03c27fc602f3 
>   docs/resourceisolation.md 0e3335d62af5e50bc7bbf9a5a563d9cbe2e97fc7 
>   docs/tutorial.md ec60e70d7f31581db736ffa454c901bb5f9172ea 
>   docs/userguide.md 2e5af23d3c2fba2b22d7c7dbd4e6d383f3d2bb6c 
> 
> Diff: https://reviews.apache.org/r/18562/diff/
> 
> 
> Testing
> ---
> 
> Rendered at 
> https://github.com/wfarner/incubator-aurora/blob/wfarner/doc_cleanup2/docs
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 18562: Simplfiy TOC and anchors in remaining docs, among other niceties.

[Bill Farner]

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


Ping?

- Bill Farner


On Feb. 27, 2014, 4:06 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18562/
> ---
> 
> (Updated Feb. 27, 2014, 4:06 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Tom Galloway.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> - Simplify TOC and anchors
> - Enable syntax highlighting
> - Minor cleanups, make formatting more consistent
> 
> 
> Diffs
> -
> 
>   docs/clientcommands.md d87f6292eef31e5a919cc6dd0e4ef8455345522d 
>   docs/configurationtutorial.md 7fe8948424dbd37ae698f1f74a9c48c3c9323cc5 
>   docs/hooks.md 8bc00c8c0d672e8eb4eef4a41f7b03c27fc602f3 
>   docs/resourceisolation.md 0e3335d62af5e50bc7bbf9a5a563d9cbe2e97fc7 
>   docs/tutorial.md ec60e70d7f31581db736ffa454c901bb5f9172ea 
>   docs/userguide.md 2e5af23d3c2fba2b22d7c7dbd4e6d383f3d2bb6c 
> 
> Diff: https://reviews.apache.org/r/18562/diff/
> 
> 
> Testing
> ---
> 
> Rendered at 
> https://github.com/wfarner/incubator-aurora/blob/wfarner/doc_cleanup2/docs
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 18484: Count on task timeouts and task pruning to be idempotent, simplifying handling code.

[Bill Farner]

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


Ping?  Suman — waiting on a review from you.

- Bill Farner


On Feb. 25, 2014, 10:25 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18484/
> ---
> 
> (Updated Feb. 25, 2014, 10:25 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Suman Karumuri.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> These async operations already took provisions to be idempotent, but still 
> had additional code to keep track of and clean up work made irrelevant.
> 
> In this change, we explicitly lean on the fact that the operations are 
> idempotent, and allow things like a task timeout to fire but no-op for a task 
> that successfully made it out of a transient state.
> 
> More details on specific changes:
> - HistoryPruner used to maintain a mapping from job key to task ID.  
> Addressed TODO to use storage instead.
> - Removed collection of outstanding tasks in transient states.  This was 
> useful when vetting the feature, but is no longer used in practice.
> - Attempted to make HistoryPrunerTest more readable.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java 
> c80c000ddb5627e9d753bd59e77231e3050470e5 
>   src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 
> 82b483b019639e6d1013f834ea047827cb171fa6 
>   src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 
> 811f68c792ce8eb3f687f249307612c90091ad59 
>   src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java 
> 71d6a9ed943306386dfe88074ee07f69f6ca15d1 
> 
> Diff: https://reviews.apache.org/r/18484/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build
> 
> 
> Thanks,
> 
> Bill Farner
> 
>