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




src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
(line 759)
<https://reviews.apache.org/r/42759/#comment177220>

    `int` cannot be `null`, so `@Nullable` is out of place here



src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
(line 781)
<https://reviews.apache.org/r/42759/#comment177223>

    Return an error if the `AddInstancesConfig` field is non-null.



src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
(line 796)
<https://reviews.apache.org/r/42759/#comment177225>

    Use `Range.openClosed` and you don't need the +1 on either side.



src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
(line 801)
<https://reviews.apache.org/r/42759/#comment177222>

    Return an error if the `InstanceKey` field is non-null.



src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java 
(line 84)
<https://reviews.apache.org/r/42759/#comment177227>

    s/@Nullable //



src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 (line 1361)
<https://reviews.apache.org/r/42759/#comment177221>

    I was thinking about a strategy for things like this, since i have found 
some of our compatibility modes very difficult to undo (moving to 
TaskConfig.job is actively in flight now and proving quite time-consuming).  
How would you feel about handling these bridges with a compatibility function, 
or in this case 2 functions that capture the way the field or function is being 
used.
    
    In this case, introduce a helper function:
    
    ```
    static Response deprecatedAddInstances(SchedulerThriftInterface thrift, 
$OLD_ARGS) {
    
    }
    
    static newAddInstances(SchedulerThriftInterface thrift, $NEW_ARGS) {
    
    }
    ```
    
    This makes it very obvious what mode each caller is operating in, and makes 
for clean code removal when we end a deprecation cycle.
    
    What do you think?



src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 (line 1403)
<https://reviews.apache.org/r/42759/#comment177228>

    `setTier(",")` seemed out of place at first.  Would a blatantly obvious 
value like `"INVALID"` accomplish the same goal?
    
    Or perhaps this class would benefit from a canonical invalid configuration 
field that is used in all such places (provided we're not checking varied ways 
a task is invalid, of course).


- Bill Farner


On Jan. 25, 2016, 3:34 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42759/
> -----------------------------------------------------------
> 
> (Updated Jan. 25, 2016, 3:34 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Bugs: AURORA-1258
>     https://issues.apache.org/jira/browse/AURORA-1258
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> The addInstances() RPC is getting a new life acting as `scaleOut` now.
> 
> 
> Diffs
> -----
> 
>   NEWS f2798f6a2d6841606e99a93677067e58dc5cf5cc 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> a93df2165c208c4963975aeb4f174df602baa476 
>   src/main/java/org/apache/aurora/scheduler/base/Query.java 
> 7bf0afb5c4d53942cb11b7e405196c94590bd75c 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroAuthorizingParamInterceptor.java
>  21e565ebe64971359c81426709b47c7c9cf3900c 
>   src/main/java/org/apache/aurora/scheduler/state/LockManagerImpl.java 
> 59c9786e07fbaf167aefa4e5004d7b2abe6bda57 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  662cae179142b34896b11275bd3b425903f22e3e 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  c374343a44bd5cc2e74c96b5cdf3760d0ff3278a 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  b9ce2ddba051ab2baed59108d70537d84a031934 
> 
> Diff: https://reviews.apache.org/r/42759/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to