subject:"Re\: Review Request 66192\: \[WIP\] Variable group size updates"

Re: Review Request 66192: [WIP] Variable group size updates

2018-08-24 Thread Renan DelValle


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

(Updated Aug. 24, 2018, 4:43 p.m.)


Review request for Aurora, David McLaughlin, Jordan Ly, Santhosh Kumar 
Shanmugham, and Stephan Erb.


Changes
---

Done:

* Added ability for client to specify update strategy.
* Backwards compatibility support for Pystachio definitions that do not have or 
use Update Strategy.
* Changing tests in python client to reflect new update strategy.
* Adding code to handle backfilling update settings that did not have update 
strategies.
* Documentation.


Still to do:

* Change Web UI task page to show what update strategy is being used.
* Discuss wether it would be a good idea to merge VariableBatchUpdate code with 
BatchUpdate code since BatchUpdate is a specific case of VariableBatchUpdate.
* Add a few end to end tests.
* Add tests to python client for new data structures.


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 (updated)
-

  RELEASE-NOTES.md e6edbdeb577450f1119183a247e54a00e8a964d1 
  api/src/main/thrift/org/apache/aurora/gen/api.thrift 
dac2267dfe7e2e7563568adf8f04f6e9edc598d4 
  docs/reference/configuration.md 0632559e83ab1b547541c4087ae503318d2590d5 
  examples/jobs/hello_world_variable_update.aurora PRE-CREATION 
  
src/main/java/org/apache/aurora/scheduler/storage/durability/ThriftBackfill.java
 41a2f0b5b897c4fc3437cc8fd052478a4de8a426 
  
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 
74820c99ef769083a247608691dd7dea117a673a 
  src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 
bc8008ec5dcba98923703d0d35881f3fea7c5b2d 
  
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 
  src/main/python/apache/aurora/client/api/__init__.py 
34822bcbacbba7ca6c3cd6ebf7f63a5f9d1c820f 
  src/main/python/apache/aurora/client/api/updater_util.py 
5c2d95384a639a08cb0f2dece83c671de7e54764 
  src/main/python/apache/aurora/config/schema/base.py 
bf75660f44240ec78f27f8fc76418968bd0e2fc7 
  src/main/python/apache/aurora/config/thrift.py 
2ffa5b6905ecfbd1835df60cfe861a13f08a6492 
  
src/test/java/org/apache/aurora/scheduler/storage/AbstractJobUpdateStoreTest.java
 3a93650635fb92fea306498cb86e35a0e25cf847 
  
src/test/java/org/apache/aurora/scheduler/storage/durability/DataCompatibilityTest.java
 3372ceccbd91629d5fc3f45ac1ebdebc19326b32 
  
src/test/java/org/apache/aurora/scheduler/storage/durability/DurableStorageTest.java
 ddb9d06928de8ba73b6592b79cc741e5e97f0ff0 
  src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotterImplIT.java 
4c1918fc33b5123dc522c49739dcf5690bdec3b0 
  
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
 334fd5d321d202cd9dfc10941c43fd07e0296aeb 
  src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
c96def1a51d354d0a55cbb6f574306ef2b5237c2 
  src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 
01bdcf1f0613b9c0297321762e0d2b032dd4841c 
  
src/test/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategyTest.java
 PRE-CREATION 
  src/test/python/apache/aurora/client/api/test_api.py 
9d6d9de903bf0cb9ac2b6415e76f8fb70e051514 
  src/test/python/apache/aurora/client/api/test_updater_util.py 
48926c3695dbcf38abd33b2d58a64d8e1ea1cdd3 
  
src/test/resources/org/apache/aurora/scheduler/storage/durability/goldens/current/saveJobUpdate
 3876767610f1d535d95b7dbe5d27dd8188b42f05 


Diff: https://reviews.apache.org/r/66192/diff/4/

Changes: https://reviews.apache.org/r/66192/diff/3-4/


Testing (updated)
---

Jenkins integration test.

Watiing on end to end tests as they might be currently broken.


Thanks,

Renan DelValle

Re: Review Request 66192: [WIP] Variable group size updates

2018-05-16 Thread Renan DelValle



> On May 16, 2018, 2:54 p.m., Stephan Erb wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
> > Lines 784-786 (original), 788-828 (patched)
> > 
> >
> > Doing such a conversion and validation in `SchedulerThriftInterface` 
> > will only adapt newly submitted tasks. Tasks read from storage will still 
> > have the old configuration layout - this adds unnecessary complexity.
> > 
> > Please have a look at the `validateAndPopulate` method and the 
> > `ThriftBackfill` class used in there for guidance of how we normally handle 
> > this.

You are absolutely right. The backfill will be in the next version of this 
patch. I was thinking of a way of maintaining -1/+1 compatibility with thrift 
clients, but if that's not our usual MO, then I'm all for just having the 
backfill.


- Renan


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


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
> 
>

Re: Review Request 66192: [WIP] Variable group size updates

2018-05-16 Thread Stephan Erb



> On May 15, 2018, 4:26 a.m., Aurora ReviewBot wrote:
> > Master (805a53f) is red with this patch.
> >   ./build-support/jenkins/build.sh
> > 
> > :compileJmhJavaNote: 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java
> >  uses or overrides a deprecated API.
> > Note: Recompile with -Xlint:deprecation for details.
> > 
> > :processJmhResources NO-SOURCE
> > :jmhClasses
> > :checkstyleJmh
> > :checkstyleMain[ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:789:7:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:790:9:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:802:7:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:805:14:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:808:14:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:816:9:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:123:9:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:127:16:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java:20:8:
> >  Unused import - com.google.common.collect.FluentIterable. [UnusedImports]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java:22:
> >  Wrong order for 'java.util.stream.Collectors' import. Order should be: 
> > java, javax, scala, com, net, org. Each group should be separated 
> > by a single blank line. [ImportOrder]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:19:
> >  Wrong order for 'java.util.List' import. Order should be: java, javax, 
> > scala, com, net, org. Each group should be separated by a single 
> > blank line. [ImportOrder]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:25:
> >  Wrong order for 'java.util.stream.Collectors' import. Order should be: 
> > java, javax, scala, com, net, org. Each group should be separated 
> > by a single blank line. [ImportOrder]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:70:11:
> >  Redundant 'final' modifier. [RedundantModifier]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:70:44:
> >  '{' is not preceded with whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:100:11:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:109:49:
> >  '-' is not preceded with whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:109:50:
> >  '-' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:117:7:
> >  'if'

Re: Review Request 66192: [WIP] Variable group size updates

2018-05-16 Thread Stephan Erb


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




src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
Lines 784-786 (original), 788-828 (patched)


Doing such a conversion and validation in `SchedulerThriftInterface` will 
only adapt newly submitted tasks. Tasks read from storage will still have the 
old configuration layout - this adds unnecessary complexity.

Please have a look at the `validateAndPopulate` method and the 
`ThriftBackfill` class used in there for guidance of how we normally handle 
this.



src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java
Lines 122 (patched)


With the current backfill mechanism, this will break for tasks read from 
storage.


- Stephan Erb


On May 15, 2018, 4:19 a.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66192/
> ---
> 
> (Updated May 15, 2018, 4:19 a.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
> 
>

Re: Review Request 66192: [WIP] Variable group size updates

2018-05-15 Thread Santhosh Kumar Shanmugham



> 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)
> > 
> >
> > Sums up to be exactly `mutableRequest.instanceCount`?
> 
> Renan DelValle wrote:
> 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.)

That makes sense. I missed the earlier conversation.


> 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)
> > 
> >
> > Can you include an example for the rolling forward and backward cases?
> 
> Renan DelValle wrote:
> Can you expand on what you mean by including an example? Should I put it 
> in as a comment on the code?

Yes. Please include that in the javadoc.


- Santhosh Kumar


---
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
> 
>

Re: Review Request 66192: [WIP] Variable group size updates

2018-05-15 Thread Santhosh Kumar Shanmugham



> On May 14, 2018, 7:26 p.m., Aurora ReviewBot wrote:
> > Master (805a53f) is red with this patch.
> >   ./build-support/jenkins/build.sh
> > 
> > :compileJmhJavaNote: 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java
> >  uses or overrides a deprecated API.
> > Note: Recompile with -Xlint:deprecation for details.
> > 
> > :processJmhResources NO-SOURCE
> > :jmhClasses
> > :checkstyleJmh
> > :checkstyleMain[ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:789:7:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:790:9:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:802:7:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:805:14:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:808:14:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:816:9:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:123:9:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:127:16:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java:20:8:
> >  Unused import - com.google.common.collect.FluentIterable. [UnusedImports]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java:22:
> >  Wrong order for 'java.util.stream.Collectors' import. Order should be: 
> > java, javax, scala, com, net, org. Each group should be separated 
> > by a single blank line. [ImportOrder]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:19:
> >  Wrong order for 'java.util.List' import. Order should be: java, javax, 
> > scala, com, net, org. Each group should be separated by a single 
> > blank line. [ImportOrder]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:25:
> >  Wrong order for 'java.util.stream.Collectors' import. Order should be: 
> > java, javax, scala, com, net, org. Each group should be separated 
> > by a single blank line. [ImportOrder]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:70:11:
> >  Redundant 'final' modifier. [RedundantModifier]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:70:44:
> >  '{' is not preceded with whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:100:11:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:109:49:
> >  '-' is not preceded with whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:109:50:
> >  '-' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:117:7:
> >  'if'

Re: Review Request 66192: [WIP] Variable group size updates

2018-05-15 Thread Renan DelValle



> 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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
> 
>

Re: Review Request 66192: [WIP] Variable group size updates

2018-05-15 Thread Renan DelValle



> On May 14, 2018, 7:26 p.m., Aurora ReviewBot wrote:
> > Master (805a53f) is red with this patch.
> >   ./build-support/jenkins/build.sh
> > 
> > :compileJmhJavaNote: 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java
> >  uses or overrides a deprecated API.
> > Note: Recompile with -Xlint:deprecation for details.
> > 
> > :processJmhResources NO-SOURCE
> > :jmhClasses
> > :checkstyleJmh
> > :checkstyleMain[ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:789:7:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:790:9:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:802:7:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:805:14:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:808:14:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:816:9:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:123:9:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:127:16:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java:20:8:
> >  Unused import - com.google.common.collect.FluentIterable. [UnusedImports]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java:22:
> >  Wrong order for 'java.util.stream.Collectors' import. Order should be: 
> > java, javax, scala, com, net, org. Each group should be separated 
> > by a single blank line. [ImportOrder]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:19:
> >  Wrong order for 'java.util.List' import. Order should be: java, javax, 
> > scala, com, net, org. Each group should be separated by a single 
> > blank line. [ImportOrder]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:25:
> >  Wrong order for 'java.util.stream.Collectors' import. Order should be: 
> > java, javax, scala, com, net, org. Each group should be separated 
> > by a single blank line. [ImportOrder]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:70:11:
> >  Redundant 'final' modifier. [RedundantModifier]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:70:44:
> >  '{' is not preceded with whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:100:11:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:109:49:
> >  '-' is not preceded with whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:109:50:
> >  '-' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:117:7:
> >  'if'

Re: Review Request 66192: [WIP] Variable group size updates

2018-05-15 Thread Santhosh Kumar Shanmugham



> On May 14, 2018, 7:26 p.m., Aurora ReviewBot wrote:
> > Master (805a53f) is red with this patch.
> >   ./build-support/jenkins/build.sh
> > 
> > :compileJmhJavaNote: 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java
> >  uses or overrides a deprecated API.
> > Note: Recompile with -Xlint:deprecation for details.
> > 
> > :processJmhResources NO-SOURCE
> > :jmhClasses
> > :checkstyleJmh
> > :checkstyleMain[ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:789:7:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:790:9:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:802:7:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:805:14:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:808:14:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:816:9:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:123:9:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:127:16:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java:20:8:
> >  Unused import - com.google.common.collect.FluentIterable. [UnusedImports]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java:22:
> >  Wrong order for 'java.util.stream.Collectors' import. Order should be: 
> > java, javax, scala, com, net, org. Each group should be separated 
> > by a single blank line. [ImportOrder]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:19:
> >  Wrong order for 'java.util.List' import. Order should be: java, javax, 
> > scala, com, net, org. Each group should be separated by a single 
> > blank line. [ImportOrder]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:25:
> >  Wrong order for 'java.util.stream.Collectors' import. Order should be: 
> > java, javax, scala, com, net, org. Each group should be separated 
> > by a single blank line. [ImportOrder]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:70:11:
> >  Redundant 'final' modifier. [RedundantModifier]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:70:44:
> >  '{' is not preceded with whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:100:11:
> >  'if' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:109:49:
> >  '-' is not preceded with whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:109:50:
> >  '-' is not followed by whitespace. [WhitespaceAround]
> > [ant:checkstyle] [ERROR] 
> > /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:117:7:
> >  'if'

Re: Review Request 66192: [WIP] Variable group size updates

2018-05-14 Thread Aurora ReviewBot


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



Master (805a53f) is red with this patch.
  ./build-support/jenkins/build.sh

:compileJmhJavaNote: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/jmh/java/org/apache/aurora/benchmark/fakes/FakeSchedulerDriver.java
 uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.

:processJmhResources NO-SOURCE
:jmhClasses
:checkstyleJmh
:checkstyleMain[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:789:7:
 'if' is not followed by whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:790:9:
 'if' is not followed by whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:802:7:
 'if' is not followed by whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:805:14:
 'if' is not followed by whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:808:14:
 'if' is not followed by whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:816:9:
 'if' is not followed by whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:123:9:
 'if' is not followed by whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:127:16:
 'if' is not followed by whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java:20:8:
 Unused import - com.google.common.collect.FluentIterable. [UnusedImports]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java:22:
 Wrong order for 'java.util.stream.Collectors' import. Order should be: java, 
javax, scala, com, net, org. Each group should be separated by a single 
blank line. [ImportOrder]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:19:
 Wrong order for 'java.util.List' import. Order should be: java, javax, scala, 
com, net, org. Each group should be separated by a single blank line. 
[ImportOrder]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:25:
 Wrong order for 'java.util.stream.Collectors' import. Order should be: java, 
javax, scala, com, net, org. Each group should be separated by a single 
blank line. [ImportOrder]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:70:11:
 Redundant 'final' modifier. [RedundantModifier]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:70:44:
 '{' is not preceded with whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:100:11:
 'if' is not followed by whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:109:49:
 '-' is not preceded with whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:109:50:
 '-' is not followed by whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:117:7:
 'if' is not followed by whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR]

Re: Review Request 66192: [WIP] Variable group size updates

2018-05-14 Thread Renan DelValle

---
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.

Changes
---

* After an offline conversation with Jordan, changing Thrift Schema to make it
a better experience from the UX perspective.
* Using the number of overall instance changes to be made (kill, update,
create) to calculate the step. This fixes a previous issue where starting an
update which would result in a decreased instance count would behave oddly.
* Addressed the nits pointed out by Santhosh.

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 (updated)
-

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/

Changes: https://reviews.apache.org/r/66192/diff/2-3/

Testing
---

Thanks,

Renan DelValle

Re: Review Request 66192: [WIP] Variable group size updates

2018-05-14 Thread Santhosh Kumar Shanmugham


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



Approach looks good to me.


src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
Lines 826 (patched)


Sums up to be exactly `mutableRequest.instanceCount`?



src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java
Lines 46 (patched)


nit - s/Creates an/Creates a/



src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java
Lines 65 (patched)


Can you include an example for the rolling forward and backward cases?



src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java
Lines 67 (patched)


nit - s/where/we are/


- Santhosh Kumar Shanmugham


On May 8, 2018, 4:26 p.m., Renan DelValle wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66192/
> ---
> 
> (Updated May 8, 2018, 4:26 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/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Renan DelValle
> 
>

Re: Review Request 66192: [WIP] Variable group size updates

2018-05-08 Thread Aurora ReviewBot


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



Master (805a53f) is red with this patch.
  ./build-support/jenkins/build.sh

[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:804:13:
 'if' is not followed by whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:816:7:
 'if' is not followed by whitespace. [WhitespaceAround]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:122:
 'case' child have incorrect indentation level 10, expected level should be 8. 
[Indentation]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:123:
 'block' child have incorrect indentation level 12, expected level should be 
10. [Indentation]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:125:
 'block' child have incorrect indentation level 12, expected level should be 
10. [Indentation]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:127:
 'case' child have incorrect indentation level 10, expected level should be 8. 
[Indentation]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:128:
 'block' child have incorrect indentation level 12, expected level should be 
10. [Indentation]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:129:
 'block' child have incorrect indentation level 12, expected level should be 
10. [Indentation]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:131:
 'case' child have incorrect indentation level 10, expected level should be 8. 
[Indentation]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:132:
 'case' child have incorrect indentation level 10, expected level should be 8. 
[Indentation]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java:135:
 'block' child have incorrect indentation level 12, expected level should be 
10. [Indentation]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java:20:8:
 Unused import - com.google.common.collect.FluentIterable. [UnusedImports]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java:22:
 Wrong order for 'java.util.stream.Collectors' import. Order should be: java, 
javax, scala, com, net, org. Each group should be separated by a single 
blank line. [ImportOrder]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:19:
 Wrong order for 'java.util.List' import. Order should be: java, javax, scala, 
com, net, org. Each group should be separated by a single blank line. 
[ImportOrder]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:25:
 Wrong order for 'java.util.stream.Collectors' import. Order should be: java, 
javax, scala, com, net, org. Each group should be separated by a single 
blank line. [ImportOrder]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:48:
 Line is longer than 100 characters (found 101). [LineLength]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:51:
 Line is longer than 100 characters (found 109). [LineLength]
[ant:checkstyle] [ERROR] 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:65:11:
 Redundant 'final' modifier. [RedundantModifier]
[ant:checkstyle] [ERROR]

Re: Review Request 66192: [WIP] Variable group size updates

2018-05-08 Thread Renan DelValle

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

(Updated May 8, 2018, 4:26 p.m.)

Review request for Aurora, David McLaughlin, Jordan Ly, Santhosh Kumar
Shanmugham, and Stephan Erb.

Changes
---

* Variable update group strategy now determines what step it is in dynamically
-- no more writing to storage.
* Surface are greatly reduced and potential path towards merging Batch and
Variable Batch were created.
* Currently works for the following cases:
- Update results in more instances.
- Update only updates existing instances.
* Does not work in its current form when the update results in less instances
(will address this in the next iteration).
* Introduced an optional JobUpdateStrategyType which allows for explicitly
setting which strategy to use in order to avoid confusion. If this field is not
set, the type is determined to be QUEUE or BATCH depending on
waitFortBatchCompletion for backwards compatibility. The plan is to deprecate
waitForBatchCompletion and updateGroupSize depending on feedback.
* Backwards compatible by using optional fields and using old fields to
populate the new ones.

Thanks for all the feedback so far, very much appreciated!

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.

Diffs (updated)
-

api/src/main/thrift/org/apache/aurora/gen/api.thrift
ef754e32172e7490a47a13e7b526f243ffa3efeb

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/2/

Changes: https://reviews.apache.org/r/66192/diff/1-2/

Testing
---

Thanks,

Renan DelValle

Re: Review Request 66192: [WIP] Variable group size updates

2018-03-23 Thread Stephan Erb



> 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:

Re: Review Request 66192: [WIP] Variable group size updates

2018-03-21 Thread Renan DelValle



> On March 20, 2018, 9:29 p.m., David McLaughlin wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java
> > Lines 107-111 (patched)
> > 
> >
> > Is this idea of a step sate needed? Strategies writing to the database 
> > seems like a design smell. 
> > 
> > If I have 10 instances:
> > 
> > [0,1,2,3,4,5,6,7,8,9] 
> > 
> > And I pass a VariableBatchStrategy of: [1, 4, 5]
> > 
> > Then using the BatchStrategy it is *always* in the following order:
> > 
> > [[0], [1,2,3,4], [5,6,7,8,9]]
> > 
> > And I don't *think* I need to write the step to the database to figure 
> > out which step I'm in? I can use the number idle and active to figure out 
> > exactly how many have been processed.
> 
> Jordan Ly wrote:
> +1, removing the need for persistent state would greatly reduce the 
> surface area of this patch.
> 
> Santhosh Kumar Shanmugham wrote:
> +1

Thanks for the feedback! 

I thought about doing it this way originally, but I wasn't totally convinced it 
would work well given the only info I had to go on inside of the strategy was 
how many tasks were waiting to be updated and how many were in the progress of 
being updated. 

As David pointed out, the order will remain the same (unless of course changes 
are made to 
https://github.com/apache/aurora/blob/2e1ca42887bc8ea1e8c6cddebe9d1cf29268c714/src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java#L190
 ) so the update order should be deterministic (at least within the same 
version of Aurora).


The key points for this feature are: how this would survive leader 
elections/scheduler restarts, being able to roll back in a predictable manner 
at any given point of failure, and manainting the order in which tasks(shards) 
would get updated. 

I think I should be able to tackle all of these without writing to the database 
to persist with this new approach.


- Renan


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


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 
>   
>

Re: Review Request 66192: [WIP] Variable group size updates

2018-03-21 Thread Renan DelValle



> 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
> 
>

Re: Review Request 66192: [WIP] Variable group size updates

2018-03-21 Thread Santhosh Kumar Shanmugham



> On March 20, 2018, 9:29 p.m., David McLaughlin wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java
> > Lines 107-111 (patched)
> > 
> >
> > Is this idea of a step sate needed? Strategies writing to the database 
> > seems like a design smell. 
> > 
> > If I have 10 instances:
> > 
> > [0,1,2,3,4,5,6,7,8,9] 
> > 
> > And I pass a VariableBatchStrategy of: [1, 4, 5]
> > 
> > Then using the BatchStrategy it is *always* in the following order:
> > 
> > [[0], [1,2,3,4], [5,6,7,8,9]]
> > 
> > And I don't *think* I need to write the step to the database to figure 
> > out which step I'm in? I can use the number idle and active to figure out 
> > exactly how many have been processed.
> 
> Jordan Ly wrote:
> +1, removing the need for persistent state would greatly reduce the 
> surface area of this patch.

+1


- Santhosh Kumar


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


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
> 
>

Re: Review Request 66192: [WIP] Variable group size updates

2018-03-21 Thread Santhosh Kumar Shanmugham



> 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).

+1 I think it should be an either-or. There should be logic in the API to 
clearly message this case.


- Santhosh Kumar


---
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
> 
>

Re: Review Request 66192: [WIP] Variable group size updates

2018-03-21 Thread Jordan Ly


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



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).

- Jordan Ly


On March 21, 2018, 2: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, 2: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
> 
>

Re: Review Request 66192: [WIP] Variable group size updates

2018-03-20 Thread David McLaughlin


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




src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java
Lines 107-111 (patched)


Is this idea of a step sate needed? Strategies writing to the database 
seems like a design smell. 

If I have 10 instances:

[0,1,2,3,4,5,6,7,8,9] 

And I pass a VariableBatchStrategy of: [1, 4, 5]

Then using the BatchStrategy it is *always* in the following order:

[[0], [1,2,3,4], [5,6,7,8,9]]

And I don't *think* I need to write the step to the database to figure out 
which step I'm in? I can use the number idle and active to figure out exactly 
how many have been processed.


- David McLaughlin


On March 21, 2018, 2: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, 2: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
> 
>

Re: Review Request 66192: [WIP] Variable group size updates

2018-03-20 Thread Aurora ReviewBot

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

Master (f32086d) is red with this patch.
  ./build-support/jenkins/build.sh

  found: IJobUpdateInstructions,boolean
  reason: actual and formal argument lists differ in length
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java:78:
 error: method newUpdate in interface UpdateFactory cannot be applied to given 
types;
Update update = factory.newUpdate(IJobUpdateInstructions.build(config), 
true);
   ^
  required: IJobUpdate,boolean,Storage
  found: IJobUpdateInstructions,boolean
  reason: actual and formal argument lists differ in length
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java:89:
 error: method newUpdate in interface UpdateFactory cannot be applied to given 
types;
Update update = factory.newUpdate(IJobUpdateInstructions.build(config), 
false);
   ^
  required: IJobUpdate,boolean,Storage
  found: IJobUpdateInstructions,boolean
  reason: actual and formal argument lists differ in length
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java:98:
 error: method newUpdate in interface UpdateFactory cannot be applied to given 
types;
Update update = factory.newUpdate(IJobUpdateInstructions.build(config), 
true);
   ^
  required: IJobUpdate,boolean,Storage
  found: IJobUpdateInstructions,boolean
  reason: actual and formal argument lists differ in length
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/java/org/apache/aurora/scheduler/storage/durability/DurableStorageTest.java
 uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
5 errors
 FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':compileTestJava'.
> Compilation failed; see the compiler error output for details.

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug 
option to get more log output.

* Get more help at https://help.gradle.org

BUILD FAILED in 3m 48s
28 actionable tasks: 22 executed, 6 up-to-date

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot

On March 21, 2018, 2: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, 2: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 
>   
>

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

Re: Review Request 66192: [WIP] Variable group size updates

22 matches

Site Navigation

Mail list logo

Footer information