Review Request 23254: Refactoring SchedulerCore (killTasks)

2014-07-02 Thread Maxim Khutornenko 

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

Review request for Aurora, Kevin Sweeney and Bill Farner.


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


Repository: aurora


Description
---

Splitting the earlier RB for easier reviewing. 

Addressing killTasks refactoring. Also moving some useful tests out of 
BaseSchedulerCoreImplTest.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
27599f75603542069084631baf9195b8ad75e902 
  src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java 
628464de0032db4eb5884e3b74346b2390408993 
  src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
b6d06f39ac39570eac4495d97d3adaabe8357bf7 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
2549dd33d08dfc6058d985127a3f0c1f3984eaa7 
  
src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
 2298971ffac45a284f9130e2122aeea8b39dc852 
  
src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java 
d2bd65eef5a8ceea92dd62044e5e2c621c4a4f3e 
  src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
94eb0a22037187625636b24707d4ac6741b55f22 
  src/test/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java 
PRE-CREATION 
  
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 2cffa74ba36e2afda3340658d6b1afd6cb50cf2c 

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


Testing
---

gradle -Pq clean build


Thanks,

Maxim Khutornenko

Re: Review Request 23254: Refactoring SchedulerCore (killTasks)

2014-07-02 Thread Maxim Khutornenko 

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

(Updated July 3, 2014, 1:48 a.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


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


Repository: aurora


Description
---

Splitting the earlier RB for easier reviewing. 

Addressing killTasks refactoring. Also moving some useful tests out of 
BaseSchedulerCoreImplTest.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
27599f75603542069084631baf9195b8ad75e902 
  src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java 
628464de0032db4eb5884e3b74346b2390408993 
  src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
b6d06f39ac39570eac4495d97d3adaabe8357bf7 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
2549dd33d08dfc6058d985127a3f0c1f3984eaa7 
  
src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
 2298971ffac45a284f9130e2122aeea8b39dc852 
  
src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java 
d2bd65eef5a8ceea92dd62044e5e2c621c4a4f3e 
  src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
94eb0a22037187625636b24707d4ac6741b55f22 
  src/test/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java 
PRE-CREATION 
  
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 2cffa74ba36e2afda3340658d6b1afd6cb50cf2c 

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


Testing
---

gradle -Pq clean build


Thanks,

Maxim Khutornenko

Re: Review Request 23254: Refactoring SchedulerCore (killTasks)

2014-07-21 Thread Maxim Khutornenko 

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


Ping.

- Maxim Khutornenko


On July 3, 2014, 1:48 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23254/
> ---
> 
> (Updated July 3, 2014, 1:48 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-94
> https://issues.apache.org/jira/browse/AURORA-94
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Splitting the earlier RB for easier reviewing. 
> 
> Addressing killTasks refactoring. Also moving some useful tests out of 
> BaseSchedulerCoreImplTest.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
> 27599f75603542069084631baf9195b8ad75e902 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java 
> 628464de0032db4eb5884e3b74346b2390408993 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
> b6d06f39ac39570eac4495d97d3adaabe8357bf7 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  2549dd33d08dfc6058d985127a3f0c1f3984eaa7 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  2298971ffac45a284f9130e2122aeea8b39dc852 
>   
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java
>  d2bd65eef5a8ceea92dd62044e5e2c621c4a4f3e 
>   src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
> 94eb0a22037187625636b24707d4ac6741b55f22 
>   src/test/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  2cffa74ba36e2afda3340658d6b1afd6cb50cf2c 
> 
> Diff: https://reviews.apache.org/r/23254/diff/
> 
> 
> Testing
> ---
> 
> gradle -Pq clean build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 23254: Refactoring SchedulerCore (killTasks)

2014-07-21 Thread Bill Farner 

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



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


We should avoid taking advantage of passing nulls where possible, and in 
fact i suggest changing addMessage to MorePreconditions.checkNotBlank on that.  
However, you'll need to take care in cases like addMessage(x, x, 
exception.getMessage()).  For those, it makes sense to add an overload that 
accepts an Exception instead of String.



src/test/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java


Is this file relevant to the change?



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


There's a subtle detail in this test case, that active() is automatically 
added to the task query.  Can you break that out into an independent test case?



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


Use a constant for string that is otherwise magic.


- Bill Farner


On July 3, 2014, 1:48 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23254/
> ---
> 
> (Updated July 3, 2014, 1:48 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-94
> https://issues.apache.org/jira/browse/AURORA-94
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Splitting the earlier RB for easier reviewing. 
> 
> Addressing killTasks refactoring. Also moving some useful tests out of 
> BaseSchedulerCoreImplTest.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
> 27599f75603542069084631baf9195b8ad75e902 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java 
> 628464de0032db4eb5884e3b74346b2390408993 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
> b6d06f39ac39570eac4495d97d3adaabe8357bf7 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  2549dd33d08dfc6058d985127a3f0c1f3984eaa7 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  2298971ffac45a284f9130e2122aeea8b39dc852 
>   
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java
>  d2bd65eef5a8ceea92dd62044e5e2c621c4a4f3e 
>   src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
> 94eb0a22037187625636b24707d4ac6741b55f22 
>   src/test/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  2cffa74ba36e2afda3340658d6b1afd6cb50cf2c 
> 
> Diff: https://reviews.apache.org/r/23254/diff/
> 
> 
> Testing
> ---
> 
> gradle -Pq clean build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 23254: Refactoring SchedulerCore (killTasks)

2014-07-21 Thread Maxim Khutornenko 


> On July 21, 2014, 10:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  line 721
> > 
> >
> > We should avoid taking advantage of passing nulls where possible, and 
> > in fact i suggest changing addMessage to MorePreconditions.checkNotBlank on 
> > that.  However, you'll need to take care in cases like addMessage(x, x, 
> > exception.getMessage()).  For those, it makes sense to add an overload that 
> > accepts an Exception instead of String.

Makes sense. Done.


> On July 21, 2014, 10:16 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java, 
> > line 1
> > 
> >
> > Is this file relevant to the change?

This is related to the "Also moving some useful tests out of 
BaseSchedulerCoreImplTest." part of the changelist. Trying to balance the diff 
size.


> On July 21, 2014, 10:16 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java,
> >  line 431
> > 
> >
> > There's a subtle detail in this test case, that active() is 
> > automatically added to the task query.  Can you break that out into an 
> > independent test case?

Sure, done.


> On July 21, 2014, 10:16 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java,
> >  line 509
> > 
> >
> > Use a constant for string that is otherwise magic.

Done.


- Maxim


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


On July 3, 2014, 1:48 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23254/
> ---
> 
> (Updated July 3, 2014, 1:48 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-94
> https://issues.apache.org/jira/browse/AURORA-94
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Splitting the earlier RB for easier reviewing. 
> 
> Addressing killTasks refactoring. Also moving some useful tests out of 
> BaseSchedulerCoreImplTest.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
> 27599f75603542069084631baf9195b8ad75e902 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java 
> 628464de0032db4eb5884e3b74346b2390408993 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
> b6d06f39ac39570eac4495d97d3adaabe8357bf7 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  2549dd33d08dfc6058d985127a3f0c1f3984eaa7 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  2298971ffac45a284f9130e2122aeea8b39dc852 
>   
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java
>  d2bd65eef5a8ceea92dd62044e5e2c621c4a4f3e 
>   src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
> 94eb0a22037187625636b24707d4ac6741b55f22 
>   src/test/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  2cffa74ba36e2afda3340658d6b1afd6cb50cf2c 
> 
> Diff: https://reviews.apache.org/r/23254/diff/
> 
> 
> Testing
> ---
> 
> gradle -Pq clean build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 23254: Refactoring SchedulerCore (killTasks)

2014-07-21 Thread Maxim Khutornenko 

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

(Updated July 21, 2014, 11:23 p.m.)


Review request for Aurora, Kevin Sweeney and Bill Farner.


Changes
---

CR comments.


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


Repository: aurora


Description
---

Splitting the earlier RB for easier reviewing. 

Addressing killTasks refactoring. Also moving some useful tests out of 
BaseSchedulerCoreImplTest.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
27599f75603542069084631baf9195b8ad75e902 
  src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java 
628464de0032db4eb5884e3b74346b2390408993 
  src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
b6d06f39ac39570eac4495d97d3adaabe8357bf7 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
95ee7b784889d66cae9b714bd88c27a136a54b3a 
  src/main/java/org/apache/aurora/scheduler/thrift/Util.java 
4ebb608d28b667bf47573524aa8878ac3b21ecfe 
  src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 
8692051fae08a59e4b70b330eb6723f72ff04d43 
  
src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
 2298971ffac45a284f9130e2122aeea8b39dc852 
  
src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java 
d2bd65eef5a8ceea92dd62044e5e2c621c4a4f3e 
  src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
94eb0a22037187625636b24707d4ac6741b55f22 
  src/test/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java 
PRE-CREATION 
  
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 b28761d5bbfdd2fea4714c79b47be5f5eb3a5280 

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


Testing
---

gradle -Pq clean build


Thanks,

Maxim Khutornenko

Re: Review Request 23254: Refactoring SchedulerCore (killTasks)

2014-07-21 Thread Bill Farner 

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

Ship it!


Ship It!

- Bill Farner


On July 21, 2014, 11:23 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23254/
> ---
> 
> (Updated July 21, 2014, 11:23 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-94
> https://issues.apache.org/jira/browse/AURORA-94
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Splitting the earlier RB for easier reviewing. 
> 
> Addressing killTasks refactoring. Also moving some useful tests out of 
> BaseSchedulerCoreImplTest.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
> 27599f75603542069084631baf9195b8ad75e902 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java 
> 628464de0032db4eb5884e3b74346b2390408993 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
> b6d06f39ac39570eac4495d97d3adaabe8357bf7 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  95ee7b784889d66cae9b714bd88c27a136a54b3a 
>   src/main/java/org/apache/aurora/scheduler/thrift/Util.java 
> 4ebb608d28b667bf47573524aa8878ac3b21ecfe 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 
> 8692051fae08a59e4b70b330eb6723f72ff04d43 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  2298971ffac45a284f9130e2122aeea8b39dc852 
>   
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java
>  d2bd65eef5a8ceea92dd62044e5e2c621c4a4f3e 
>   src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
> 94eb0a22037187625636b24707d4ac6741b55f22 
>   src/test/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  b28761d5bbfdd2fea4714c79b47be5f5eb3a5280 
> 
> Diff: https://reviews.apache.org/r/23254/diff/
> 
> 
> Testing
> ---
> 
> gradle -Pq clean build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 23254: Refactoring SchedulerCore (killTasks)

2014-07-22 Thread Kevin Sweeney 

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

Ship it!


Ship It!

- Kevin Sweeney


On July 21, 2014, 4:23 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23254/
> ---
> 
> (Updated July 21, 2014, 4:23 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Bill Farner.
> 
> 
> Bugs: AURORA-94
> https://issues.apache.org/jira/browse/AURORA-94
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Splitting the earlier RB for easier reviewing. 
> 
> Addressing killTasks refactoring. Also moving some useful tests out of 
> BaseSchedulerCoreImplTest.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/ServletModule.java 
> 27599f75603542069084631baf9195b8ad75e902 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java 
> 628464de0032db4eb5884e3b74346b2390408993 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
> b6d06f39ac39570eac4495d97d3adaabe8357bf7 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  95ee7b784889d66cae9b714bd88c27a136a54b3a 
>   src/main/java/org/apache/aurora/scheduler/thrift/Util.java 
> 4ebb608d28b667bf47573524aa8878ac3b21ecfe 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 
> 8692051fae08a59e4b70b330eb6723f72ff04d43 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  2298971ffac45a284f9130e2122aeea8b39dc852 
>   
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java
>  d2bd65eef5a8ceea92dd62044e5e2c621c4a4f3e 
>   src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
> 94eb0a22037187625636b24707d4ac6741b55f22 
>   src/test/java/org/apache/aurora/scheduler/storage/StorageBackfillTest.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  b28761d5bbfdd2fea4714c79b47be5f5eb3a5280 
> 
> Diff: https://reviews.apache.org/r/23254/diff/
> 
> 
> Testing
> ---
> 
> gradle -Pq clean build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>