subject:"Review Request 52821\: Fix the \-enable_revocable

Re: Review Request 52821: Fix the -enable_revocable_ram flag

2016-10-17 Thread Aurora ReviewBot


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



Master (ac8b802) 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 Oct. 17, 2016, 9:52 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52821/
> ---
> 
> (Updated Oct. 17, 2016, 9:52 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1794
> https://issues.apache.org/jira/browse/AURORA-1794
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The mentioned flag has been introduced in 
> https://reviews.apache.org/r/51807/. Unfortunately, as detailed in the bug 
> report, my testing was not thorough enough. 
> 
> Problem description:
> 
> * The flag is used the `ResourceType` enum constructor. This implies the flag 
> value needs to be available during class loading.
> * Values supplied via the scheduler command line are only set at runtime, 
> right at the beginning `main` [1].
> * Luckily, there is a check in our arg parsing library that warns if a value 
> is changed after it has already been read. In other words: We get an 
> exception if we change the flag, because it has already been read during 
> class loading.
> 
> This patch corrects this issue by treating the arguments as a supplier which 
> can be read lazily at runtime. The patch also extends the existing e2e test 
> for revocable resources to also consider RAM.
> 
> [1] 
> https://github.com/apache/aurora/blob/b417be38fe1fcae6b85f7e91cea961ab272adf3f/src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java#L197
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 368f91720e9c8f84e139538a020da577e637851b 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 8e915c6233428ca35d3ee11ea8d7b7c008a88568 
>   examples/vagrant/mesos_config/etc_mesos-slave/modules 
> 4352bcad8022e9a82fe1e13744a02d1105e52fe9 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 4d1f0c6a53e995f845512b64b4f78e0a5a72 
>   src/main/java/org/apache/aurora/scheduler/resources/ResourceSettings.java 
> c49fd06cd11f4269b5c0b25014d22707c513fa5d 
>   src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java 
> e1a5dcef726f021df89bccb7e81e63fe60336009 
> 
> Diff: https://reviews.apache.org/r/52821/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 52821: Fix the -enable_revocable_ram flag

2016-10-17 Thread Stephan Erb

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

(Updated Oct. 17, 2016, 11:52 p.m.)

Review request for Aurora, Joshua Cohen and Zameer Manji.

Changes
---

Rebase.

Bugs: AURORA-1794
https://issues.apache.org/jira/browse/AURORA-1794

Repository: aurora

Description
---

The mentioned flag has been introduced in https://reviews.apache.org/r/51807/.
Unfortunately, as detailed in the bug report, my testing was not thorough
enough.

Problem description:

* The flag is used the `ResourceType` enum constructor. This implies the flag
value needs to be available during class loading.
* Values supplied via the scheduler command line are only set at runtime, right
at the beginning `main` [1].
* Luckily, there is a check in our arg parsing library that warns if a value is
changed after it has already been read. In other words: We get an exception if
we change the flag, because it has already been read during class loading.

This patch corrects this issue by treating the arguments as a supplier which
can be read lazily at runtime. The patch also extends the existing e2e test for
revocable resources to also consider RAM.

[1]
https://github.com/apache/aurora/blob/b417be38fe1fcae6b85f7e91cea961ab272adf3f/src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java#L197

Diffs (updated)
-

RELEASE-NOTES.md 368f91720e9c8f84e139538a020da577e637851b
commons-args/src/main/java/org/apache/aurora/common/args/Arg.java
8e915c6233428ca35d3ee11ea8d7b7c008a88568
examples/vagrant/mesos_config/etc_mesos-slave/modules
4352bcad8022e9a82fe1e13744a02d1105e52fe9
examples/vagrant/upstart/aurora-scheduler.conf
4d1f0c6a53e995f845512b64b4f78e0a5a72
src/main/java/org/apache/aurora/scheduler/resources/ResourceSettings.java
c49fd06cd11f4269b5c0b25014d22707c513fa5d
src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java
e1a5dcef726f021df89bccb7e81e63fe60336009

Diff: https://reviews.apache.org/r/52821/diff/

Testing
---

./gradlew -Pq build
./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh

Thanks,

Stephan Erb

Re: Review Request 52821: Fix the -enable_revocable_ram flag

2016-10-17 Thread Aurora ReviewBot


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



This patch does not apply cleanly against master (ac8b802), do you need to 
rebase?

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

- Aurora ReviewBot


On Oct. 17, 2016, 9:24 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52821/
> ---
> 
> (Updated Oct. 17, 2016, 9:24 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1794
> https://issues.apache.org/jira/browse/AURORA-1794
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The mentioned flag has been introduced in 
> https://reviews.apache.org/r/51807/. Unfortunately, as detailed in the bug 
> report, my testing was not thorough enough. 
> 
> Problem description:
> 
> * The flag is used the `ResourceType` enum constructor. This implies the flag 
> value needs to be available during class loading.
> * Values supplied via the scheduler command line are only set at runtime, 
> right at the beginning `main` [1].
> * Luckily, there is a check in our arg parsing library that warns if a value 
> is changed after it has already been read. In other words: We get an 
> exception if we change the flag, because it has already been read during 
> class loading.
> 
> This patch corrects this issue by treating the arguments as a supplier which 
> can be read lazily at runtime. The patch also extends the existing e2e test 
> for revocable resources to also consider RAM.
> 
> [1] 
> https://github.com/apache/aurora/blob/b417be38fe1fcae6b85f7e91cea961ab272adf3f/src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java#L197
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 03349b0b687bade48a2d19712dcb63802f3ef273 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 8e915c6233428ca35d3ee11ea8d7b7c008a88568 
>   examples/vagrant/mesos_config/etc_mesos-slave/modules 
> 4352bcad8022e9a82fe1e13744a02d1105e52fe9 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 4d1f0c6a53e995f845512b64b4f78e0a5a72 
>   src/main/java/org/apache/aurora/scheduler/resources/ResourceSettings.java 
> c49fd06cd11f4269b5c0b25014d22707c513fa5d 
>   src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java 
> e1a5dcef726f021df89bccb7e81e63fe60336009 
> 
> Diff: https://reviews.apache.org/r/52821/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 52821: Fix the -enable_revocable_ram flag

2016-10-17 Thread Stephan Erb

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

(Updated Oct. 17, 2016, 11:24 p.m.)

Review request for Aurora, Joshua Cohen and Zameer Manji.

Changes
---

Rename FALSE_SUPPLIER to NOT_REVOCABLE.

Bugs: AURORA-1794
https://issues.apache.org/jira/browse/AURORA-1794

Repository: aurora

Description
---

The mentioned flag has been introduced in https://reviews.apache.org/r/51807/.
Unfortunately, as detailed in the bug report, my testing was not thorough
enough.

Problem description:

This patch corrects this issue by treating the arguments as a supplier which
can be read lazily at runtime. The patch also extends the existing e2e test for
revocable resources to also consider RAM.

[1]
https://github.com/apache/aurora/blob/b417be38fe1fcae6b85f7e91cea961ab272adf3f/src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java#L197

Diffs (updated)
-

RELEASE-NOTES.md 03349b0b687bade48a2d19712dcb63802f3ef273
commons-args/src/main/java/org/apache/aurora/common/args/Arg.java
8e915c6233428ca35d3ee11ea8d7b7c008a88568
examples/vagrant/mesos_config/etc_mesos-slave/modules
4352bcad8022e9a82fe1e13744a02d1105e52fe9
examples/vagrant/upstart/aurora-scheduler.conf
4d1f0c6a53e995f845512b64b4f78e0a5a72
src/main/java/org/apache/aurora/scheduler/resources/ResourceSettings.java
c49fd06cd11f4269b5c0b25014d22707c513fa5d
src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java
e1a5dcef726f021df89bccb7e81e63fe60336009

Diff: https://reviews.apache.org/r/52821/diff/

Testing
---

./gradlew -Pq build
./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh

Thanks,

Stephan Erb

Re: Review Request 52821: Fix the -enable_revocable_ram flag

2016-10-17 Thread Zameer Manji


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


Ship it!




Ship It!

- Zameer Manji


On Oct. 13, 2016, 2:27 a.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52821/
> ---
> 
> (Updated Oct. 13, 2016, 2:27 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1794
> https://issues.apache.org/jira/browse/AURORA-1794
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The mentioned flag has been introduced in 
> https://reviews.apache.org/r/51807/. Unfortunately, as detailed in the bug 
> report, my testing was not thorough enough. 
> 
> Problem description:
> 
> * The flag is used the `ResourceType` enum constructor. This implies the flag 
> value needs to be available during class loading.
> * Values supplied via the scheduler command line are only set at runtime, 
> right at the beginning `main` [1].
> * Luckily, there is a check in our arg parsing library that warns if a value 
> is changed after it has already been read. In other words: We get an 
> exception if we change the flag, because it has already been read during 
> class loading.
> 
> This patch corrects this issue by treating the arguments as a supplier which 
> can be read lazily at runtime. The patch also extends the existing e2e test 
> for revocable resources to also consider RAM.
> 
> [1] 
> https://github.com/apache/aurora/blob/b417be38fe1fcae6b85f7e91cea961ab272adf3f/src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java#L197
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 03349b0b687bade48a2d19712dcb63802f3ef273 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 8e915c6233428ca35d3ee11ea8d7b7c008a88568 
>   examples/vagrant/mesos_config/etc_mesos-slave/modules 
> 4352bcad8022e9a82fe1e13744a02d1105e52fe9 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 4d1f0c6a53e995f845512b64b4f78e0a5a72 
>   src/main/java/org/apache/aurora/scheduler/resources/ResourceSettings.java 
> c49fd06cd11f4269b5c0b25014d22707c513fa5d 
>   src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java 
> e1a5dcef726f021df89bccb7e81e63fe60336009 
> 
> Diff: https://reviews.apache.org/r/52821/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 52821: Fix the -enable_revocable_ram flag

2016-10-17 Thread Joshua Cohen


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


Ship it!




Thanks for clarifying re: calling the supplier in the constructor vs getter.


src/main/java/org/apache/aurora/scheduler/resources/ResourceSettings.java (line 
36)


Maybe call this `NOT_REVOCABLE` so it's usage is clear at the call site?


- Joshua Cohen


On Oct. 13, 2016, 9:27 a.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52821/
> ---
> 
> (Updated Oct. 13, 2016, 9:27 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1794
> https://issues.apache.org/jira/browse/AURORA-1794
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The mentioned flag has been introduced in 
> https://reviews.apache.org/r/51807/. Unfortunately, as detailed in the bug 
> report, my testing was not thorough enough. 
> 
> Problem description:
> 
> * The flag is used the `ResourceType` enum constructor. This implies the flag 
> value needs to be available during class loading.
> * Values supplied via the scheduler command line are only set at runtime, 
> right at the beginning `main` [1].
> * Luckily, there is a check in our arg parsing library that warns if a value 
> is changed after it has already been read. In other words: We get an 
> exception if we change the flag, because it has already been read during 
> class loading.
> 
> This patch corrects this issue by treating the arguments as a supplier which 
> can be read lazily at runtime. The patch also extends the existing e2e test 
> for revocable resources to also consider RAM.
> 
> [1] 
> https://github.com/apache/aurora/blob/b417be38fe1fcae6b85f7e91cea961ab272adf3f/src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java#L197
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 03349b0b687bade48a2d19712dcb63802f3ef273 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 8e915c6233428ca35d3ee11ea8d7b7c008a88568 
>   examples/vagrant/mesos_config/etc_mesos-slave/modules 
> 4352bcad8022e9a82fe1e13744a02d1105e52fe9 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 4d1f0c6a53e995f845512b64b4f78e0a5a72 
>   src/main/java/org/apache/aurora/scheduler/resources/ResourceSettings.java 
> c49fd06cd11f4269b5c0b25014d22707c513fa5d 
>   src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java 
> e1a5dcef726f021df89bccb7e81e63fe60336009 
> 
> Diff: https://reviews.apache.org/r/52821/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 52821: Fix the -enable_revocable_ram flag

2016-10-17 Thread Stephan Erb

> On Oct. 13, 2016, 10:24 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java, line 
> > 215
> > 
> >
> > Is there a reason we can't apply the `get` call here rather than in the 
> > getter?
> 
> Zameer Manji wrote:
> +1

The constructor is invoked when the Enum class is first initialized. All 
instances of the Enum are created before any may be used. This implies that the 
`get()` would be called before the command line arguments are loaded.

Therefore, when changing the code as proposed, the scheduler does not start. It 
fails with:

```
Oct 17, 2016 2:49:42 PM org.apache.aurora.common.args.apt.Configuration load
INFO: Loading @CmdLine config from: 
[jar:file:/home/vagrant/aurora/dist/install/aurora-scheduler/lib/commons-0.17.0-SNAPSHOT.jar!/org/apache/aurora/common/args/apt/cmdline.arg.info.txt.1,

jar:file:/home/vagrant/aurora/dist/install/aurora-scheduler/lib/aurora-0.17.0-SNAPSHOT.jar!/org/apache/aurora/common/args/apt/cmdline.arg.info.txt.2]
I1017 14:49:42.684 [main, Log:192] Logging initialized @1268ms
W1017 14:49:43.085 [main, ArgScanner:306] Found argument name collisions, args 
must be referenced by canonical names: [slow_query_log_threshold]
Exception in thread "main" java.lang.IllegalStateException: A value cannot be 
changed after it was read.
at 
com.google.common.base.Preconditions.checkState(Preconditions.java:174)
at org.apache.aurora.common.args.Arg.set(Arg.java:55)
at 
org.apache.aurora.common.args.ArgumentInfo.setValue(ArgumentInfo.java:128)
at org.apache.aurora.common.args.OptionInfo.load(OptionInfo.java:131)
at org.apache.aurora.common.args.ArgScanner.process(ArgScanner.java:368)
at org.apache.aurora.common.args.ArgScanner.parse(ArgScanner.java:200)
at org.apache.aurora.common.args.ArgScanner.parse(ArgScanner.java:178)
at org.apache.aurora.common.args.ArgScanner.parse(ArgScanner.java:155)
at 
org.apache.aurora.scheduler.app.SchedulerMain.applyStaticArgumentValues(SchedulerMain.java:226)
at 
org.apache.aurora.scheduler.app.SchedulerMain.main(SchedulerMain.java:197)

```

By moving the `get()` to the getter we can work around this limitation.

- Stephan

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

On Oct. 13, 2016, 11:27 a.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52821/
> ---
> 
> (Updated Oct. 13, 2016, 11:27 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1794
> https://issues.apache.org/jira/browse/AURORA-1794
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The mentioned flag has been introduced in 
> https://reviews.apache.org/r/51807/. Unfortunately, as detailed in the bug 
> report, my testing was not thorough enough. 
> 
> Problem description:
> 
> * The flag is used the `ResourceType` enum constructor. This implies the flag 
> value needs to be available during class loading.
> * Values supplied via the scheduler command line are only set at runtime, 
> right at the beginning `main` [1].
> * Luckily, there is a check in our arg parsing library that warns if a value 
> is changed after it has already been read. In other words: We get an 
> exception if we change the flag, because it has already been read during 
> class loading.
> 
> This patch corrects this issue by treating the arguments as a supplier which 
> can be read lazily at runtime. The patch also extends the existing e2e test 
> for revocable resources to also consider RAM.
> 
> [1] 
> https://github.com/apache/aurora/blob/b417be38fe1fcae6b85f7e91cea961ab272adf3f/src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java#L197
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 03349b0b687bade48a2d19712dcb63802f3ef273 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 8e915c6233428ca35d3ee11ea8d7b7c008a88568 
>   examples/vagrant/mesos_config/etc_mesos-slave/modules 
> 4352bcad8022e9a82fe1e13744a02d1105e52fe9 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 4d1f0c6a53e995f845512b64b4f78e0a5a72 
>   src/main/java/org/apache/aurora/scheduler/resources/ResourceSettings.java 
> c49fd06cd11f4269b5c0b25014d22707c513fa5d 
>   src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java 
> e1a5dcef726f021df89bccb7e81e63fe60336009 
> 
> Diff: https://reviews.apache.org/r/52821/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 52821: Fix the -enable_revocable_ram flag

2016-10-13 Thread Zameer Manji



> On Oct. 13, 2016, 1:24 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java, line 
> > 215
> > 
> >
> > Is there a reason we can't apply the `get` call here rather than in the 
> > getter?

+1


- Zameer


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


On Oct. 13, 2016, 2:27 a.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52821/
> ---
> 
> (Updated Oct. 13, 2016, 2:27 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1794
> https://issues.apache.org/jira/browse/AURORA-1794
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The mentioned flag has been introduced in 
> https://reviews.apache.org/r/51807/. Unfortunately, as detailed in the bug 
> report, my testing was not thorough enough. 
> 
> Problem description:
> 
> * The flag is used the `ResourceType` enum constructor. This implies the flag 
> value needs to be available during class loading.
> * Values supplied via the scheduler command line are only set at runtime, 
> right at the beginning `main` [1].
> * Luckily, there is a check in our arg parsing library that warns if a value 
> is changed after it has already been read. In other words: We get an 
> exception if we change the flag, because it has already been read during 
> class loading.
> 
> This patch corrects this issue by treating the arguments as a supplier which 
> can be read lazily at runtime. The patch also extends the existing e2e test 
> for revocable resources to also consider RAM.
> 
> [1] 
> https://github.com/apache/aurora/blob/b417be38fe1fcae6b85f7e91cea961ab272adf3f/src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java#L197
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 03349b0b687bade48a2d19712dcb63802f3ef273 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 8e915c6233428ca35d3ee11ea8d7b7c008a88568 
>   examples/vagrant/mesos_config/etc_mesos-slave/modules 
> 4352bcad8022e9a82fe1e13744a02d1105e52fe9 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 4d1f0c6a53e995f845512b64b4f78e0a5a72 
>   src/main/java/org/apache/aurora/scheduler/resources/ResourceSettings.java 
> c49fd06cd11f4269b5c0b25014d22707c513fa5d 
>   src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java 
> e1a5dcef726f021df89bccb7e81e63fe60336009 
> 
> Diff: https://reviews.apache.org/r/52821/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 52821: Fix the -enable_revocable_ram flag

2016-10-13 Thread Joshua Cohen


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




src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java (line 215)


Is there a reason we can't apply the `get` call here rather than in the 
getter?


- Joshua Cohen


On Oct. 13, 2016, 9:27 a.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52821/
> ---
> 
> (Updated Oct. 13, 2016, 9:27 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1794
> https://issues.apache.org/jira/browse/AURORA-1794
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The mentioned flag has been introduced in 
> https://reviews.apache.org/r/51807/. Unfortunately, as detailed in the bug 
> report, my testing was not thorough enough. 
> 
> Problem description:
> 
> * The flag is used the `ResourceType` enum constructor. This implies the flag 
> value needs to be available during class loading.
> * Values supplied via the scheduler command line are only set at runtime, 
> right at the beginning `main` [1].
> * Luckily, there is a check in our arg parsing library that warns if a value 
> is changed after it has already been read. In other words: We get an 
> exception if we change the flag, because it has already been read during 
> class loading.
> 
> This patch corrects this issue by treating the arguments as a supplier which 
> can be read lazily at runtime. The patch also extends the existing e2e test 
> for revocable resources to also consider RAM.
> 
> [1] 
> https://github.com/apache/aurora/blob/b417be38fe1fcae6b85f7e91cea961ab272adf3f/src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java#L197
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 03349b0b687bade48a2d19712dcb63802f3ef273 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 8e915c6233428ca35d3ee11ea8d7b7c008a88568 
>   examples/vagrant/mesos_config/etc_mesos-slave/modules 
> 4352bcad8022e9a82fe1e13744a02d1105e52fe9 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 4d1f0c6a53e995f845512b64b4f78e0a5a72 
>   src/main/java/org/apache/aurora/scheduler/resources/ResourceSettings.java 
> c49fd06cd11f4269b5c0b25014d22707c513fa5d 
>   src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java 
> e1a5dcef726f021df89bccb7e81e63fe60336009 
> 
> Diff: https://reviews.apache.org/r/52821/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 52821: Fix the -enable_revocable_ram flag

2016-10-13 Thread Aurora ReviewBot


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



Master (8256000) 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 Oct. 13, 2016, 9:27 a.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52821/
> ---
> 
> (Updated Oct. 13, 2016, 9:27 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Zameer Manji.
> 
> 
> Bugs: AURORA-1794
> https://issues.apache.org/jira/browse/AURORA-1794
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> The mentioned flag has been introduced in 
> https://reviews.apache.org/r/51807/. Unfortunately, as detailed in the bug 
> report, my testing was not thorough enough. 
> 
> Problem description:
> 
> * The flag is used the `ResourceType` enum constructor. This implies the flag 
> value needs to be available during class loading.
> * Values supplied via the scheduler command line are only set at runtime, 
> right at the beginning `main` [1].
> * Luckily, there is a check in our arg parsing library that warns if a value 
> is changed after it has already been read. In other words: We get an 
> exception if we change the flag, because it has already been read during 
> class loading.
> 
> This patch corrects this issue by treating the arguments as a supplier which 
> can be read lazily at runtime. The patch also extends the existing e2e test 
> for revocable resources to also consider RAM.
> 
> [1] 
> https://github.com/apache/aurora/blob/b417be38fe1fcae6b85f7e91cea961ab272adf3f/src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java#L197
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 03349b0b687bade48a2d19712dcb63802f3ef273 
>   commons-args/src/main/java/org/apache/aurora/common/args/Arg.java 
> 8e915c6233428ca35d3ee11ea8d7b7c008a88568 
>   examples/vagrant/mesos_config/etc_mesos-slave/modules 
> 4352bcad8022e9a82fe1e13744a02d1105e52fe9 
>   examples/vagrant/upstart/aurora-scheduler.conf 
> 4d1f0c6a53e995f845512b64b4f78e0a5a72 
>   src/main/java/org/apache/aurora/scheduler/resources/ResourceSettings.java 
> c49fd06cd11f4269b5c0b25014d22707c513fa5d 
>   src/main/java/org/apache/aurora/scheduler/resources/ResourceType.java 
> e1a5dcef726f021df89bccb7e81e63fe60336009 
> 
> Diff: https://reviews.apache.org/r/52821/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Review Request 52821: Fix the -enable_revocable_ram flag

2016-10-13 Thread Stephan Erb

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

Review request for Aurora, Joshua Cohen and Zameer Manji.

Bugs: AURORA-1794
https://issues.apache.org/jira/browse/AURORA-1794

Repository: aurora

Description
---

The mentioned flag has been introduced in https://reviews.apache.org/r/51807/.
Unfortunately, as detailed in the bug report, my testing was not thorough
enough.

Problem description:

This patch corrects this issue by treating the arguments as a supplier which
can be read lazily at runtime. The patch also extends the existing e2e test for
revocable resources to also consider RAM.

[1]
https://github.com/apache/aurora/blob/b417be38fe1fcae6b85f7e91cea961ab272adf3f/src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java#L197

Diffs
-

Diff: https://reviews.apache.org/r/52821/diff/

Testing
---

./gradlew -Pq build
./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh

Thanks,

Stephan Erb

Re: Review Request 52821: Fix the -enable_revocable_ram flag

Re: Review Request 52821: Fix the -enable_revocable_ram flag

Re: Review Request 52821: Fix the -enable_revocable_ram flag

Re: Review Request 52821: Fix the -enable_revocable_ram flag

Re: Review Request 52821: Fix the -enable_revocable_ram flag

Re: Review Request 52821: Fix the -enable_revocable_ram flag

Re: Review Request 52821: Fix the -enable_revocable_ram flag

Re: Review Request 52821: Fix the -enable_revocable_ram flag

Re: Review Request 52821: Fix the -enable_revocable_ram flag

Re: Review Request 52821: Fix the -enable_revocable_ram flag

Review Request 52821: Fix the -enable_revocable_ram flag

11 matches

Site Navigation

Mail list logo

Footer information