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...@gmail.com> on 2018/01/03 23:29:02 UTC
Review Request 64934: Custom converter to allow the
-thermos_executor_resources flag to take an empty string and parse it to an
empty list
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64934/
-----------------------------------------------------------
Review request for Aurora, Stephan Erb and Bill Farner.
Repository: aurora
Description
-------
Fixes the issue that caused the voting to fail for the 0.19.0 Aurora packages.
Fix cribbed from: https://github.com/cbeust/jcommander/pull/422
Implemented as a custom converter as suggested here: https://reviews.apache.org/r/64824/
Diffs
-----
src/main/java/org/apache/aurora/scheduler/config/converters/StringListConverter.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java 76ce39e837ea9dce81fdc66018611dfd11612388
Diff: https://reviews.apache.org/r/64934/diff/1/
Testing
-------
End to end.
Style check.
Integration.
Ran vagrant box, added `-thermos_executor_resources=""` created and killed a job.
Thanks,
Renan DelValle
Re: Review Request 64934: Custom converter to allow the
-thermos_executor_resources flag to take an empty string and parse it to an
empty list
Posted by Bill Farner <wf...@apache.org>.
> On Jan. 3, 2018, 3:49 p.m., Aurora ReviewBot wrote:
> > Master (f1d9caf) is green with this patch.
> > ./build-support/jenkins/build.sh
> >
> > However, it appears that it might lack test coverage.
> >
> > I will refresh this build result if you post a review containing "@ReviewBot retry"
> However, it appears that it might lack test coverage
I concur with the bot. Can you add a minimal regression test case in `CommandLineTest`?
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64934/#review194725
-----------------------------------------------------------
On Jan. 3, 2018, 3:29 p.m., Renan DelValle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64934/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2018, 3:29 p.m.)
>
>
> Review request for Aurora, Stephan Erb and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fixes the issue that caused the voting to fail for the 0.19.0 Aurora packages.
>
> Fix cribbed from: https://github.com/cbeust/jcommander/pull/422
>
> Implemented as a custom converter as suggested here: https://reviews.apache.org/r/64824/
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/config/converters/StringListConverter.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java 76ce39e837ea9dce81fdc66018611dfd11612388
>
>
> Diff: https://reviews.apache.org/r/64934/diff/1/
>
>
> Testing
> -------
>
> End to end.
> Style check.
> Integration.
> Ran vagrant box, added `-thermos_executor_resources=""` created and killed a job.
>
>
> Thanks,
>
> Renan DelValle
>
>
Re: Review Request 64934: Custom converter to allow the
-thermos_executor_resources flag to take an empty string and parse it to an
empty list
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64934/#review194725
-----------------------------------------------------------
Master (f1d9caf) is green with this patch.
./build-support/jenkins/build.sh
However, it appears that it might lack test coverage.
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Jan. 3, 2018, 11:29 p.m., Renan DelValle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64934/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2018, 11:29 p.m.)
>
>
> Review request for Aurora, Stephan Erb and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fixes the issue that caused the voting to fail for the 0.19.0 Aurora packages.
>
> Fix cribbed from: https://github.com/cbeust/jcommander/pull/422
>
> Implemented as a custom converter as suggested here: https://reviews.apache.org/r/64824/
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/config/converters/StringListConverter.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java 76ce39e837ea9dce81fdc66018611dfd11612388
>
>
> Diff: https://reviews.apache.org/r/64934/diff/1/
>
>
> Testing
> -------
>
> End to end.
> Style check.
> Integration.
> Ran vagrant box, added `-thermos_executor_resources=""` created and killed a job.
>
>
> Thanks,
>
> Renan DelValle
>
>
Re: Review Request 64934: Custom converter to allow the
-thermos_executor_resources flag to take an empty string and parse it to an
empty list
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64934/#review195048
-----------------------------------------------------------
Ship it!
Thanks for the patch!
- Bill Farner
On Jan. 8, 2018, 5:22 p.m., Renan DelValle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64934/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2018, 5:22 p.m.)
>
>
> Review request for Aurora, Stephan Erb and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fixes the issue that caused the voting to fail for the 0.19.0 Aurora packages.
>
> Fix cribbed from: https://github.com/cbeust/jcommander/pull/422
>
> Implemented as a custom converter as suggested here: https://reviews.apache.org/r/64824/
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/app/AppModule.java 817a0193f6d32632f2dfd05373114da7a3885ed2
> src/main/java/org/apache/aurora/scheduler/config/splitters/CommaSplitter.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java 76ce39e837ea9dce81fdc66018611dfd11612388
> src/main/java/org/apache/aurora/scheduler/discovery/FlaggedZooKeeperConfig.java 694a4fc1b4a28dea391558424d33b5656cf8a74c
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java 9d757dbe98b11df5f7e81cd81c361d000e648620
> src/main/java/org/apache/aurora/scheduler/offers/OfferManagerModule.java 823c36949d0e93f8578534b1bf22edaf1cdc58ac
> src/main/java/org/apache/aurora/scheduler/preemptor/PreemptorModule.java 007ebc2c1ec3e9de7d566facc8b234373631f67a
> src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImplModule.java 2ddd4f5c49b720fc0c92f4fdf0f299af5f8a3cdd
> src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java 7c8d2dfcecde120ff9e27879d3fe701f54ca79a4
> src/main/java/org/apache/aurora/scheduler/state/StateModule.java 46e9227b4c1574a9782098b27e2b560fab8978fd
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java a66d8049f513d9cdb4aa26d0286fce352977667a
> src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java f685d2e32215fcfb55deb5601ebae05262e93823
>
>
> Diff: https://reviews.apache.org/r/64934/diff/2/
>
>
> Testing
> -------
>
> End to end.
> Style check.
> Integration.
> Ran vagrant box, added `-thermos_executor_resources=""` created and killed a job.
>
>
> Thanks,
>
> Renan DelValle
>
>
Re: Review Request 64934: Custom converter to allow the
-thermos_executor_resources flag to take an empty string and parse it to an
empty list
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64934/#review195009
-----------------------------------------------------------
Ship it!
Master (8e5e08e) 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 Jan. 9, 2018, 1:22 a.m., Renan DelValle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64934/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2018, 1:22 a.m.)
>
>
> Review request for Aurora, Stephan Erb and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fixes the issue that caused the voting to fail for the 0.19.0 Aurora packages.
>
> Fix cribbed from: https://github.com/cbeust/jcommander/pull/422
>
> Implemented as a custom converter as suggested here: https://reviews.apache.org/r/64824/
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/app/AppModule.java 817a0193f6d32632f2dfd05373114da7a3885ed2
> src/main/java/org/apache/aurora/scheduler/config/splitters/CommaSplitter.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java 76ce39e837ea9dce81fdc66018611dfd11612388
> src/main/java/org/apache/aurora/scheduler/discovery/FlaggedZooKeeperConfig.java 694a4fc1b4a28dea391558424d33b5656cf8a74c
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java 9d757dbe98b11df5f7e81cd81c361d000e648620
> src/main/java/org/apache/aurora/scheduler/offers/OfferManagerModule.java 823c36949d0e93f8578534b1bf22edaf1cdc58ac
> src/main/java/org/apache/aurora/scheduler/preemptor/PreemptorModule.java 007ebc2c1ec3e9de7d566facc8b234373631f67a
> src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImplModule.java 2ddd4f5c49b720fc0c92f4fdf0f299af5f8a3cdd
> src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java 7c8d2dfcecde120ff9e27879d3fe701f54ca79a4
> src/main/java/org/apache/aurora/scheduler/state/StateModule.java 46e9227b4c1574a9782098b27e2b560fab8978fd
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java a66d8049f513d9cdb4aa26d0286fce352977667a
> src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java f685d2e32215fcfb55deb5601ebae05262e93823
>
>
> Diff: https://reviews.apache.org/r/64934/diff/2/
>
>
> Testing
> -------
>
> End to end.
> Style check.
> Integration.
> Ran vagrant box, added `-thermos_executor_resources=""` created and killed a job.
>
>
> Thanks,
>
> Renan DelValle
>
>
Re: Review Request 64934: Custom converter to allow the
-thermos_executor_resources flag to take an empty string and parse it to an
empty list
Posted by Renan DelValle <re...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64934/
-----------------------------------------------------------
(Updated Jan. 8, 2018, 5:22 p.m.)
Review request for Aurora, Stephan Erb and Bill Farner.
Changes
-------
Moved from IStringConverter to IStringSplitter in order to be used by all parameters that eventually return a list.
Changed all parameters that return a list to use CommaSplitter instead of CommaParameterSplitter (default).
CommaSplitter will now also return an empty list if single quotation marks with nothing in between is used ('').
Added tests to make sure that empty lists are returned when a parameter is used with no argument.
For -thermos_executor_resources added two more tests to make sure -thermos_executor_resources='' and -thermos_executor_resources="" both return an empty list.
Repository: aurora
Description
-------
Fixes the issue that caused the voting to fail for the 0.19.0 Aurora packages.
Fix cribbed from: https://github.com/cbeust/jcommander/pull/422
Implemented as a custom converter as suggested here: https://reviews.apache.org/r/64824/
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/app/AppModule.java 817a0193f6d32632f2dfd05373114da7a3885ed2
src/main/java/org/apache/aurora/scheduler/config/splitters/CommaSplitter.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java 76ce39e837ea9dce81fdc66018611dfd11612388
src/main/java/org/apache/aurora/scheduler/discovery/FlaggedZooKeeperConfig.java 694a4fc1b4a28dea391558424d33b5656cf8a74c
src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java 9d757dbe98b11df5f7e81cd81c361d000e648620
src/main/java/org/apache/aurora/scheduler/offers/OfferManagerModule.java 823c36949d0e93f8578534b1bf22edaf1cdc58ac
src/main/java/org/apache/aurora/scheduler/preemptor/PreemptorModule.java 007ebc2c1ec3e9de7d566facc8b234373631f67a
src/main/java/org/apache/aurora/scheduler/scheduling/TaskAssignerImplModule.java 2ddd4f5c49b720fc0c92f4fdf0f299af5f8a3cdd
src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java 7c8d2dfcecde120ff9e27879d3fe701f54ca79a4
src/main/java/org/apache/aurora/scheduler/state/StateModule.java 46e9227b4c1574a9782098b27e2b560fab8978fd
src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java a66d8049f513d9cdb4aa26d0286fce352977667a
src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java f685d2e32215fcfb55deb5601ebae05262e93823
Diff: https://reviews.apache.org/r/64934/diff/2/
Changes: https://reviews.apache.org/r/64934/diff/1-2/
Testing
-------
End to end.
Style check.
Integration.
Ran vagrant box, added `-thermos_executor_resources=""` created and killed a job.
Thanks,
Renan DelValle
Re: Review Request 64934: Custom converter to allow the
-thermos_executor_resources flag to take an empty string and parse it to an
empty list
Posted by Renan DelValle <re...@gmail.com>.
> On Jan. 4, 2018, 7 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
> > Lines 73 (patched)
> > <https://reviews.apache.org/r/64934/diff/1/?file=1930104#file1930104line73>
> >
> > This bug affects all `@Parameter(..) List<>` fields. Can you apply to them as well?
> >
> > This should identify all the List parameters:
> > ```
> > $ grep ImmutableList.of src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java
> > ```
Sure, working on this, hope to have it done by EOD today.
- Renan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64934/#review194826
-----------------------------------------------------------
On Jan. 3, 2018, 3:29 p.m., Renan DelValle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64934/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2018, 3:29 p.m.)
>
>
> Review request for Aurora, Stephan Erb and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fixes the issue that caused the voting to fail for the 0.19.0 Aurora packages.
>
> Fix cribbed from: https://github.com/cbeust/jcommander/pull/422
>
> Implemented as a custom converter as suggested here: https://reviews.apache.org/r/64824/
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/config/converters/StringListConverter.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java 76ce39e837ea9dce81fdc66018611dfd11612388
>
>
> Diff: https://reviews.apache.org/r/64934/diff/1/
>
>
> Testing
> -------
>
> End to end.
> Style check.
> Integration.
> Ran vagrant box, added `-thermos_executor_resources=""` created and killed a job.
>
>
> Thanks,
>
> Renan DelValle
>
>
Re: Review Request 64934: Custom converter to allow the
-thermos_executor_resources flag to take an empty string and parse it to an
empty list
Posted by Bill Farner <wf...@apache.org>.
> On Jan. 4, 2018, 7 p.m., Bill Farner wrote:
> >
Thanks for the patch!
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64934/#review194826
-----------------------------------------------------------
On Jan. 3, 2018, 3:29 p.m., Renan DelValle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64934/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2018, 3:29 p.m.)
>
>
> Review request for Aurora, Stephan Erb and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fixes the issue that caused the voting to fail for the 0.19.0 Aurora packages.
>
> Fix cribbed from: https://github.com/cbeust/jcommander/pull/422
>
> Implemented as a custom converter as suggested here: https://reviews.apache.org/r/64824/
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/config/converters/StringListConverter.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java 76ce39e837ea9dce81fdc66018611dfd11612388
>
>
> Diff: https://reviews.apache.org/r/64934/diff/1/
>
>
> Testing
> -------
>
> End to end.
> Style check.
> Integration.
> Ran vagrant box, added `-thermos_executor_resources=""` created and killed a job.
>
>
> Thanks,
>
> Renan DelValle
>
>
Re: Review Request 64934: Custom converter to allow the
-thermos_executor_resources flag to take an empty string and parse it to an
empty list
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64934/#review194826
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java
Lines 73 (patched)
<https://reviews.apache.org/r/64934/#comment273882>
This bug affects all `@Parameter(..) List<>` fields. Can you apply to them as well?
This should identify all the List parameters:
```
$ grep ImmutableList.of src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java
```
- Bill Farner
On Jan. 3, 2018, 3:29 p.m., Renan DelValle wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64934/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2018, 3:29 p.m.)
>
>
> Review request for Aurora, Stephan Erb and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Fixes the issue that caused the voting to fail for the 0.19.0 Aurora packages.
>
> Fix cribbed from: https://github.com/cbeust/jcommander/pull/422
>
> Implemented as a custom converter as suggested here: https://reviews.apache.org/r/64824/
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/config/converters/StringListConverter.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/configuration/executor/ExecutorModule.java 76ce39e837ea9dce81fdc66018611dfd11612388
>
>
> Diff: https://reviews.apache.org/r/64934/diff/1/
>
>
> Testing
> -------
>
> End to end.
> Style check.
> Integration.
> Ran vagrant box, added `-thermos_executor_resources=""` created and killed a job.
>
>
> Thanks,
>
> Renan DelValle
>
>