> On Jan. 26, 2016, 12:08 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  line 759
> > <https://reviews.apache.org/r/42759/diff/1/?file=1220607#file1220607line759>
> >
> >     `int` cannot be `null`, so `@Nullable` is out of place here

Doh, got too excited copy-pasting.


> On Jan. 26, 2016, 12:08 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  line 785
> > <https://reviews.apache.org/r/42759/diff/1/?file=1220607#file1220607line785>
> >
> >     Return an error if the `AddInstancesConfig` field is non-null.

That's already taken care of by the IF check above.


> On Jan. 26, 2016, 12:08 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  line 800
> > <https://reviews.apache.org/r/42759/diff/1/?file=1220607#file1220607line800>
> >
> >     Use `Range.openClosed` and you don't need the +1 on either side.

Good idea! Done.


> On Jan. 26, 2016, 12:08 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  line 805
> > <https://reviews.apache.org/r/42759/diff/1/?file=1220607#file1220607line805>
> >
> >     Return an error if the `InstanceKey` field is non-null.

I don't think there is much value in this check given that shiro auth would 
throw if both are non-null anyway. For those cases when scheduler is used in 
unsecure mode, consumers should be clearly familar with the new API behavior or 
they won't see the effect of InstanceKey being passed.


> On Jan. 26, 2016, 12:08 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java,
> >  line 84
> > <https://reviews.apache.org/r/42759/diff/1/?file=1220608#file1220608line84>
> >
> >     s/@Nullable //

Done.


> On Jan. 26, 2016, 12:08 a.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java,
> >  line 1361
> > <https://reviews.apache.org/r/42759/diff/1/?file=1220609#file1220609line1361>
> >
> >     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?

SGTM. Converted all applicable tests to new behavior.


> On Jan. 26, 2016, 12:08 a.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java,
> >  line 1403
> > <https://reviews.apache.org/r/42759/diff/1/?file=1220609#file1220609line1403>
> >
> >     `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).

INVALID would not work here as we mostly do high level checks for valid 
identifiers. Taken the Fixtures route to define a const INVALID_TASK_CONFIG.


- Maxim


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


On Jan. 25, 2016, 11: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, 11: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