> On Sept. 7, 2016, 9:45 p.m., Santhosh Kumar Shanmugham wrote:
> > api/src/main/thrift/org/apache/aurora/gen/api.thrift, lines 1209-1214
> > <https://reviews.apache.org/r/51662/diff/4/?file=1493667#file1493667line1209>
> >
> >     Did we consider combining the "explicit" and "implicit" reconciliation 
> > RPCs into a single "reconcile" RPC with an optional batchSize argument, so 
> > that it maps to the mesos master API.
> >     
> >     http://mesos.apache.org/documentation/latest/reconciliation/
> 
> Karthik Anantha Padmanabhan wrote:
>     We have more than one option on the scheduler for the explicit reconcile. 
> If at all we want to expose those to the admin CLI in the future it may be 
> better to have them as two separate API's.
> 
> Santhosh Kumar Shanmugham wrote:
>     "We have more than one option on the scheduler for the explicit 
> reconcile." - can you given an example of what you mean by this.
> 
> Karthik Anantha Padmanabhan wrote:
>     There is another releavant parameter called `explicitBatchDelaySeconds` 
> which dictates how long we wait in between batches before we call reconcile 
> on mesos. 
>     
>     But it is possible that we may add another option in the future ? In that 
> case wouldn't it be better to have two separate API's ?
> 
> Karthik Anantha Padmanabhan wrote:
>     Hmm actually if we are passing in a struct of `ExplicitReconcileSettings` 
> , then adding options will be easier. So I can check `isNull` on the settings 
> object on the scheduler to identify an implicit reconcile. 
>     
>     I can go either way. I just don't know if there is a strong reason to 
> make it a single API as opposed to two.

Please, don't save a few lines by merging the RPCs or forking off of the 
presence of the `batchSize`. Explicit and implicit reconciliations have very 
different scopes and cover different failure scenarios. I'd advocate in favor 
of Karthik's original proposal with separate call paths (RPCs and client 
commands) that clearly state their purpose in the admin API.


> On Sept. 7, 2016, 9:45 p.m., Santhosh Kumar Shanmugham wrote:
> > src/main/python/apache/aurora/admin/admin.py, line 342
> > <https://reviews.apache.org/r/51662/diff/4/?file=1493670#file1493670line342>
> >
> >     'default=0' here and the usage doc reads default as 1000.
> 
> Karthik Anantha Padmanabhan wrote:
>     We want the default value to match the one from 
> settings.explicitBatchSize on the scheduler. So on the scheduler we use the 
> default value if the value here is 0. Although I agree that the 0 here is a 
> bit misleading. 
>     
>     I like the optional batchSize argument for the explicit reconcile RPC. 
>     
>     I am thinking there can be a 
>     
>     struct ExplicitReconcileSettings {
>      1. optional i32 batchSize;
>     }
>     
>     This can let us add more explicit reconcile settings in the future. 
>     
>     Also does thrift lets us declare optional parameters in a function 
> (unless it is wrapped in a struct) ?
> 
> Santhosh Kumar Shanmugham wrote:
>     AFAIK, Thrift does not support optional function parameters, due to the 
> limitations of the languages that it gets translated to. You are on the 
> correct path, in making this a 'struct'.

Don't really see much value in overcomplicating this scenario. You can leave 
the `int` on the API side by have your command_option as `default=None` and 
traslate `None` to `0` in the client/api call. Your call.


- Maxim


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


On Sept. 7, 2016, 9:08 p.m., Karthik Anantha Padmanabhan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51662/
> -----------------------------------------------------------
> 
> (Updated Sept. 7, 2016, 9:08 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> AURORA-1602 Aurora admin commands for reconcilation
> 
> 
> Diffs
> -----
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   
> src/main/java/org/apache/aurora/scheduler/reconciliation/TaskReconciler.java 
> 3275d72a0a74909e635bce615e2036ec72aa38ee 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   src/main/python/apache/aurora/admin/admin.py 
> 76009b9c1c7a130c25abad544a176dc590dafb12 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 9149c3018ae58d405f284fcbd4076d251ccc8192 
>   
> src/test/java/org/apache/aurora/scheduler/reconciliation/TaskReconcilerTest.java
>  b9317dc20456f90057ec2bf4d10619a5ae986187 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  779dc302602ae8842084807ca868a491ea99b676 
>   src/test/python/apache/aurora/admin/test_admin.py 
> eb193c4a33f5275f05a995338446bec0e19bc1cf 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afac2500551af2fce406bb906aa4e33f353e90a1 
> 
> Diff: https://reviews.apache.org/r/51662/diff/
> 
> 
> Testing
> -------
> 
> * Manually tested on my local vagrant installation
> * ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> Karthik Anantha Padmanabhan
> 
>

Reply via email to