aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maxim Khutornenko" <ma...@apache.org>
Subject Re: Review Request 23254: Refactoring SchedulerCore (killTasks)
Date Mon, 21 Jul 2014 23:23:20 GMT


> On July 21, 2014, 10:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
line 721
> > <https://reviews.apache.org/r/23254/diff/1/?file=623290#file623290line721>
> >
> >     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
> > <https://reviews.apache.org/r/23254/diff/1/?file=623294#file623294line1>
> >
> >     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
> > <https://reviews.apache.org/r/23254/diff/1/?file=623295#file623295line431>
> >
> >     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
> > <https://reviews.apache.org/r/23254/diff/1/?file=623295#file623295line509>
> >
> >     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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message