You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Renan DelValle <re...@apache.org> on 2018/05/08 23:26:41 UTC
Re: Review Request 66192: [WIP] Variable group size updates
-----------------------------------------------------------
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.
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/2/
Changes: https://reviews.apache.org/r/66192/diff/1-2/
Testing
-------
Thanks,
Renan DelValle
Re: Review Request 66192: [WIP] Variable group size updates
Posted by Renan DelValle <re...@apache.org>.
> On May 14, 2018, 10:58 a.m., Santhosh Kumar Shanmugham wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
> > Lines 826 (patched)
> > <https://reviews.apache.org/r/66192/diff/2/?file=2018260#file2018260line826>
> >
> > Sums up to be exactly `mutableRequest.instanceCount`?
This is something I wonderd myself. The current design follows the philosophy that the sum of all update groups does not have to be equal to the instance count.
If the instance count is greater than the instanceCount, the update will carry forward until we run out of instance to update.
For example, if we have update groups 1,2,3 and our instance count is 5, we will get the following steps in practice: 1, 2, 2
If the instance count is lesser than the instance count, the update will forward repeating the value of the last group size until the update completes.
For example, if we have update groups 1,2 and our instance count is 5, we will get the following steps: 1, 2, 2
I will write thourough documentation on this so that users know what to expect when this update strategy is used.
One benefit of implementing the variable group size update this way is that it provides a path going forward to have a single batch strategy in the code base.
Since we repeat the last group size we have, having a list of group sizes of length 1 is equivalent to a batch update. (This was done based on a comment by Stephan during the first review round that resonated with me.)
> On May 14, 2018, 10:58 a.m., Santhosh Kumar Shanmugham wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java
> > Lines 46 (patched)
> > <https://reviews.apache.org/r/66192/diff/2/?file=2018263#file2018263line46>
> >
> > nit - s/Creates an/Creates a/
Fixed thanks!
> On May 14, 2018, 10:58 a.m., Santhosh Kumar Shanmugham wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java
> > Lines 65 (patched)
> > <https://reviews.apache.org/r/66192/diff/2/?file=2018263#file2018263line65>
> >
> > Can you include an example for the rolling forward and backward cases?
Can you expand on what you mean by including an example? Should I put it in as a comment on the code?
> On May 14, 2018, 10:58 a.m., Santhosh Kumar Shanmugham wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java
> > Lines 67 (patched)
> > <https://reviews.apache.org/r/66192/diff/2/?file=2018263#file2018263line67>
> >
> > nit - s/where/we are/
Fixed thanks!
- Renan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66192/#review203048
-----------------------------------------------------------
On May 14, 2018, 7:19 p.m., Renan DelValle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66192/
> -----------------------------------------------------------
>
> (Updated May 14, 2018, 7:19 p.m.)
>
>
> Review request for Aurora, David McLaughlin, Jordan Ly, Santhosh Kumar Shanmugham, and Stephan Erb.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding support for variable group sizes when executing an update.
>
> Design doc for this change is here:
> https://docs.google.com/document/d/1xGk4ueH8YlmJCk6hQJh85u4to4M1VQD0l630IOchvgY/edit#heading=h.lg3hty82f5cz
>
> I opted for the path of least resistance with regards to the Thrift changes as I didn't see any benefit in making the larger changes required to make the interfaces a bit more flexible.
>
> Requesting feedback on these changes and the approach from the community before I proceed.
>
> Tests will be added after the community approves of the direciton and approach.
>
> Note to reviewers: Changes made in ActiveLimitedStrategy.java were made to move towards getting rid of FluentIterable. I figured since I was touching that code, it wouldn't hurt to test the Java 8 equivalent of it. I can get rid of the change here and make it in a separate patch if desired.
>
>
> Diffs
> -----
>
> api/src/main/thrift/org/apache/aurora/gen/api.thrift ef754e32172e7490a47a13e7b526f243ffa3efeb
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 9fc0416086dd3eb2e2f4e8f659da59fcdea2b22b
> src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 3992aa77fc305adc390a4aaeb1d3939d6241ddbd
> src/main/java/org/apache/aurora/scheduler/updater/strategy/ActiveLimitedStrategy.java 855ea9c20788b51695b7eff5ac0970f0d52a9546
> src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/66192/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Renan DelValle
>
>
Re: Review Request 66192: [WIP] Variable group size updates
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
> On May 14, 2018, 10:58 a.m., Santhosh Kumar Shanmugham wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
> > Lines 826 (patched)
> > <https://reviews.apache.org/r/66192/diff/2/?file=2018260#file2018260line826>
> >
> > Sums up to be exactly `mutableRequest.instanceCount`?
>
> 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)
> > <https://reviews.apache.org/r/66192/diff/2/?file=2018263#file2018263line65>
> >
> > 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
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/66192/#comment285106>
Sums up to be exactly `mutableRequest.instanceCount`?
src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java
Lines 46 (patched)
<https://reviews.apache.org/r/66192/#comment285107>
nit - s/Creates an/Creates a/
src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java
Lines 65 (patched)
<https://reviews.apache.org/r/66192/#comment285109>
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)
<https://reviews.apache.org/r/66192/#comment285108>
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
Posted by Stephan Erb <se...@apache.org>.
> 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' 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:127: Line is longer than 100 characters (found 101). [LineLength]
> > FAILED
> >
> > FAILURE: Build failed with an exception.
> >
> > * What went wrong:
> > Execution failed for task ':checkstyleMain'.
> > > Checkstyle rule violations were found. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/main.html
> >
> > * 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 5m 13s
> > 32 actionable tasks: 26 executed, 6 up-to-date
> >
> >
> > I will refresh this build result if you post a review containing "@ReviewBot retry"
>
> Santhosh Kumar Shanmugham wrote:
> Can you clean up the style issues?
>
> Also, will this be configurable via the pystachio config?
>
> Renan DelValle wrote:
> Definitely, just wanted to get the latest version out to get feedback on the Thrift Schema changes. This will indeed be configurable via pystachio, just wanted to settle on the Thrift Schema before making those changes.
>
> Santhosh Kumar Shanmugham wrote:
> Ack.
The changes in `api.thrift` look good to me!
- Stephan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66192/#review203093
-----------------------------------------------------------
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
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
> 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' 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:127: Line is longer than 100 characters (found 101). [LineLength]
> > FAILED
> >
> > FAILURE: Build failed with an exception.
> >
> > * What went wrong:
> > Execution failed for task ':checkstyleMain'.
> > > Checkstyle rule violations were found. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/main.html
> >
> > * 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 5m 13s
> > 32 actionable tasks: 26 executed, 6 up-to-date
> >
> >
> > I will refresh this build result if you post a review containing "@ReviewBot retry"
Can you clean up the style issues?
Also, will this be configurable via the pystachio config?
- Santhosh Kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66192/#review203093
-----------------------------------------------------------
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
Posted by Renan DelValle <re...@apache.org>.
> 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' 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:127: Line is longer than 100 characters (found 101). [LineLength]
> > FAILED
> >
> > FAILURE: Build failed with an exception.
> >
> > * What went wrong:
> > Execution failed for task ':checkstyleMain'.
> > > Checkstyle rule violations were found. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/main.html
> >
> > * 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 5m 13s
> > 32 actionable tasks: 26 executed, 6 up-to-date
> >
> >
> > I will refresh this build result if you post a review containing "@ReviewBot retry"
>
> Santhosh Kumar Shanmugham wrote:
> Can you clean up the style issues?
>
> Also, will this be configurable via the pystachio config?
Definitely, just wanted to get the latest version out to get feedback on the Thrift Schema changes. This will indeed be configurable via pystachio, just wanted to settle on the Thrift Schema before making those changes.
- Renan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66192/#review203093
-----------------------------------------------------------
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
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
> 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' 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:127: Line is longer than 100 characters (found 101). [LineLength]
> > FAILED
> >
> > FAILURE: Build failed with an exception.
> >
> > * What went wrong:
> > Execution failed for task ':checkstyleMain'.
> > > Checkstyle rule violations were found. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/main.html
> >
> > * 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 5m 13s
> > 32 actionable tasks: 26 executed, 6 up-to-date
> >
> >
> > I will refresh this build result if you post a review containing "@ReviewBot retry"
>
> Santhosh Kumar Shanmugham wrote:
> Can you clean up the style issues?
>
> Also, will this be configurable via the pystachio config?
>
> Renan DelValle wrote:
> Definitely, just wanted to get the latest version out to get feedback on the Thrift Schema changes. This will indeed be configurable via pystachio, just wanted to settle on the Thrift Schema before making those changes.
Ack.
- Santhosh Kumar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66192/#review203093
-----------------------------------------------------------
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
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
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] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:127: Line is longer than 100 characters (found 101). [LineLength]
FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':checkstyleMain'.
> Checkstyle rule violations were found. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/main.html
* 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 5m 13s
32 actionable tasks: 26 executed, 6 up-to-date
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On May 15, 2018, 2: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, 2: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
Posted by Renan DelValle <re...@apache.org>.
> 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)
> > <https://reviews.apache.org/r/66192/diff/3/?file=2022592#file2022592line788>
> >
> > 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
Posted by Stephan Erb <se...@apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/66192/#comment285401>
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)
<https://reviews.apache.org/r/66192/#comment285402>
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: Variable group size updates
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66192/#review208213
-----------------------------------------------------------
Master (80191a2) is red with this patch.
./build-support/jenkins/build.sh
52 errors, 0 warnings potentially fixable with the `--fix` option.
npm ERR! Linux 4.4.0-131-generic
npm ERR! argv "/home/jenkins/jenkins-slave/workspace/AuroraBot/ui/.gradle/nodejs/node-v6.9.1-linux-x64/bin/node" "/home/jenkins/jenkins-slave/workspace/AuroraBot/ui/.gradle/nodejs/node-v6.9.1-linux-x64/bin/npm" "run" "lint"
npm ERR! node v6.9.1
npm ERR! npm v3.10.8
npm ERR! code ELIFECYCLE
npm ERR! apache-aurora@1.0.0 lint: `eslint src/main/js plugin/ --ext .js`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the apache-aurora@1.0.0 lint script 'eslint src/main/js plugin/ --ext .js'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the apache-aurora package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR! eslint src/main/js plugin/ --ext .js
npm ERR! You can get information on how to open an issue for this project with:
npm ERR! npm bugs apache-aurora
npm ERR! Or if that isn't available, you can get their info via:
npm ERR! npm owner ls apache-aurora
npm ERR! There is likely additional logging output above.
npm ERR! Please include the following file with any support request:
npm ERR! /home/jenkins/jenkins-slave/workspace/AuroraBot/ui/npm-debug.log
:ui:lint FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':ui:lint'.
> Process 'command '/home/jenkins/jenkins-slave/workspace/AuroraBot/ui/.gradle/nodejs/node-v6.9.1-linux-x64/bin/npm'' finished with non-zero exit value 1
* 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 8m 2s
56 actionable tasks: 45 executed, 11 up-to-date
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Sept. 1, 2018, 2:01 a.m., Renan DelValle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66192/
> -----------------------------------------------------------
>
> (Updated Sept. 1, 2018, 2:01 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
>
>
> Diffs
> -----
>
> 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
> ui/src/main/js/components/UpdateSettings.js d7fbe00859d89cc0523a2aa604abba9585000662
>
>
> Diff: https://reviews.apache.org/r/66192/diff/5/
>
>
> Testing
> -------
>
> Jenkins integration test passed.
>
> End to end tests passed.
>
>
> File Attachments
> ----------------
>
> Queue update strategy
> https://reviews.apache.org/media/uploaded/files/2018/09/01/dbd016f3-95e8-49f9-9e7b-e0c6d62eab97__Screenshot_from_2018-08-31_18-57-47.png
> Batch Update Strategy
> https://reviews.apache.org/media/uploaded/files/2018/09/01/f28728fc-6eae-471e-b69f-bb6fdffdd342__updatebatch.png
> Variable Batch Strategy
> https://reviews.apache.org/media/uploaded/files/2018/09/01/7b153c90-213a-4267-ac64-6bbbccb14d73__updatestrategyvarbatch.png
>
>
> Thanks,
>
> Renan DelValle
>
>
Re: Review Request 66192: Variable group size updates
Posted by Renan DelValle <re...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66192/
-----------------------------------------------------------
(Updated Aug. 31, 2018, 7:01 p.m.)
Review request for Aurora, David McLaughlin, Jordan Ly, Santhosh Kumar Shanmugham, and Stephan Erb.
Changes
-------
Added a python tests to make sure thrift conversion from pystachio works.
Changed Update Web UI to show update strategy being used.
Still to do:
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 update strategy field to UI tests.
Repository: aurora
Description (updated)
-------
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
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
ui/src/main/js/components/UpdateSettings.js d7fbe00859d89cc0523a2aa604abba9585000662
Diff: https://reviews.apache.org/r/66192/diff/5/
Changes: https://reviews.apache.org/r/66192/diff/4-5/
Testing
-------
Jenkins integration test passed.
End to end tests passed.
File Attachments (updated)
----------------
Queue update strategy
https://reviews.apache.org/media/uploaded/files/2018/09/01/dbd016f3-95e8-49f9-9e7b-e0c6d62eab97__Screenshot_from_2018-08-31_18-57-47.png
Batch Update Strategy
https://reviews.apache.org/media/uploaded/files/2018/09/01/f28728fc-6eae-471e-b69f-bb6fdffdd342__updatebatch.png
Variable Batch Strategy
https://reviews.apache.org/media/uploaded/files/2018/09/01/7b153c90-213a-4267-ac64-6bbbccb14d73__updatestrategyvarbatch.png
Thanks,
Renan DelValle
Re: Review Request 66192: Variable group size updates
Posted by Renan DelValle <re...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66192/
-----------------------------------------------------------
(Updated Aug. 27, 2018, 3:44 p.m.)
Review request for Aurora, David McLaughlin, Jordan Ly, Santhosh Kumar Shanmugham, and Stephan Erb.
Changes
-------
Updating tests done to reflect end to end tests passed.
Summary (updated)
-----------------
Variable group size updates
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
-----
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/
Testing (updated)
-------
Jenkins integration test passed.
End to end tests passed.
Thanks,
Renan DelValle
Re: Review Request 66192: [WIP] Variable group size updates
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66192/#review207930
-----------------------------------------------------------
Ship it!
Master (29eae2c) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Aug. 24, 2018, 11:43 p.m., Renan DelValle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66192/
> -----------------------------------------------------------
>
> (Updated Aug. 24, 2018, 11:43 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
> -----
>
> 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/
>
>
> Testing
> -------
>
> 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
Posted by Renan DelValle <re...@apache.org>.
-----------------------------------------------------------
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
Posted by Renan DelValle <re...@apache.org>.
-----------------------------------------------------------
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
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
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] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:65: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:74: Line is longer than 100 characters (found 114). [LineLength]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:81: Line is longer than 100 characters (found 114). [LineLength]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:87: Line is longer than 100 characters (found 116). [LineLength]
[ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/updater/strategy/VariableBatchStrategy.java:94: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:94: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:106: Line is longer than 100 characters (found 101). [LineLength]
FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':checkstyleMain'.
> Checkstyle rule violations were found. See the report at: file:///home/jenkins/jenkins-slave/workspace/AuroraBot/dist/reports/checkstyle/main.html
* 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 4m 12s
32 actionable tasks: 26 executed, 6 up-to-date
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On May 8, 2018, 11: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, 11: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
>
>