> On March 21, 2018, 10:33 p.m., Jordan Ly wrote: > > I am mostly concerned about the UX. Users will be able to specify both > > batch size and variable batch size and must know that variable batch sizing > > takes precedent over other strategies. > > > > Is it worth it to make a larger investment into the Thrift interface and > > avoid ambiguity? Or refactor the current batching strategy to use the new > > variable codepath (a single batch size specified to the variable strategy > > should behave the same as the current implementation). > > Santhosh Kumar Shanmugham wrote: > +1 I think it should be an either-or. There should be logic in the API to > clearly message this case. > > Renan DelValle wrote: > I've thought about refactoring the batching strategy, and I'm willing to > travel down that path as the current batch stategy is a specific case (single > step, repeated over and over as Jordan pointed out) of the functionality that > I'm trying to achieve with this patch but I'll have to do something like wrap > around a single item list which might look funky. I'll give it a shot. > > Changing the Thrift interface is always tricky because it almost always > results in some yak shaving but if the consensus is that this is the better, > more clear approach, then I'm game to implement it that way. > > Appreciate all the feedback!
Just an idea: Would it help to cap `step` at the length of `maxActive`, so that the last batch size is used for all remaining instances? This would also give us a somewhat nice deprecation path for the existing fixed batch size. In addition, I believe you will need to account for that case anyway. A user might accidentally have a list of batches whose sum is smaller than the instance count of the update. We need to make sure that either the update request gets reject at submission or that it runs through properly. The proposal above would allow the update to run through. - Stephan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66192/#review199707 ----------------------------------------------------------- On March 21, 2018, 3:10 a.m., Renan DelValle wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66192/ > ----------------------------------------------------------- > > (Updated March 21, 2018, 3:10 a.m.) > > > Review request for Aurora, David McLaughlin, 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 > api/src/main/thrift/org/apache/aurora/gen/storage.thrift > b79e2045ccda05d5058565f81988dfe33feea8f1 > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java > b25f3831cecc58c90375c90b16142421f8f09e38 > src/main/java/org/apache/aurora/scheduler/storage/durability/Loader.java > 10864f122eff5027c88d835baae6de483d960218 > > src/main/java/org/apache/aurora/scheduler/storage/durability/WriteRecorder.java > 8d70cae35289a9e36142bab288cf0c9398ebd2d4 > > src/main/java/org/apache/aurora/scheduler/storage/mem/MemJobUpdateStore.java > 9e86b9e276ea90a249284a824705b5bbf19dcbce > > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java > 9fc0416086dd3eb2e2f4e8f659da59fcdea2b22b > > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java > f8be8058f3a80a18b999d2666e2adb33e1e55fef > 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/1/ > > > Testing > ------- > > > Thanks, > > Renan DelValle > >
