Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

[Aurora ReviewBot]

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

Ship it!


Master (952ef6d) 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. 13, 2016, 7:20 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 13, 2016, 7:20 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1576
> https://issues.apache.org/jira/browse/AURORA-1576
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow for plugging in cli-configurable filters that are invoked post shiro 
> filters.
> 
> 
> Diffs
> -
> 
>   NEWS ecb26d2af2c624042e7819acde004278520b3cae 
>   docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  ac92117e14c105bec74e49d8e80eb56008237abf 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $ ./gradlew clean test
> ...
> BUILD SUCCESSFUL
> 
> Total time: 3 mins 24.616 secs
> ```
> 
> End to end tests:
> ```
> $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ...
> 
> *** OK (All tests passed) ***
> 
> aurora-scheduler-kerberos stop/waiting
> aurora-scheduler start/running, process 15362
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> ```
> 
> 
> Thanks,
> 
> Amol Deshmukh
> 
>

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

[Aurora ReviewBot]

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

Ship it!


Master (952ef6d) 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. 13, 2016, 2:08 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 2:08 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new class OfferAllocation, which allcoates resources to 
> resources field in TaskInfo and ExecutorInfo from an offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> 1. Unit tested with old and new tests;
> 2. vagrant integration tests: I manually separate out the vagrant box's cpu 
> and memory between 'aurora-test' role and '*' and verified that jobs can 
> still be launched (I can post the vagrant change in another follow upon 
> request).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42177: Vagrant change to reserve part of the dev cluster's resources to 'aurora-role'

[Zhitao Li]

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

(Updated Jan. 12, 2016, 8:04 p.m.)


Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
Farner.


Changes
---

Added formal review dep to ensure we don't try to commit this before the 
multi-role support patch.

-wfarner


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


Repository: aurora


Description
---

This is how I tested changes in https://reviews.apache.org/r/42176/.


Diffs
-

  examples/vagrant/upstart/aurora-scheduler.conf 
4033184451f36cb5f0233ea96e3dceaae6741275 
  examples/vagrant/upstart/mesos-master.conf 
9d7491c08e14e3951b2fd2f74291a2009883c379 
  examples/vagrant/upstart/mesos-slave.conf 
1ef059b17d16d4f1594a19ff6422ea653a413359 

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


Testing
---

Integration test script.


Thanks,

Zhitao Li

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

[Amol Deshmukh]

> On Jan. 13, 2016, 1:31 a.m., Bill Farner wrote:
> > It sounds like you ran into an issue that necessitated this patch.  Is 
> > there anything you can add to `docs/security.md` so others don't get stuck 
> > down the same dead end?
> > 
> > Also, please add a NEWS entry summarizing the new field so we make sure to 
> > highlight it in the next release.

Ack.


> On Jan. 13, 2016, 1:31 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java,
> >  line 82
> > 
> >
> > How about `shiro_after_auth_filter`?  While more verbose, it avoids the 
> > term overload of `post` with HTTP POST request (which is what i actually 
> > thought this was for at first).  If you do rename, please apply it 
> > throughout the patch.

Ack.


> On Jan. 13, 2016, 1:31 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java,
> >  line 138
> > 
> >
> > Make this `Optional` to have a consistent API/internals, and change the 
> > `null` above to `Optional.empty()`

Using Optional for parameter types offers no benefit since it still doesn't 
prevent the caller from invoking it with a null value. i.e. the null check 
cannot be avoided (albeit with different consequences) when the parameter type 
is Optional.

FWIW, there is support for this rationale in Kevin Bourrillion's comment here: 
https://plus.google.com/+JordanLewiser/posts/ayfR8F56PGy


> On Jan. 13, 2016, 1:31 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java,
> >  line 209
> > 
> >
> > Usually we try to minimize the scope of suppressions to prevent 
> > unexpectedly covering real warnings.  Consider extracting a function that 
> > covers only the line requiring the suppression.  This obviates the need for 
> > the comment `// For xyz` and prevents issues later on.

Ack.


> On Jan. 13, 2016, 1:31 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java,
> >  line 118
> > 
> >
> > Drop the `maybe`, the `Optional` type is sufficient to indicate the 
> > possibly-absent value.
> > 
> > Also, you should generally prefer to accept `Key` instead of `Class` 
> > for APIs relating to identifiers for guice bindings. So in this case, use 
> > `Key` everywhere except the `Arg` as that's more general 
> > and has parity with the `ShiroWebModule` API.  Additionally, you should 
> > remove the line `bind(postFilter).in(Singleton.class);`, and require that 
> > the filter be externally bound.  This gives the caller greater flexibility 
> > when it comes to defining the binding.

Thanks for the guidance here.


> On Jan. 13, 2016, 1:31 a.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java,
> >  line 115
> > 
> >
> > Can you approach this with a mock?  Seems like it would be more direct 
> > and less brittle way to verify calls.  With the suggestion above to accept 
> > `Key` and avoid the internal `bind()`, this should become much more doable 
> > than with the current API.

Yes, removing the ``bind(...)`` makes this possible. Thanks!


- Amol


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


On Jan. 12, 2016, 6:52 p.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 12, 2016, 6:52 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1576
> https://issues.apache.org/jira/browse/AURORA-1576
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow for plugging in cli-configurable filters that are invoked post shiro 
> filters.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> 19c8a1fe06a24022da11f74d7c96b2942587 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  ac92117e14c105bec74e49d8e80eb56008237abf 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $ 

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

[Amol Deshmukh]

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

(Updated Jan. 13, 2016, 7:20 a.m.)


Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.


Changes
---

* Addressed wfarner's review comments


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


Repository: aurora


Description
---

Allow for plugging in cli-configurable filters that are invoked post shiro 
filters.


Diffs (updated)
-

  NEWS ecb26d2af2c624042e7819acde004278520b3cae 
  docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
  
src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
 4b6a872737e5934394e9511af7b6bd96c6034e72 
  
src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java 
ac92117e14c105bec74e49d8e80eb56008237abf 

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


Testing
---

Unit tests:
```
$ ./gradlew clean test
...
BUILD SUCCESSFUL

Total time: 3 mins 24.616 secs
```

End to end tests:
```
$ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
...

*** OK (All tests passed) ***

aurora-scheduler-kerberos stop/waiting
aurora-scheduler start/running, process 15362
+ RETCODE=0
+ restore_netrc
+ mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
+ true
Connection to 127.0.0.1 closed.

```


Thanks,

Amol Deshmukh

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

[Bill Farner]

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


Thanks for the changes so far, nearly there!


src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 39)


I did a quick rearrangement of this method (after applying the comment 
below), i think the result is a bit easier to read, and uses fewer temp 
variables.

I also cheated and omitted the `requireNonNull` calls as nulls will 
immediately trigger NPE for all parameters.

```
  public static AcceptedOffer create(
  Offer offer,
  ResourceSlot taskSlot,
  ResourceSlot executorSlot,
  Set selectedPorts,
  TierInfo tierInfo) throws Resources.InsufficientResourcesException {

List reservedFirst = ImmutableList.builder()
.addAll(Iterables.filter(offer.getResourcesList(), RESERVED))
.addAll(Iterables.filter(offer.getResourcesList(), NOT_RESERVED))
.build();

boolean revocable = tierInfo.isRevocable();
List cpuResources = filterToBuilders(
reservedFirst,
ResourceType.CPUS.getName(),
revocable ? Resources.REVOCABLE : Resources.NON_REVOCABLE);
List memResources = filterToBuilderNonRevocable(
reservedFirst, ResourceType.RAM_MB.getName());
List diskResources = filterToBuilderNonRevocable(
reservedFirst, ResourceType.DISK_MB.getName());
List portsResources = filterToBuilderNonRevocable(
reservedFirst, ResourceType.PORTS.getName());

List taskResources = ImmutableList.builder()
.addAll(allocateScalarType(cpuResources, taskSlot.getNumCpus(), 
revocable))
.addAll(allocateScalarType(memResources, 
taskSlot.getRam().as(Data.MB), false))
.addAll(allocateScalarType(diskResources, 
taskSlot.getDisk().as(Data.MB), false))
.addAll(allocateRangeType(portsResources, selectedPorts))
.build();

List executorResources = ImmutableList.builder()
.addAll(allocateScalarType(cpuResources, executorSlot.getNumCpus(), 
revocable))
.addAll(allocateScalarType(memResources, 
executorSlot.getRam().as(Data.MB), false))
.addAll(allocateScalarType(diskResources, 
executorSlot.getDisk().as(Data.MB), false))
.build();

return new AcceptedOffer(taskResources, executorResources);
  }
```



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 119)


feel free to declare as `List` instead of `ImmutableList`, ditto in the 
constructor.



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 129)


This is a good general-purpose implementation of this behavior, but in 
practice it seems like overkill.  Can you instead match the behavior currently 
on master and synthesize the `Resource` with `makeMesosRangeResource(PORTS, 
selectedPorts)`?



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 177)


Remove `final` here and throughout the method.  We only use `final` when 
necessary, or to ensure immutability of class field references.



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 178)


This parameter is now effectively a return value, so instead make it a 
formal return type.



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 181)


`remaining` instead of `outstanding`



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 199)


Move this up to the line after the declaration of `used` to make for easier 
readability of the accounting logic.



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 200)


`r.getScalarBuilder().setValue(available - used);`



src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java (line 154)


`runMultipleRolesTest` to make it clear this is not intended to be a test 
case



src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java (line 253)


You could simplify this a bit for callers by changing `Set... 
values` to `Integer... values`.  This way callers don't need the 
`ImmutableSet.of()` boilerplate.  It also resolves the unchecked generic vararg 
compiler warning you have now.

On this end, you will remove the `for` loop and do:
 

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Bill Farner]

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



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 40)


The interface should really be removed.  It's not acting as an interface 
hiding an implementation, so its value is not worth its weight.  Everything 
else can remain as-is (e.g. static factory function).



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 62)


Please move ~all of this method body into `create` to avoid doing 
significant work in the constructor.

Background on this convention can be found here if you're interested: 
http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/

The constructor should whittle down to something as basic as:

```
AcceptedOffer(ImmutableList taskResources, 
ImmutableList executorResources) {
  this.taskResources = requireNonNull(taskResources);
  this.executorResources = requireNonNull(executorResources);
}
```



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (lines 76 - 97)


A few thoughts for this code block:
1. rather than sort, filter reserved resources and arrange them first.  the 
sort is extra work, and currently defines an arbitrary sub-sort on role name 
which is misleading at best.
2. convert to List upfront and use these to track the 
remaining capacity of the resource by mutating them

If you agree, you would start this block with something like
```
List reservedFirst = ImmutableList.builder()
  .addAll(Iterables.filter(offer.getResourcesList(), RESERVED)
  .addAll(Iterables.filter(offer.getResourcesList(), NOT_RESERVED)
  .build()

List availableResources = 
FluentIterable.from(reservedFirst)
.transform(Resource::toBuilder)
.toList();
```

This will also allow you to avoid modifying the `List` itself (as is 
currently done below with `ListIterator.remove/add`) since you can adjust the 
remaining capacity in `Resource.Builder` instances rather than constructing new 
ones.



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 110)


I'd like to strive to remove `@VisibleForTesting` from all methods in this 
class and make them `private`.  It will likely require more fixtures/helpers in 
the unit test, but the result will be a unit test that is not so highly coupled 
to the implementation.  Do you see anything preventing this approach?



src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java (line 199)


I now see why you do indeed need _some_ flag for `revocable` because both 
`REVOCABLE` and `NON_REVOCABLE` filters are active (neither are a no-op).  I 
suggest that you accept `Predicate` and introduce an overload to make 
the call sites more readable (`false` does not indicate much)

```
static List filterNonRevocableAndSort(List resources, 
String name) {
  return filterAndSort(
  resources,
  name,
  revocable ? Resources.REVOCABLE : Resources.NON_REVOCABLE);
}

static List filterAndSort(
List resources,
String name,
Predicate additionalFilter) {

  return FluentIterable.from(resources)
  .filter(e -> e.getName().equals(name))
  .filter(additionalFilter)
  .toSortedList(PREFER_RESERVED);
}
```

Then most call sites become `filterNonRevocableAndSort`, and the call for 
CPU passes `Resources.REVOCABLE`.  Does that make sense?


- Bill Farner


On Jan. 11, 2016, 6:29 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 11, 2016, 6:29 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new interface OfferAllocation (with implementation), which 
> allcoates resources to resources field in TaskInfo and ExecutorInfo from an 
> offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present. This can be  customized with a differnet 
> comparator from PREFER_RESERVED in the future when needed.
> 
> Several caveats:
> 1. This 

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

[Amol Deshmukh]

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

(Updated Jan. 12, 2016, 6:52 p.m.)


Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.


Changes
---

- Changed multiline comments to use // instead of /* ... */


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


Repository: aurora


Description
---

Allow for plugging in cli-configurable filters that are invoked post shiro 
filters.


Diffs (updated)
-

  
src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
 4b6a872737e5934394e9511af7b6bd96c6034e72 
  src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
19c8a1fe06a24022da11f74d7c96b2942587 
  
src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java 
ac92117e14c105bec74e49d8e80eb56008237abf 

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


Testing
---

Unit tests:
```
$ ./gradlew clean test
...
BUILD SUCCESSFUL

Total time: 3 mins 24.616 secs
```

End to end tests:
```
$ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
...

*** OK (All tests passed) ***

aurora-scheduler-kerberos stop/waiting
aurora-scheduler start/running, process 15362
+ RETCODE=0
+ restore_netrc
+ mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
+ true
Connection to 127.0.0.1 closed.

```


Thanks,

Amol Deshmukh

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

[Amol Deshmukh]

> On Jan. 12, 2016, 5:41 p.m., Joshua Cohen wrote:
> > src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java,
> >  line 126
> > 
> >
> > Do we need to use a `StatsProvider` for this? Can we just expose call 
> > counts from this class directly? Are there even threading concerns that 
> > require atomicity?
> 
> Amol Deshmukh wrote:
> I debated about the alternative - that would require making the injector 
> protected and getting the filter instance from the injector in 
> ``HttpSecurityIT``. I don't have a strong preference -- I can update the 
> review with that change.

I spoke too soon - the filter is bound inside ``ShiroWebModule`` which being a 
``PrivateModule`` doesn't allow access to the filter instance. So I think I'm 
hitting a wall with the alternate approach.


- Amol


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


On Jan. 12, 2016, 1:15 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 12, 2016, 1:15 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1576
> https://issues.apache.org/jira/browse/AURORA-1576
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow for plugging in cli-configurable filters that are invoked post shiro 
> filters.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> 19c8a1fe06a24022da11f74d7c96b2942587 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  ac92117e14c105bec74e49d8e80eb56008237abf 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $ ./gradlew clean test
> ...
> BUILD SUCCESSFUL
> 
> Total time: 3 mins 24.616 secs
> ```
> 
> End to end tests:
> ```
> $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ...
> 
> *** OK (All tests passed) ***
> 
> aurora-scheduler-kerberos stop/waiting
> aurora-scheduler start/running, process 15362
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> ```
> 
> 
> Thanks,
> 
> Amol Deshmukh
> 
>

Re: Review Request 42077: Introduces -default_docker_parameters scheduler flag.

[Bill Farner]

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


I'm good for a ship it once a NEWS entry is added.


docs/deploying-aurora-scheduler.md (line 183)


s/be use/be used/


- Bill Farner


On Jan. 11, 2016, 11:32 a.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42077/
> ---
> 
> (Updated Jan. 11, 2016, 11:32 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1575
> https://issues.apache.org/jira/browse/AURORA-1575
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This flag allows cluster administrators to set arbitrary
> Docker parameters which will apply to all jobs.
> 
> Also cleans up some of the existing unit tests around task config.
> 
> 
> Diffs
> -
> 
>   README.md f3b23247cf3f7e550c1714b4fb8227a2baab3b41 
>   
> commons/src/main/java/org/apache/aurora/common/args/parsers/MultimapParser.java
>  PRE-CREATION 
>   docs/deploying-aurora-scheduler.md 8a1e68e5d54e9b8b66139bfc731563668584fa77 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 4eee8e31a4ccd25ba4a5bcb60b67c79979c3b9b0 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> d7f3c60d383cf10afb0c0fcf4fe29972b183458c 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  05e8b714043dea89039ce9a1fc4b32c65ab15fe4 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  f3b62cc957186bc9673060830572bc1cc073ac49 
> 
> Diff: https://reviews.apache.org/r/42077/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

[Amol Deshmukh]

> On Jan. 12, 2016, 5:41 p.m., Joshua Cohen wrote:
> > It would've been nice if, by default, we did not wire up the post-filter at 
> > all, and only asserted its counters when it has been wired in, but from a 
> > quick glance at the way the tests are set up, it doesn't seem to be 
> > possible to conditionally configure the `HttpSecurityModule`.

I could set this up as a ``Parameterized`` test and parameterize the filter 
inclusion. However, given that the filter is pass through and does not add any 
behavior at all, it seems like overkill.


> On Jan. 12, 2016, 5:41 p.m., Joshua Cohen wrote:
> > src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java,
> >  line 126
> > 
> >
> > Do we need to use a `StatsProvider` for this? Can we just expose call 
> > counts from this class directly? Are there even threading concerns that 
> > require atomicity?

I debated about the alternative - that would require making the injector 
protected and getting the filter instance from the injector in 
``HttpSecurityIT``. I don't have a strong preference -- I can update the review 
with that change.


> On Jan. 12, 2016, 5:41 p.m., Joshua Cohen wrote:
> > src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java,
> >  lines 156-159
> > 
> >
> > Nit, just use `//` for multi-line comments.

Ack.


> On Jan. 12, 2016, 5:41 p.m., Joshua Cohen wrote:
> > src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java,
> >  lines 174-179
> > 
> >
> > Does this need to be here? Can this just be done in the downstream 
> > user-defined filter instead?

Unfortunately, this appears to be the only reasonable place to override the 
binding since ``ShiroWebModule`` is a ``PrivateModule``.

The ``bindSessionManager`` method is offered as an extension point specifically 
for this purpose per javadocs on ``ShiroWebModule``: 
https://shiro.apache.org/static/1.2.3/apidocs/src-html/org/apache/shiro/guice/web/ShiroWebModule.html#line.190
 (static links to shiro v1.2.4 source are broken at the moment, but there is no 
change wrt this method in shiro 1.2.4)


- Amol


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


On Jan. 12, 2016, 1:15 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 12, 2016, 1:15 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1576
> https://issues.apache.org/jira/browse/AURORA-1576
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow for plugging in cli-configurable filters that are invoked post shiro 
> filters.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> 19c8a1fe06a24022da11f74d7c96b2942587 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  ac92117e14c105bec74e49d8e80eb56008237abf 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $ ./gradlew clean test
> ...
> BUILD SUCCESSFUL
> 
> Total time: 3 mins 24.616 secs
> ```
> 
> End to end tests:
> ```
> $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ...
> 
> *** OK (All tests passed) ***
> 
> aurora-scheduler-kerberos stop/waiting
> aurora-scheduler start/running, process 15362
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> ```
> 
> 
> Thanks,
> 
> Amol Deshmukh
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

[Aurora ReviewBot]

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

Ship it!


Master (d542bd1) 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. 12, 2016, 6:52 p.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 12, 2016, 6:52 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1576
> https://issues.apache.org/jira/browse/AURORA-1576
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow for plugging in cli-configurable filters that are invoked post shiro 
> filters.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> 19c8a1fe06a24022da11f74d7c96b2942587 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  ac92117e14c105bec74e49d8e80eb56008237abf 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $ ./gradlew clean test
> ...
> BUILD SUCCESSFUL
> 
> Total time: 3 mins 24.616 secs
> ```
> 
> End to end tests:
> ```
> $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ...
> 
> *** OK (All tests passed) ***
> 
> aurora-scheduler-kerberos stop/waiting
> aurora-scheduler start/running, process 15362
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> ```
> 
> 
> Thanks,
> 
> Amol Deshmukh
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

[Joshua Cohen]

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

Ship it!


Ship It!

- Joshua Cohen


On Jan. 12, 2016, 6:52 p.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 12, 2016, 6:52 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1576
> https://issues.apache.org/jira/browse/AURORA-1576
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow for plugging in cli-configurable filters that are invoked post shiro 
> filters.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> 19c8a1fe06a24022da11f74d7c96b2942587 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  ac92117e14c105bec74e49d8e80eb56008237abf 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $ ./gradlew clean test
> ...
> BUILD SUCCESSFUL
> 
> Total time: 3 mins 24.616 secs
> ```
> 
> End to end tests:
> ```
> $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ...
> 
> *** OK (All tests passed) ***
> 
> aurora-scheduler-kerberos stop/waiting
> aurora-scheduler start/running, process 15362
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> ```
> 
> 
> Thanks,
> 
> Amol Deshmukh
> 
>

Re: Review Request 42077: Introduces -default_docker_parameters scheduler flag.

[Stephan Erb]

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

Ship it!


Ship It!

- Stephan Erb


On Jan. 12, 2016, 8:58 p.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42077/
> ---
> 
> (Updated Jan. 12, 2016, 8:58 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1575
> https://issues.apache.org/jira/browse/AURORA-1575
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This flag allows cluster administrators to set arbitrary
> Docker parameters which will apply to all jobs.
> 
> Also cleans up some of the existing unit tests around task config.
> 
> 
> Diffs
> -
> 
>   NEWS 83a1213e2ea4ab6b3706cd70c015ba9f16735520 
>   README.md f3b23247cf3f7e550c1714b4fb8227a2baab3b41 
>   
> commons/src/main/java/org/apache/aurora/common/args/parsers/MultimapParser.java
>  PRE-CREATION 
>   docs/deploying-aurora-scheduler.md 8a1e68e5d54e9b8b66139bfc731563668584fa77 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 4eee8e31a4ccd25ba4a5bcb60b67c79979c3b9b0 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> d7f3c60d383cf10afb0c0fcf4fe29972b183458c 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  05e8b714043dea89039ce9a1fc4b32c65ab15fe4 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  f3b62cc957186bc9673060830572bc1cc073ac49 
> 
> Diff: https://reviews.apache.org/r/42077/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 42077: Introduces -default_docker_parameters scheduler flag.

[George Sirois]

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

(Updated Jan. 12, 2016, 7:58 p.m.)


Review request for Aurora, Joshua Cohen and Bill Farner.


Changes
---

Documentation fixes + NEWS addition.


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


Repository: aurora


Description
---

This flag allows cluster administrators to set arbitrary
Docker parameters which will apply to all jobs.

Also cleans up some of the existing unit tests around task config.


Diffs (updated)
-

  NEWS 83a1213e2ea4ab6b3706cd70c015ba9f16735520 
  README.md f3b23247cf3f7e550c1714b4fb8227a2baab3b41 
  
commons/src/main/java/org/apache/aurora/common/args/parsers/MultimapParser.java 
PRE-CREATION 
  docs/deploying-aurora-scheduler.md 8a1e68e5d54e9b8b66139bfc731563668584fa77 
  src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
4eee8e31a4ccd25ba4a5bcb60b67c79979c3b9b0 
  src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
d7f3c60d383cf10afb0c0fcf4fe29972b183458c 
  
src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
 05e8b714043dea89039ce9a1fc4b32c65ab15fe4 
  
src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
 f3b62cc957186bc9673060830572bc1cc073ac49 

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


Testing
---

./build-support/jenkins/build.sh


Thanks,

George Sirois

Re: Review Request 42077: Introduces -default_docker_parameters scheduler flag.

[Bill Farner]

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

Ship it!


Ship It!

- Bill Farner


On Jan. 12, 2016, 11:58 a.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42077/
> ---
> 
> (Updated Jan. 12, 2016, 11:58 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1575
> https://issues.apache.org/jira/browse/AURORA-1575
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This flag allows cluster administrators to set arbitrary
> Docker parameters which will apply to all jobs.
> 
> Also cleans up some of the existing unit tests around task config.
> 
> 
> Diffs
> -
> 
>   NEWS 83a1213e2ea4ab6b3706cd70c015ba9f16735520 
>   README.md f3b23247cf3f7e550c1714b4fb8227a2baab3b41 
>   
> commons/src/main/java/org/apache/aurora/common/args/parsers/MultimapParser.java
>  PRE-CREATION 
>   docs/deploying-aurora-scheduler.md 8a1e68e5d54e9b8b66139bfc731563668584fa77 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 4eee8e31a4ccd25ba4a5bcb60b67c79979c3b9b0 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> d7f3c60d383cf10afb0c0fcf4fe29972b183458c 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  05e8b714043dea89039ce9a1fc4b32c65ab15fe4 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  f3b62cc957186bc9673060830572bc1cc073ac49 
> 
> Diff: https://reviews.apache.org/r/42077/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 42077: Introduces -default_docker_parameters scheduler flag.

[Aurora ReviewBot]

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

Ship it!


Master (d542bd1) 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. 12, 2016, 7:58 p.m., George Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42077/
> ---
> 
> (Updated Jan. 12, 2016, 7:58 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Bill Farner.
> 
> 
> Bugs: AURORA-1575
> https://issues.apache.org/jira/browse/AURORA-1575
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This flag allows cluster administrators to set arbitrary
> Docker parameters which will apply to all jobs.
> 
> Also cleans up some of the existing unit tests around task config.
> 
> 
> Diffs
> -
> 
>   NEWS 83a1213e2ea4ab6b3706cd70c015ba9f16735520 
>   README.md f3b23247cf3f7e550c1714b4fb8227a2baab3b41 
>   
> commons/src/main/java/org/apache/aurora/common/args/parsers/MultimapParser.java
>  PRE-CREATION 
>   docs/deploying-aurora-scheduler.md 8a1e68e5d54e9b8b66139bfc731563668584fa77 
>   src/main/java/org/apache/aurora/scheduler/app/AppModule.java 
> 4eee8e31a4ccd25ba4a5bcb60b67c79979c3b9b0 
>   src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> d7f3c60d383cf10afb0c0fcf4fe29972b183458c 
>   
> src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java
>  05e8b714043dea89039ce9a1fc4b32c65ab15fe4 
>   
> src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java
>  f3b62cc957186bc9673060830572bc1cc073ac49 
> 
> Diff: https://reviews.apache.org/r/42077/diff/
> 
> 
> Testing
> ---
> 
> ./build-support/jenkins/build.sh
> 
> 
> Thanks,
> 
> George Sirois
> 
>

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Bill Farner]

> On Jan. 11, 2016, 3:42 p.m., Zhitao Li wrote:
> > src/main/java/org/apache/aurora/scheduler/OfferAllocation.java, line 188
> > 
> >
> > Hmm, I think I'd like to keep this for two reasons:
> > 
> > 1. Even though only one callsite uses value other than `false`, the 
> > `false` value still dictates a `Predicate` to use, so we'll have more code 
> > duplicate if we move this out;
> > 2. Right now aurora only supports `cpus` as revocable resource. I 
> > actually wanted to discuss this separately as current prediction in our 
> > company indicates that we might be more memory bounded. In such a case, we 
> > could be using memory as revocable resources, too.

> Even though only one callsite uses value other than false, the false value 
> still dictates a Predicate to use, so we'll have more code duplicate if we 
> move this out

Not sure i agree with the code duplication, but i will follow up on the next 
patch draft.

> ...we might be more memory bounded. In such a case, we could be using memory 
> as revocable resources, too.

I'm not sure if that is possible, but it definitely is not recommended.

http://mesos.apache.org/documentation/latest/oversubscription/
> It is recommended only to oversubscribe compressible resources such as cpu 
> shares, bandwidth, etc.


> On Jan. 11, 2016, 3:42 p.m., Zhitao Li wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line 
> > 120
> > 
> >
> > I don't plan on any further usage of the interface other than here in 
> > this series of patches.
> > 
> > The only possible customization that could happen is the reverse 
> > ordering of resource preference (`'*'` vs `reserved role`) but I'd like to 
> > withhold it until someone request such feature.
> > 
> > I think I'll keep the interface definition, change the 
> > `AcceptedOfferImpl` to package private, and use a factory method anyway. We 
> > can revisit this later. How does this sound?

I will follow up on the latest draft.


- Bill


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


On Jan. 11, 2016, 6:29 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 11, 2016, 6:29 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new interface OfferAllocation (with implementation), which 
> allcoates resources to resources field in TaskInfo and ExecutorInfo from an 
> offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present. This can be  customized with a differnet 
> comparator from PREFER_RESERVED in the future when needed.
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure;
> 2. Old code used to construct resources has not been cleaned up yet;
> 3. OfferAllocationImpl's constructor allows to throw, which is a bit awkward.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/AcceptedOfferImplTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 

Re: Review Request 42225: Add `--show-error` to curl when bootstrapping thrift.

[Aurora ReviewBot]

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

Ship it!


Master (d542bd1) 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. 12, 2016, 11:51 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42225/
> ---
> 
> (Updated Jan. 12, 2016, 11:51 p.m.)
> 
> 
> Review request for Aurora and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> From the curl documentation:
> 
> -S, --show-error
> 
> When used with -s it makes curl show an error message if it fails.
> 
> 
> It's possible for curl to fail when grabbing the tarball or patch and this 
> will
> show users why it failed.
> 
> 
> Diffs
> -
> 
>   build-support/thrift/Makefile 8b1da2a01b9ecc1832d4e56bfaacb5b40cdbbab0 
> 
> Diff: https://reviews.apache.org/r/42225/diff/
> 
> 
> Testing
> ---
> 
> Ran `make` in the `build-support/thrift` directory.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Review Request 42225: Add `--show-error` to curl when bootstrapping thrift.

[Zameer Manji]

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

Review request for Aurora and John Sirois.


Repository: aurora


Description
---

>From the curl documentation:

-S, --show-error

When used with -s it makes curl show an error message if it fails.


It's possible for curl to fail when grabbing the tarball or patch and this will
show users why it failed.


Diffs
-

  build-support/thrift/Makefile 8b1da2a01b9ecc1832d4e56bfaacb5b40cdbbab0 

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


Testing
---

Ran `make` in the `build-support/thrift` directory.


Thanks,

Zameer Manji

Re: Review Request 42225: Add `--show-error` to curl when bootstrapping thrift.

[John Sirois]

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

Ship it!


I almost always use -sS for this very reason - this makes sense to me!


build-support/thrift/Makefile (line 50)


>100 but I'm not sure how this goes in non-py|java files for aurora.


- John Sirois


On Jan. 12, 2016, 4:51 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42225/
> ---
> 
> (Updated Jan. 12, 2016, 4:51 p.m.)
> 
> 
> Review request for Aurora and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> From the curl documentation:
> 
> -S, --show-error
> 
> When used with -s it makes curl show an error message if it fails.
> 
> 
> It's possible for curl to fail when grabbing the tarball or patch and this 
> will
> show users why it failed.
> 
> 
> Diffs
> -
> 
>   build-support/thrift/Makefile 8b1da2a01b9ecc1832d4e56bfaacb5b40cdbbab0 
> 
> Diff: https://reviews.apache.org/r/42225/diff/
> 
> 
> Testing
> ---
> 
> Ran `make` in the `build-support/thrift` directory.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42225: Add `--show-error` to curl when bootstrapping thrift.

[Bill Farner]

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

Ship it!


Ship It!

- Bill Farner


On Jan. 12, 2016, 3:51 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42225/
> ---
> 
> (Updated Jan. 12, 2016, 3:51 p.m.)
> 
> 
> Review request for Aurora and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> From the curl documentation:
> 
> -S, --show-error
> 
> When used with -s it makes curl show an error message if it fails.
> 
> 
> It's possible for curl to fail when grabbing the tarball or patch and this 
> will
> show users why it failed.
> 
> 
> Diffs
> -
> 
>   build-support/thrift/Makefile 8b1da2a01b9ecc1832d4e56bfaacb5b40cdbbab0 
> 
> Diff: https://reviews.apache.org/r/42225/diff/
> 
> 
> Testing
> ---
> 
> Ran `make` in the `build-support/thrift` directory.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

[Bill Farner]

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


It sounds like you ran into an issue that necessitated this patch.  Is there 
anything you can add to `docs/security.md` so others don't get stuck down the 
same dead end?

Also, please add a NEWS entry summarizing the new field so we make sure to 
highlight it in the next release.


src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
 (line 82)


How about `shiro_after_auth_filter`?  While more verbose, it avoids the 
term overload of `post` with HTTP POST request (which is what i actually 
thought this was for at first).  If you do rename, please apply it throughout 
the patch.



src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
 (line 118)


Drop the `maybe`, the `Optional` type is sufficient to indicate the 
possibly-absent value.

Also, you should generally prefer to accept `Key` instead of `Class` for 
APIs relating to identifiers for guice bindings. So in this case, use `Key` everywhere except the `Arg` as that's more general and has 
parity with the `ShiroWebModule` API.  Additionally, you should remove the line 
`bind(postFilter).in(Singleton.class);`, and require that the filter be 
externally bound.  This gives the caller greater flexibility when it comes to 
defining the binding.



src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
 (line 138)


Make this `Optional` to have a consistent API/internals, and change the 
`null` above to `Optional.empty()`



src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
 (line 209)


Usually we try to minimize the scope of suppressions to prevent 
unexpectedly covering real warnings.  Consider extracting a function that 
covers only the line requiring the suppression.  This obviates the need for the 
comment `// For xyz` and prevents issues later on.



src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java 
(line 112)


Can you approach this with a mock?  Seems like it would be more direct and 
less brittle way to verify calls.  With the suggestion above to accept `Key` 
and avoid the internal `bind()`, this should become much more doable than with 
the current API.


- Bill Farner


On Jan. 12, 2016, 10:52 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 12, 2016, 10:52 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1576
> https://issues.apache.org/jira/browse/AURORA-1576
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow for plugging in cli-configurable filters that are invoked post shiro 
> filters.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> 19c8a1fe06a24022da11f74d7c96b2942587 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  ac92117e14c105bec74e49d8e80eb56008237abf 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $ ./gradlew clean test
> ...
> BUILD SUCCESSFUL
> 
> Total time: 3 mins 24.616 secs
> ```
> 
> End to end tests:
> ```
> $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ...
> 
> *** OK (All tests passed) ***
> 
> aurora-scheduler-kerberos stop/waiting
> aurora-scheduler start/running, process 15362
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> ```
> 
> 
> Thanks,
> 
> Amol Deshmukh
> 
>

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Zhitao Li]

> On Jan. 12, 2016, 6:05 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 40
> > 
> >
> > The interface should really be removed.  It's not acting as an 
> > interface hiding an implementation, so its value is not worth its weight.  
> > Everything else can remain as-is (e.g. static factory function).

Will do.


> On Jan. 12, 2016, 6:05 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 62
> > 
> >
> > Please move ~all of this method body into `create` to avoid doing 
> > significant work in the constructor.
> > 
> > Background on this convention can be found here if you're interested: 
> > http://misko.hevery.com/code-reviewers-guide/flaw-constructor-does-real-work/
> > 
> > The constructor should whittle down to something as basic as:
> > 
> > ```
> > AcceptedOffer(ImmutableList taskResources, 
> > ImmutableList executorResources) {
> >   this.taskResources = requireNonNull(taskResources);
> >   this.executorResources = requireNonNull(executorResources);
> > }
> > ```

Will do.


> On Jan. 12, 2016, 6:05 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 199
> > 
> >
> > I now see why you do indeed need _some_ flag for `revocable` because 
> > both `REVOCABLE` and `NON_REVOCABLE` filters are active (neither are a 
> > no-op).  I suggest that you accept `Predicate` and introduce an 
> > overload to make the call sites more readable (`false` does not indicate 
> > much)
> > 
> > ```
> > static List filterNonRevocableAndSort(List 
> > resources, String name) {
> >   return filterAndSort(
> >   resources,
> >   name,
> >   revocable ? Resources.REVOCABLE : Resources.NON_REVOCABLE);
> > }
> > 
> > static List filterAndSort(
> > List resources,
> > String name,
> > Predicate additionalFilter) {
> > 
> >   return FluentIterable.from(resources)
> >   .filter(e -> e.getName().equals(name))
> >   .filter(additionalFilter)
> >   .toSortedList(PREFER_RESERVED);
> > }
> > ```
> > 
> > Then most call sites become `filterNonRevocableAndSort`, and the call 
> > for CPU passes `Resources.REVOCABLE`.  Does that make sense?

Yes this makes sense. The only difference I'd like is to make this return an 
`ArrayList` instead of `ImmutableList` from `toSortedList` because this list 
needs to be mutable so we can allocate resources from it.


> On Jan. 12, 2016, 6:05 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, lines 76-97
> > 
> >
> > A few thoughts for this code block:
> > 1. rather than sort, filter reserved resources and arrange them first.  
> > the sort is extra work, and currently defines an arbitrary sub-sort on role 
> > name which is misleading at best.
> > 2. convert to List upfront and use these to track the 
> > remaining capacity of the resource by mutating them
> > 
> > If you agree, you would start this block with something like
> > ```
> > List reservedFirst = ImmutableList.builder()
> >   .addAll(Iterables.filter(offer.getResourcesList(), RESERVED)
> >   .addAll(Iterables.filter(offer.getResourcesList(), NOT_RESERVED)
> >   .build()
> > 
> > List availableResources = 
> > FluentIterable.from(reservedFirst)
> > .transform(Resource::toBuilder)
> > .toList();
> > ```
> > 
> > This will also allow you to avoid modifying the `List` itself (as is 
> > currently done below with `ListIterator.remove/add`) since you can adjust 
> > the remaining capacity in `Resource.Builder` instances rather than 
> > constructing new ones.

Will do.


> On Jan. 12, 2016, 6:05 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java, line 110
> > 
> >
> > I'd like to strive to remove `@VisibleForTesting` from all methods in 
> > this class and make them `private`.  It will likely require more 
> > fixtures/helpers in the unit test, but the result will be a unit test that 
> > is not so highly coupled to the implementation.  Do you see anything 
> > preventing this approach?

Will do. I think I just need one helper function to be package private so I 
don't repeat it in test.


- Zhitao


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

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

[Zhitao Li]

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

(Updated Jan. 13, 2016, 1:34 a.m.)


Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
Farner.


Changes
---

Better code and test organized by Will's comments, making all lists in the 
review immutable.

Also updatd review summyar and description.


Summary (updated)
-

New class to allocate resources of multiple roles from offer.


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


Repository: aurora


Description (updated)
---

This review is a prototype for introducing multiple role support in Aurora.
This creates a new class OfferAllocation, which allcoates resources to 
resources field in TaskInfo and ExecutorInfo from an offer.

Current implementation prefers reserved resources over shared resources ('*' 
role) if both are present

Several caveats:
1. This performs the allocate after scheduling decision in 
TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency and 
late failure.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
7c3d681c216b78eeecebbe950186e5a79c6fe982 
  src/main/java/org/apache/aurora/scheduler/Resources.java 
db422a959ee7b982c2a44323de41ad75d1a40754 
  
src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
 2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
  src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
8fdadda67478bb3110aa442b7d78493cf9c3edb4 
  src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
7e8e456e288986eb0ce92a123b294e1e25d8ed18 
  src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
e4ae943303823ac4bfbe999ed22f5999484462d8 
  
src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
 33149ab415292eff04f38b61f2b1d1eac79f347a 
  src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
  src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
3cbe9acd75def14ae2e0986914ba621fb164b3e4 

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


Testing
---

1. Unit tested with old and new tests;
2. vagrant integration tests: I manually separate out the vagrant box's cpu and 
memory between 'aurora-test' role and '*' and verified that jobs can still be 
launched (I can post the vagrant change in another follow upon request).


Thanks,

Zhitao Li

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

[Bill Farner]

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



src/main/java/org/apache/aurora/scheduler/Resources.java (line 50)


Looks like an overly-aggressive find/replace.  I've been endlessly 
frustrated by intellij doing this when you forget to untick the "search text 
and comments" option (which is always ticked).

Please skim through the patch to find other spots impacted.


- Bill Farner


On Jan. 12, 2016, 5:34 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 12, 2016, 5:34 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new class OfferAllocation, which allcoates resources to 
> resources field in TaskInfo and ExecutorInfo from an offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> 1. Unit tested with old and new tests;
> 2. vagrant integration tests: I manually separate out the vagrant box's cpu 
> and memory between 'aurora-test' role and '*' and verified that jobs can 
> still be launched (I can post the vagrant change in another follow upon 
> request).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

[John Sirois]

> On Jan. 12, 2016, 6:49 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/Resources.java, line 50
> > 
> >
> > Looks like an overly-aggressive find/replace.  I've been endlessly 
> > frustrated by intellij doing this when you forget to untick the "search 
> > text and comments" option (which is always ticked).
> > 
> > Please skim through the patch to find other spots impacted.

In IntelliJ settings, search for "Find Usages Settings" - I map this to 
`ctrl-alt-shift-7` - pops up dialog with scope and seach in text or not and 
other options.


- John


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


On Jan. 12, 2016, 6:34 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 12, 2016, 6:34 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new class OfferAllocation, which allcoates resources to 
> resources field in TaskInfo and ExecutorInfo from an offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> 1. Unit tested with old and new tests;
> 2. vagrant integration tests: I manually separate out the vagrant box's cpu 
> and memory between 'aurora-test' role and '*' and verified that jobs can 
> still be launched (I can post the vagrant change in another follow upon 
> request).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

[Aurora ReviewBot]

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

Ship it!


Master (952ef6d) 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. 13, 2016, 1:34 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 13, 2016, 1:34 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new class OfferAllocation, which allcoates resources to 
> resources field in TaskInfo and ExecutorInfo from an offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> 1. Unit tested with old and new tests;
> 2. vagrant integration tests: I manually separate out the vagrant box's cpu 
> and memory between 'aurora-test' role and '*' and verified that jobs can 
> still be launched (I can post the vagrant change in another follow upon 
> request).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

[John Sirois]

> On Jan. 12, 2016, 6:49 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/Resources.java, line 50
> > 
> >
> > Looks like an overly-aggressive find/replace.  I've been endlessly 
> > frustrated by intellij doing this when you forget to untick the "search 
> > text and comments" option (which is always ticked).
> > 
> > Please skim through the patch to find other spots impacted.
> 
> John Sirois wrote:
> In IntelliJ settings, search for "Find Usages Settings" - I map this to 
> `ctrl-alt-shift-7` - pops up dialog with scope and seach in text or not and 
> other options.

Oh - oops, you're issue is having to untick.  I managed to set things up so its 
unticked by default, I'll dig.


- John


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


On Jan. 12, 2016, 6:34 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 12, 2016, 6:34 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new class OfferAllocation, which allcoates resources to 
> resources field in TaskInfo and ExecutorInfo from an offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> 1. Unit tested with old and new tests;
> 2. vagrant integration tests: I manually separate out the vagrant box's cpu 
> and memory between 'aurora-test' role and '*' and verified that jobs can 
> still be launched (I can post the vagrant change in another follow upon 
> request).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42126: New class to allocate resources of multiple roles from offer.

[Zhitao Li]

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

(Updated Jan. 13, 2016, 2:08 a.m.)


Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
Farner.


Changes
---

Undo incorrect comment find/replace by Intellij.


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


Repository: aurora


Description
---

This review is a prototype for introducing multiple role support in Aurora.
This creates a new class OfferAllocation, which allcoates resources to 
resources field in TaskInfo and ExecutorInfo from an offer.

Current implementation prefers reserved resources over shared resources ('*' 
role) if both are present

Several caveats:
1. This performs the allocate after scheduling decision in 
TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency and 
late failure.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
  src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
7c3d681c216b78eeecebbe950186e5a79c6fe982 
  src/main/java/org/apache/aurora/scheduler/Resources.java 
db422a959ee7b982c2a44323de41ad75d1a40754 
  
src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
 2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
  src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
8fdadda67478bb3110aa442b7d78493cf9c3edb4 
  src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
7e8e456e288986eb0ce92a123b294e1e25d8ed18 
  src/test/java/org/apache/aurora/scheduler/AcceptedOfferTest.java PRE-CREATION 
  src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
e4ae943303823ac4bfbe999ed22f5999484462d8 
  
src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
 33149ab415292eff04f38b61f2b1d1eac79f347a 
  src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
  src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
3cbe9acd75def14ae2e0986914ba621fb164b3e4 

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


Testing
---

1. Unit tested with old and new tests;
2. vagrant integration tests: I manually separate out the vagrant box's cpu and 
memory between 'aurora-test' role and '*' and verified that jobs can still be 
launched (I can post the vagrant change in another follow upon request).


Thanks,

Zhitao Li

Re: Review Request 40877: Update rpm startup scripts to match deb patterns in use

[John Sirois]

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


Jake - I'm willing to take this patch over if you don't have time to land it.
Speak up by this Wednesday 1/13 after which time I'll assume you're ok with 
this course of action.

- John Sirois


On Dec. 2, 2015, 12:15 p.m., Jake Farrell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40877/
> ---
> 
> (Updated Dec. 2, 2015, 12:15 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora-packaging
> 
> 
> Description
> ---
> 
> Update rpm startup scripts to match deb patterns in use
> 
> 
> Diffs
> -
> 
>   specs/rpm/SOURCES/aurora.init.sh  
>   specs/rpm/SOURCES/aurora.logrotate  
>   specs/rpm/SOURCES/aurora.service  
>   specs/rpm/SOURCES/aurora.startup.sh  
>   specs/rpm/SOURCES/aurora.sysconfig  
>   specs/rpm/SOURCES/thermos-observer.init.sh  
>   specs/rpm/SOURCES/thermos-observer.logrotate  
>   specs/rpm/SOURCES/thermos-observer.service  
>   specs/rpm/SOURCES/thermos-observer.startup.sh  
>   specs/rpm/SOURCES/thermos-observer.sysconfig  
>   specs/rpm/aurora.spec 83cea36 
> 
> Diff: https://reviews.apache.org/r/40877/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jake Farrell
> 
>

Re: Review Request 42126: New interface to allocate resources of multiple roles from offer.

[Bill Farner]

> On Jan. 11, 2016, 8:25 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line 
> > 201
> > 
> >
> > Is `clearResources()` necessary here?  I think it makes sense to not 
> > intervene here if an operator with a custom config wants to specify 
> > resources, even if it's at their peril.
> 
> Zhitao Li wrote:
> This is at least necessary to get current tests pass. IIRC this is due to 
> how Aurora handles resource limit of executor right now:
> 1. A prototype of `ExecutorInfo` is created in `ExecutorSettings`;
> 2. A `ResourceSlot` representing the usage is computed right before when 
> we need to allocate resources from an offer;
> 3. The `ResourceSlot` is then passed to my new implementation to allocate 
> resources properly next to `TaskInfo`'s.
> 
> We would need to change this workflow to make sure there cannot be 
> previously set resources if we want to avoid the `clearResources()` call.
> 
> Let me know if you think tackling this workflow change should be part of 
> this change.

Sounds reasonable.  I suspect we will need to revisit this in the future, but 
probably not worth dwelling on now.


- Bill


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


On Jan. 11, 2016, 6:29 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42126/
> ---
> 
> (Updated Jan. 11, 2016, 6:29 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Dmitriy Shirchenko, and Bill 
> Farner.
> 
> 
> Bugs: AURORA-1109
> https://issues.apache.org/jira/browse/AURORA-1109
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This review is a prototype for introducing multiple role support in Aurora.
> This creates a new interface OfferAllocation (with implementation), which 
> allcoates resources to resources field in TaskInfo and ExecutorInfo from an 
> offer.
> 
> Current implementation prefers reserved resources over shared resources ('*' 
> role) if both are present. This can be  customized with a differnet 
> comparator from PREFER_RESERVED in the future when needed.
> 
> Several caveats:
> 1. This performs the allocate after scheduling decision in 
> TaskAssigner.maybeAssign is done, which leaves possibility of inconsistency 
> and late failure;
> 2. Old code used to construct resources has not been cleaned up yet;
> 3. OfferAllocationImpl's constructor allows to throw, which is a bit awkward.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/AcceptedOffer.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/ResourceSlot.java 
> 7c3d681c216b78eeecebbe950186e5a79c6fe982 
>   src/main/java/org/apache/aurora/scheduler/Resources.java 
> db422a959ee7b982c2a44323de41ad75d1a40754 
>   
> src/main/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModule.java
>  2255dd407cd1810c7df5baf17cfa85f79bfffeb8 
>   src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 
> 8fdadda67478bb3110aa442b7d78493cf9c3edb4 
>   src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 
> 7e8e456e288986eb0ce92a123b294e1e25d8ed18 
>   src/test/java/org/apache/aurora/scheduler/AcceptedOfferImplTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/ResourceSlotTest.java 
> e4ae943303823ac4bfbe999ed22f5999484462d8 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/CommandLineDriverSettingsModuleTest.java
>  33149ab415292eff04f38b61f2b1d1eac79f347a 
>   
> src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 
> a5793bffabf4e5d6195b1b99f2363d241c0cecf9 
>   src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 
> 3cbe9acd75def14ae2e0986914ba621fb164b3e4 
> 
> Diff: https://reviews.apache.org/r/42126/diff/
> 
> 
> Testing
> ---
> 
> 1. Unit tested with old and new tests;
> 2. vagrant integration tests: I manually separate out the vagrant box's cpu 
> and memory between 'aurora-test' role and '*' and verified that jobs can 
> still be launched (I can post the vagrant change in another follow upon 
> request).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

[Joshua Cohen]

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


It would've been nice if, by default, we did not wire up the post-filter at 
all, and only asserted its counters when it has been wired in, but from a quick 
glance at the way the tests are set up, it doesn't seem to be possible to 
conditionally configure the `HttpSecurityModule`.


src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
 (lines 174 - 179)


Does this need to be here? Can this just be done in the downstream 
user-defined filter instead?



src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java 
(line 123)


Do we need to use a `StatsProvider` for this? Can we just expose call 
counts from this class directly? Are there even threading concerns that require 
atomicity?



src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java 
(lines 153 - 156)


Nit, just use `//` for multi-line comments.


- Joshua Cohen


On Jan. 12, 2016, 1:15 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 12, 2016, 1:15 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1576
> https://issues.apache.org/jira/browse/AURORA-1576
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Allow for plugging in cli-configurable filters that are invoked post shiro 
> filters.
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> 19c8a1fe06a24022da11f74d7c96b2942587 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  ac92117e14c105bec74e49d8e80eb56008237abf 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $ ./gradlew clean test
> ...
> BUILD SUCCESSFUL
> 
> Total time: 3 mins 24.616 secs
> ```
> 
> End to end tests:
> ```
> $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ...
> 
> *** OK (All tests passed) ***
> 
> aurora-scheduler-kerberos stop/waiting
> aurora-scheduler start/running, process 15362
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> ```
> 
> 
> Thanks,
> 
> Amol Deshmukh
> 
>