> On March 21, 2018, 2: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.
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! - Renan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66192/#review199707 ----------------------------------------------------------- On March 20, 2018, 7:10 p.m., Renan DelValle wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66192/ > ----------------------------------------------------------- > > (Updated March 20, 2018, 7:10 p.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 > >
