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

2014-03-03 Thread 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
 




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

2014-03-03 Thread 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 18562: Simplfiy TOC and anchors in remaining docs, among other niceties.

2014-03-03 Thread 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 18658: GZIP HTTP thrift API responses

2014-03-03 Thread 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
  https://reviews.apache.org/r/18658/diff/2/?file=507574#file507574line40
 
  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: ul
 * liThe filter is mapped to a matching path/li
 * liThe response status code is =200 and 300
 * liThe content length is unknown or more than the codeminGzipSize/code 
initParameter or the minGzipSize is 0(default)/li
 * liThe content-type is in the comma separated list of mimeTypes set in the 
codemimeTypes/code initParameter or
 * if no mimeTypes are defined the content-type is not application/gzip/li
 * liNo content-encoding is specified by the resource/li
 * /ul
 
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 18655: Added JobKey set into TaskQuery.

2014-03-03 Thread 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
https://reviews.apache.org/r/18655/#comment66778

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 SetIJobKey (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 18487: Refactor LogStorage to more cleanly separate replay and write-behind modes, and require explicit implementation of mutate operations.

2014-03-03 Thread 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
  https://reviews.apache.org/r/18487/diff/2/?file=503866#file503866line335
 
  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
  https://reviews.apache.org/r/18487/diff/2/?file=503867#file503867line87
 
  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
  https://reviews.apache.org/r/18487/diff/2/?file=503867#file503867line91
 
  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
  https://reviews.apache.org/r/18487/diff/2/?file=503867#file503867line158
 
  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
  https://reviews.apache.org/r/18487/diff/2/?file=503867#file503867line280
 
  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 18487: Refactor LogStorage to more cleanly separate replay and write-behind modes, and require explicit implementation of mutate operations.

2014-03-03 Thread 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



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

2014-03-03 Thread 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 18704: Remove LogStream.close, which was a no-op everywhere.

2014-03-03 Thread 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
 




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

2014-03-03 Thread Tom Galloway
Ship it!

Tom


On Mon, Mar 3, 2014 at 9:52 AM, Maxim Khutornenko ma...@apache.org 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 https://reviews.apache.org/r/18562/diff/



Re: Review Request 17303: Added getJobSummary API

2014-03-03 Thread 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
https://reviews.apache.org/r/17303/#comment66813

s/Collection/Iterable/



src/main/java/org/apache/aurora/scheduler/base/Tasks.java
https://reviews.apache.org/r/17303/#comment66817

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
https://reviews.apache.org/r/17303/#comment66814

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
https://reviews.apache.org/r/17303/#comment66822

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



src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
https://reviews.apache.org/r/17303/#comment66824

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
https://reviews.apache.org/r/17303/#comment66825

still planning to move this closer to JobSummary?



src/main/thrift/org/apache/aurora/gen/api.thrift
https://reviews.apache.org/r/17303/#comment66826

optionally only those owned by a specific role



src/test/java/org/apache/aurora/scheduler/base/JobsTest.java
https://reviews.apache.org/r/17303/#comment66827

2014



src/test/java/org/apache/aurora/scheduler/base/TaskTestUtil.java
https://reviews.apache.org/r/17303/#comment66828

2014



src/test/java/org/apache/aurora/scheduler/base/TasksTest.java
https://reviews.apache.org/r/17303/#comment66830

2014



src/test/java/org/apache/aurora/scheduler/base/TasksTest.java
https://reviews.apache.org/r/17303/#comment66831

s/final //

throughout



src/test/java/org/apache/aurora/scheduler/base/TasksTest.java
https://reviews.apache.org/r/17303/#comment66833

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



src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
https://reviews.apache.org/r/17303/#comment66834

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/scheduler/http/SchedulerzRoleTest.java 
 912be189583419e7201e45650d18cd24a6a5a35b 
   
 

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

2014-03-03 Thread 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
  https://reviews.apache.org/r/18655/diff/1/?file=507500#file507500line387
 
  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 SetIJobKey 
  (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 18655: Added JobKey set into TaskQuery.

2014-03-03 Thread 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
  https://reviews.apache.org/r/18655/diff/1/?file=507499#file507499line178
 
  from is currently intended to unambiguously give a single IJobKey 
  instance derived from role/env/name. I am not sure exposing a SetIJobKey 
  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 18562: Simplfiy TOC and anchors in remaining docs, among other niceties.

2014-03-03 Thread 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 18580: Upgrade mesos to 0.17.0

2014-03-03 Thread 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
 




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

2014-03-03 Thread 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



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

2014-03-03 Thread 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
  https://reviews.apache.org/r/18655/diff/2/?file=509070#file509070line78
 
  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.

2014-03-03 Thread 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.

2014-03-03 Thread 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
https://reviews.apache.org/r/18655/#comment66910

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



src/main/java/org/apache/aurora/scheduler/base/Query.java
https://reviews.apache.org/r/18655/#comment66917

Not afraid. Done.



src/main/java/org/apache/aurora/scheduler/base/Query.java
https://reviews.apache.org/r/18655/#comment66920

Thanks, I see what you meant now. Done.



src/main/java/org/apache/aurora/scheduler/base/Query.java
https://reviews.apache.org/r/18655/#comment66921

Reverted.



src/main/java/org/apache/aurora/scheduler/base/Query.java
https://reviews.apache.org/r/18655/#comment66922

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.

2014-03-03 Thread 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 18713: Initiate a teardown of scheduler lifecycle upon application exit.

2014-03-03 Thread 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
https://reviews.apache.org/r/18713/#comment66968

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
 




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

2014-03-03 Thread Joe Smith


 On Feb. 24, 2014, 6:36 p.m., Kevin Sweeney wrote:
  src/main/python/apache/aurora/admin/mesos_maintenance.py, line 73
  https://reviews.apache.org/r/18334/diff/3/?file=502368#file502368line73
 
  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
  https://reviews.apache.org/r/18334/diff/3/?file=502372#file502372line53
 
  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
  https://reviews.apache.org/r/18334/diff/3/?file=502376#file502376line43
 
  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
  https://reviews.apache.org/r/18334/diff/3/?file=502376#file502376line88
 
  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
  https://reviews.apache.org/r/18334/diff/3/?file=502376#file502376line103
 
  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
  https://reviews.apache.org/r/18334/diff/3/?file=502376#file502376line129
 
  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
  https://reviews.apache.org/r/18334/diff/3/?file=502376#file502376line136
 
  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 18334: Move and unit test Maintenance module and commands

2014-03-03 Thread 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