> On May 14, 2018, 10:58 a.m., Santhosh Kumar Shanmugham wrote: > > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java > > Lines 826 (patched) > > <https://reviews.apache.org/r/66192/diff/2/?file=2018260#file2018260line826> > > > > Sums up to be exactly `mutableRequest.instanceCount`?
This is something I wonderd myself. The current design follows the philosophy that the sum of all update groups does not have to be equal to the instance count. If the instance count is greater than the instanceCount, the update will carry forward until we run out of instance to update. For example, if we have update groups 1,2,3 and our instance count is 5, we will get the following steps in practice: 1, 2, 2 If the instance count is lesser than the instance count, the update will forward repeating the value of the last group size until the update completes. For example, if we have update groups 1,2 and our instance count is 5, we will get the following steps: 1, 2, 2 I will write thourough documentation on this so that users know what to expect when this update strategy is used. One benefit of implementing the variable group size update this way is that it provides a path going forward to have a single batch strategy in the code base. Since we repeat the last group size we have, having a list of group sizes of length 1 is equivalent to a batch update. (This was done based on a comment by Stephan during the first review round that resonated with me.) > On May 14, 2018, 10:58 a.m., Santhosh Kumar Shanmugham wrote: > > src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java > > Lines 46 (patched) > > <https://reviews.apache.org/r/66192/diff/2/?file=2018263#file2018263line46> > > > > nit - s/Creates an/Creates a/ Fixed thanks! > On May 14, 2018, 10:58 a.m., Santhosh Kumar Shanmugham wrote: > > src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java > > Lines 65 (patched) > > <https://reviews.apache.org/r/66192/diff/2/?file=2018263#file2018263line65> > > > > Can you include an example for the rolling forward and backward cases? Can you expand on what you mean by including an example? Should I put it in as a comment on the code? > On May 14, 2018, 10:58 a.m., Santhosh Kumar Shanmugham wrote: > > src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java > > Lines 67 (patched) > > <https://reviews.apache.org/r/66192/diff/2/?file=2018263#file2018263line67> > > > > nit - s/where/we are/ Fixed thanks! - Renan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66192/#review203048 ----------------------------------------------------------- On May 14, 2018, 7:19 p.m., Renan DelValle wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66192/ > ----------------------------------------------------------- > > (Updated May 14, 2018, 7:19 p.m.) > > > Review request for Aurora, David McLaughlin, Jordan Ly, Santhosh Kumar > Shanmugham, and Stephan Erb. > > > Repository: aurora > > > Description > ------- > > Adding support for variable group sizes when executing an update. > > Design doc for this change is here: > https://docs.google.com/document/d/1xGk4ueH8YlmJCk6hQJh85u4to4M1VQD0l630IOchvgY/edit#heading=h.lg3hty82f5cz > > I opted for the path of least resistance with regards to the Thrift changes > as I didn't see any benefit in making the larger changes required to make the > interfaces a bit more flexible. > > Requesting feedback on these changes and the approach from the community > before I proceed. > > Tests will be added after the community approves of the direciton and > approach. > > Note to reviewers: Changes made in ActiveLimitedStrategy.java were made to > move towards getting rid of FluentIterable. I figured since I was touching > that code, it wouldn't hurt to test the Java 8 equivalent of it. I can get > rid of the change here and make it in a separate patch if desired. > > > Diffs > ----- > > api/src/main/thrift/org/apache/aurora/gen/api.thrift > ef754e32172e7490a47a13e7b526f243ffa3efeb > > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java > 9fc0416086dd3eb2e2f4e8f659da59fcdea2b22b > src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java > 3992aa77fc305adc390a4aaeb1d3939d6241ddbd > > src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java > 855ea9c20788b51695b7eff5ac0970f0d52a9546 > > src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/66192/diff/3/ > > > Testing > ------- > > > Thanks, > > Renan DelValle > >
