Re: Review Request 40114: Windows: Began adding Windows support to `process/future.hpp`

[Daniel Pravat]

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

(Updated Dec. 1, 2015, 8:38 p.m.)


Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
Remoortere, and Joseph Wu.


Repository: mesos


Description
---

Added Windows & Posix defines for several parameter list used in future.hpp. 
Provided a new parameter list for VC2015.
Move the existing parameter list into posix.hpp. 

The change has been due to: 

   1. VS 2015 won't support C++14 result_of SFINAE until Update 2,  so
  result_of must be replaced with decltype(invoke).
   2. VS 2015 won't support C++14 std::function SFINAE until Update 2, so
  converting _Deferred to std::function must be done by explicitly
  calling _Deferred's conversion function.
   3. The Standard (C++11 through 17) does not require bind's function call
  operator to SFINAE, and VS 2015's doesn't.  is_bind_expression can be
  used to manually reroute bind expressions to the 1-arg overload, where
  (conveniently) the argument will be ignored if necessary.


Diffs (updated)
-

  3rdparty/libprocess/include/Makefile.am 
47f5347988a61140c87bcd329e25d5a4d52e17a0 
  3rdparty/libprocess/include/process/future.hpp 
c9146e3a3ccf09dd37c5a8ac7000fbe84f3c710c 
  3rdparty/libprocess/include/process/posix/future.hpp PRE-CREATION 
  3rdparty/libprocess/include/process/windows/future.hpp PRE-CREATION 

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


Testing
---

OSX make check, Windows 10 make, Ubuntu 15.1 make check


Thanks,

Daniel Pravat

Re: Review Request 40115: Windows: Added support for `slave/gc.cpp'.

[Daniel Pravat]

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

(Updated Dec. 1, 2015, 9:06 p.m.)


Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
Remoortere, and Joseph Wu.


Repository: mesos


Description
---

Windows: Added support for `slave/gc.cpp'.


Diffs (updated)
-

  src/CMakeLists.txt cfe9d26c45ba42852fd1af958549954e7b04d448 
  src/slave/gc.cpp 7a8c69b4410df46ca8fd6ac009cc14e8fe5ff6d3 

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


Testing
---

Windows 10: make.bat
OSX: make check
Ubuntu: 15.1 make check


Thanks,

Daniel Pravat

Re: Review Request 40818: Fixed flaky test (AvailableResourcesAfterRescinding).

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40818]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 1, 2015, 4:52 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40818/
> ---
> 
> (Updated Dec. 1, 2015, 4:52 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addendum to b2b0eedc7371cd294b715fc91312989919d57c7a
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp 481298361574008b2cf9d213c908deb633393e31 
> 
> Diff: https://reviews.apache.org/r/40818/diff/
> 
> 
> Testing
> ---
> 
> Two test setups on Mac OS X 10.10.4.
> 
> `GTEST_FILTER="*AvailableResourcesAfterRescinding*" ./bin/mesos-tests.sh 
> --gtest_repeat=1000` *with* `allocation_interval` set to `50ms`
> The probability of a batch allocation between rescinding and `Shutdown()` is 
> low.
> 
> 
> `GTEST_FILTER="*AvailableResourcesAfterRescinding*" ./bin/mesos-tests.sh 
> --gtest_repeat=1000` *with* simulation of slow CI
> A batch allocation between rescinding and `Shutdown()` is triggered by 
> inserting the following lines after resources are recovered:
> ```
>   Clock::pause();
>   Clock::advance(flags.allocation_timeout);
>   Clock::settle();
>   Clock::resume();
> ```
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40620: Windows: Added suppport for `slave/monitor.cpp'.

[Daniel Pravat]

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

(Updated Dec. 1, 2015, 9:02 p.m.)


Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
Remoortere, and Joseph Wu.


Repository: mesos


Description
---

Windows: Added suppport for `slave/monitor.cpp'.


Diffs (updated)
-

  src/CMakeLists.txt cfe9d26c45ba42852fd1af958549954e7b04d448 

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


Testing
---

OSX: make
Windows: make


Thanks,

Daniel Pravat

Re: Review Request 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.

[Greg Mann]

> On Dec. 1, 2015, 2:24 p.m., Michael Park wrote:
> > src/master/master.cpp, line 2771
> > 
> >
> > Why is it that we need to perform validation within authorization? We 
> > perform `authorizeTask` with a `TaskInfo` that has not been validated yet, 
> > and take care of it later on. Does that pattern not work here?
> 
> Greg Mann wrote:
> That pattern would work here, and the original patches I posted worked 
> that way. We ended up switching to this solution because it makes for a 
> cleaner implementation and eliminates some redundant code. However, it does 
> come at the cost of making error messaging more difficult (as you noted in 
> another patch) and decreasing the separation of functionality that can make 
> debugging easier. I don't have a strong preference for either path at the 
> moment. Perhaps Jie could chime in with his thoughts?
> 
> Jie Yu wrote:
> I looked at the code again. I guess the reason we validate the operation 
> before authorization is because we want to get all the principals from 
> resources in UNRESERVE, and we want to make sure resources in UNRESERVE 
> operation is valid. As MPark pointed out, coupling authorization with 
> validation does cause some confusion when generating error messages. How 
> about the following:
> 
> 1) we don't perform validation in authorization. In 
> authorizeUnreserveResources, when we iterate all resources, we can do the 
> following as you previosly did (sorry about the back and forth, my bad).
> ```
> foreach (const Resource& resource, unreserve.resources()) {
>   if (Resources::isDynamicallyReserved(resource)) {
> request.mutable_reserver_principals()->add_values(
> resource.reservation().principal());
>   }
> }
> ```
> 
> 2) in Http::reserve and Http::unreserve, we still perform validation 
> first before calling master->authroizeXXX.
> 
> 3) in `Master::_accept`, we perform operation validation on 
> RESERVE/UNRESERVE.
> 
> Let me know if that's better or not?
> 
> Greg Mann wrote:
> No worries, Jie! I learned about `Future<>::repair()` in the process so I 
> consider it worthwhile :-)
> 
> I have one question about your plan: for `LAUNCH` operations, tasks are 
> authorized before they get validated, and I'm wondering if it's worth making 
> this part of the interface consistent across all types of operations, i.e. 
> perhaps we should change the code for `LAUNCH` to validate before 
> authorization as well. If we decide to do this, I could do it here or in 
> another review.

Since the validation for `LAUNCH` needs to be aware of tasks that were 
previously launched on the same operation, it makes sense in that case for 
validation to occur after authorization. However, in general, I think it makes 
more sense for the operation to be validated before authorization, so I'll 
follow the pattern that Jie suggested above for 
`RESERVE/UNRESERVE/CREATE/DESTROY`. This means that the interface will have 
slightly different expectations regarding validation depending on what type of 
operation is being performed.

An alternative would be to validate all operations *after* authorization is 
performed, which in general seems less intuitive to me, but would keep the 
interface consistent...


- Greg


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


On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> ---
> 
> (Updated Nov. 30, 2015, 8:35 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
> 
> Diff: https://reviews.apache.org/r/39987/diff/
> 
> 
> Testing
> ---
> 
> This is the third in a chain of 5 patches. `make check` was used to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40799: Removed non-ASCII characters from test case comment.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40799]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Nov. 30, 2015, 6:43 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40799/
> ---
> 
> (Updated Nov. 30, 2015, 6:43 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed non-ASCII characters from test case comment.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp 481298361574008b2cf9d213c908deb633393e31 
> 
> Diff: https://reviews.apache.org/r/40799/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 38234: Check if swap is enabled before running memory pressure related tests.

[Vinod Kone]

> On Nov. 17, 2015, 7:27 p.m., Vinod Kone wrote:
> > src/tests/containerizer/cgroups_tests.cpp, lines 560-568
> > 
> >
> > instead of asserting, it would be better if we can disable the test 
> > automatically if we detect that swap is enabled.
> > 
> > for example, we can change the test name to ROOT_CGROUPS_OOM_Listen
> > 
> > and in environment.cpp, disable the test if 'OOM' token is present in 
> > the test name and swap is enabled.
> > 
> > 
> > also, does the swap issue not affect the balloon framewok test?
> 
> Chi Zhang wrote:
> Yeah, I wanted to do that to disable other tests dymaically before and 
> final result was just to use this style, so I did the same to be consistent.
> 
> re-read the code again for balloon frame tests: slave has 96MB memory, 
> executor uses 64MB, 32 left to the task, balloon() allocates 64MB at a time, 
> which basically means first allocation request will cause OOM (64 > 32).
> 
> If the step was set to say do 3 times 16MBs, (3x16 > 32), the extra 16MB 
> will be eaten by swap and not cause a OOM if swap is enabled.

not sure why dynamica disabling didn't work before, but i'll add a TODO for now 
and commit this.


- Vinod


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


On Sept. 9, 2015, 9:33 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38234/
> ---
> 
> (Updated Sept. 9, 2015, 9:33 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: mesos-2918
> https://issues.apache.org/jira/browse/mesos-2918
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Check if swap is enabled before running memory pressure related tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cgroups_tests.cpp 
> 75a3bc0009c037dc18ce319db2eb44630f083e8c 
> 
> Diff: https://reviews.apache.org/r/38234/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Re: Review Request 40618: Include the header for boost::hash_combine explicitly

[Vinod Kone]

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

Ship it!


Ship It!

- Vinod Kone


On Nov. 23, 2015, 10:42 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40618/
> ---
> 
> (Updated Nov. 23, 2015, 10:42 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently boost/functional/hash.hpp is included implicitly, we should include 
> it explicitly.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
> bffdc3829208496ef14fecca497af73d4399867b 
> 
> Diff: https://reviews.apache.org/r/40618/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 40822: Updated the pattern for `_teardown` authorization continuation.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40822]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 1, 2015, 3:16 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40822/
> ---
> 
> (Updated Dec. 1, 2015, 3:16 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Motivated by https://reviews.apache.org/r/39988/#comment167856
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 9d729ef7f7d7ad6185934648f833e4f8a4f0a123 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
> 
> Diff: https://reviews.apache.org/r/40822/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.

[Greg Mann]

> On Dec. 1, 2015, 2:24 p.m., Michael Park wrote:
> > src/master/master.cpp, line 2771
> > 
> >
> > Why is it that we need to perform validation within authorization? We 
> > perform `authorizeTask` with a `TaskInfo` that has not been validated yet, 
> > and take care of it later on. Does that pattern not work here?
> 
> Greg Mann wrote:
> That pattern would work here, and the original patches I posted worked 
> that way. We ended up switching to this solution because it makes for a 
> cleaner implementation and eliminates some redundant code. However, it does 
> come at the cost of making error messaging more difficult (as you noted in 
> another patch) and decreasing the separation of functionality that can make 
> debugging easier. I don't have a strong preference for either path at the 
> moment. Perhaps Jie could chime in with his thoughts?
> 
> Jie Yu wrote:
> I looked at the code again. I guess the reason we validate the operation 
> before authorization is because we want to get all the principals from 
> resources in UNRESERVE, and we want to make sure resources in UNRESERVE 
> operation is valid. As MPark pointed out, coupling authorization with 
> validation does cause some confusion when generating error messages. How 
> about the following:
> 
> 1) we don't perform validation in authorization. In 
> authorizeUnreserveResources, when we iterate all resources, we can do the 
> following as you previosly did (sorry about the back and forth, my bad).
> ```
> foreach (const Resource& resource, unreserve.resources()) {
>   if (Resources::isDynamicallyReserved(resource)) {
> request.mutable_reserver_principals()->add_values(
> resource.reservation().principal());
>   }
> }
> ```
> 
> 2) in Http::reserve and Http::unreserve, we still perform validation 
> first before calling master->authroizeXXX.
> 
> 3) in `Master::_accept`, we perform operation validation on 
> RESERVE/UNRESERVE.
> 
> Let me know if that's better or not?
> 
> Greg Mann wrote:
> No worries, Jie! I learned about `Future<>::repair()` in the process so I 
> consider it worthwhile :-)
> 
> I have one question about your plan: for `LAUNCH` operations, tasks are 
> authorized before they get validated, and I'm wondering if it's worth making 
> this part of the interface consistent across all types of operations, i.e. 
> perhaps we should change the code for `LAUNCH` to validate before 
> authorization as well. If we decide to do this, I could do it here or in 
> another review.
> 
> Greg Mann wrote:
> Since the validation for `LAUNCH` needs to be aware of tasks that were 
> previously launched on the same operation, it makes sense in that case for 
> validation to occur after authorization. However, in general, I think it 
> makes more sense for the operation to be validated before authorization, so 
> I'll follow the pattern that Jie suggested above for 
> `RESERVE/UNRESERVE/CREATE/DESTROY`. This means that the interface will have 
> slightly different expectations regarding validation depending on what type 
> of operation is being performed.
> 
> An alternative would be to validate all operations *after* authorization 
> is performed, which in general seems less intuitive to me, but would keep the 
> interface consistent...
> 
> Jie Yu wrote:
> Greg, the code sample I suggested above actually tried to keep the 
> interface consistent (validation is done separately and is decoupled from 
> authorization). Just want to double check with you.

Thanks for checking Jie, I think I misread your comments previously.

Your suggested solution would keep the separation of validation and 
authorization consistent, the difference is that for HTTP endpoint operations 
validation would be done *before* authorization, while for framework operations 
validation would be done *after* authorization. I guess the question is, do we 
think it's worthwhile to establish a guideline like "you can always assume an 
operation has been validated before the authorization function is called" or 
vice-versa. Maybe it's inconsequential, since as the code exists currently it 
doesn't really matter in what order the two operations are performed.


- Greg


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


On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> ---
> 
> (Updated Nov. 30, 2015, 8:35 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 

Re: Review Request 40620: Windows: Added suppport for `slave/monitor.cpp'.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40114, 40620]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 1, 2015, 9:02 p.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40620/
> ---
> 
> (Updated Dec. 1, 2015, 9:02 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added suppport for `slave/monitor.cpp'.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt cfe9d26c45ba42852fd1af958549954e7b04d448 
> 
> Diff: https://reviews.apache.org/r/40620/diff/
> 
> 
> Testing
> ---
> 
> OSX: make
> Windows: make
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 40224: Fix wrong flags infos in /state and /flags

[Jian Qiu]

> On Nov. 23, 2015, 4:08 p.m., haosdent huang wrote:
> > I think keep the sync of these files when flags add or delete Protobuf 
> > fields a bit hard. Is it possible to change flags stringify protobuf 
> > objects as json string

Thanks hasdent. I do not think we can overload or have a specialized stringify 
for google::protobuf::Message since it is an abstract class. How about 
rewriting the operator<< overload for each protobuf flag? We can change 
https://github.com/apache/mesos/blob/master/include/mesos/authorizer/authorizer.hpp#L138
 to "return stream << JSON::protobuf(acls)". However, it still needs change for 
each protobuf flag. Any thought?


- Jian


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


On Nov. 23, 2015, 5:56 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40224/
> ---
> 
> (Updated Nov. 23, 2015, 5:56 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-3792
> https://issues.apache.org/jira/browse/MESOS-3792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix wrong flags infos in /state and /flags. We should convert protobuf type 
> flag to JSON object explicitly instead of relying on stringify
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp b0bec97ee69413bb70c2673c4ae49e74988796bf 
> 
> Diff: https://reviews.apache.org/r/40224/diff/
> 
> 
> Testing
> ---
> 
> make & make check
> 
> ./mesos-master.sh --work_dir=/tmp/mesos --acls='{"register_frameworks": 
> [{"principals": { "type": "ANY" },"roles": { "values": ["a"] }}],"run_tasks": 
> [{"principals": { "values": ["a", "b"] },"users": { "values": ["c"] 
> }}],"shutdown_frameworks": [{"principals": { "values": ["a", "b"] 
> },"framework_principals": { "values": ["c"] }}]}'
> 
> curl http://master:5050/state
> curl http://master:5050/flag
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.

[Jie Yu]

> On Dec. 1, 2015, 2:24 p.m., Michael Park wrote:
> > src/master/master.cpp, line 2771
> > 
> >
> > Why is it that we need to perform validation within authorization? We 
> > perform `authorizeTask` with a `TaskInfo` that has not been validated yet, 
> > and take care of it later on. Does that pattern not work here?
> 
> Greg Mann wrote:
> That pattern would work here, and the original patches I posted worked 
> that way. We ended up switching to this solution because it makes for a 
> cleaner implementation and eliminates some redundant code. However, it does 
> come at the cost of making error messaging more difficult (as you noted in 
> another patch) and decreasing the separation of functionality that can make 
> debugging easier. I don't have a strong preference for either path at the 
> moment. Perhaps Jie could chime in with his thoughts?
> 
> Jie Yu wrote:
> I looked at the code again. I guess the reason we validate the operation 
> before authorization is because we want to get all the principals from 
> resources in UNRESERVE, and we want to make sure resources in UNRESERVE 
> operation is valid. As MPark pointed out, coupling authorization with 
> validation does cause some confusion when generating error messages. How 
> about the following:
> 
> 1) we don't perform validation in authorization. In 
> authorizeUnreserveResources, when we iterate all resources, we can do the 
> following as you previosly did (sorry about the back and forth, my bad).
> ```
> foreach (const Resource& resource, unreserve.resources()) {
>   if (Resources::isDynamicallyReserved(resource)) {
> request.mutable_reserver_principals()->add_values(
> resource.reservation().principal());
>   }
> }
> ```
> 
> 2) in Http::reserve and Http::unreserve, we still perform validation 
> first before calling master->authroizeXXX.
> 
> 3) in `Master::_accept`, we perform operation validation on 
> RESERVE/UNRESERVE.
> 
> Let me know if that's better or not?
> 
> Greg Mann wrote:
> No worries, Jie! I learned about `Future<>::repair()` in the process so I 
> consider it worthwhile :-)
> 
> I have one question about your plan: for `LAUNCH` operations, tasks are 
> authorized before they get validated, and I'm wondering if it's worth making 
> this part of the interface consistent across all types of operations, i.e. 
> perhaps we should change the code for `LAUNCH` to validate before 
> authorization as well. If we decide to do this, I could do it here or in 
> another review.
> 
> Greg Mann wrote:
> Since the validation for `LAUNCH` needs to be aware of tasks that were 
> previously launched on the same operation, it makes sense in that case for 
> validation to occur after authorization. However, in general, I think it 
> makes more sense for the operation to be validated before authorization, so 
> I'll follow the pattern that Jie suggested above for 
> `RESERVE/UNRESERVE/CREATE/DESTROY`. This means that the interface will have 
> slightly different expectations regarding validation depending on what type 
> of operation is being performed.
> 
> An alternative would be to validate all operations *after* authorization 
> is performed, which in general seems less intuitive to me, but would keep the 
> interface consistent...
> 
> Jie Yu wrote:
> Greg, the code sample I suggested above actually tried to keep the 
> interface consistent (validation is done separately and is decoupled from 
> authorization). Just want to double check with you.
> 
> Greg Mann wrote:
> Thanks for checking Jie, I think I misread your comments previously.
> 
> Your suggested solution would keep the separation of validation and 
> authorization consistent, the difference is that for HTTP endpoint operations 
> validation would be done *before* authorization, while for framework 
> operations validation would be done *after* authorization. I guess the 
> question is, do we think it's worthwhile to establish a guideline like "you 
> can always assume an operation has been validated before the authorization 
> function is called" or vice-versa. Maybe it's inconsequential, since as the 
> code exists currently it doesn't really matter in what order the two 
> operations are performed.

Aha, now I get it. Thanks for explaining! Yeah, I think I am fine with either 
way. You can swap the order in http endpoint if the change is easy. Otherwise, 
I would probably just leave it as it is.


- Jie


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


On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically 

Review Request 40849: [WIP] Fix flaky MemoryPressureMesosTests

[Joseph Wu]

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

Review request for mesos, Bernd Mathiske, Artem Harutyunyan, and Till Toenshoff.


Bugs: MESOS-3586
https://issues.apache.org/jira/browse/MESOS-3586


Repository: mesos


Description
---

The existing tests will check that "low" pressure events occur at least as 
often as "medium" pressure events (this is the documented behavior).  However, 
the order of events and the order in which we process said events is not 
guaranteed.  When we collect the pressure events via a counter, there may be 
some events that are in-flight, and thereby not accounted for in the counters.

This patch modifies MemoryPressureMesosTests to wait for memory pressure events 
to stop before checking the counts for correctness.
The tests now stop the memory-pressure-triggering task and then wait for all 
events to be processed before checking the counters.


Diffs
-

  src/tests/containerizer/memory_pressure_tests.cpp 
e18b971c4df26c9b9c103ca73bdad4fd400d6c02 

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


Testing
---

On Debian 8:
`make check`
`sudo bin/mesos-tests.sh --gtest_filter="*MemoryPressure*" --gtest_repeat=-1 
--gtest_break_on_failure`

^ Ran the above for a couple minutes (it was previously failing ~1/5 times).

TODO: Need to run the test on other platforms it is known to be failing on.
i.e. Ubuntu 14, Centos 6, Centos 7, others?


Thanks,

Joseph Wu

Re: Review Request 40829: Improved authorization documentation.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40829]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 1, 2015, 7:10 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40829/
> ---
> 
> (Updated Dec. 1, 2015, 7:10 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved authorization documentation.
> 
> Removed reference to deprecated endpoint, avoided line wrapping in JSON
> examples, and made various other fixes.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md f5ed75fcd0785fde38058917354fcf6d668dcccb 
> 
> Diff: https://reviews.apache.org/r/40829/diff/
> 
> 
> Testing
> ---
> 
> Previewed using site-docker.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40115: Windows: Added support for `slave/gc.cpp'.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [40114, 39537, 39538, 39539, 39540, 39541, 39383, 39559, 39219]

Failed command: ./support/apply-review.sh -n -r 39219

Error:
 2015-12-02 02:20:21 URL:https://reviews.apache.org/r/39219/diff/raw/ 
[3137/3137] -> "39219.patch" [1]
error: patch failed: src/slave/state.cpp:16
error: src/slave/state.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Dec. 1, 2015, 9:06 p.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40115/
> ---
> 
> (Updated Dec. 1, 2015, 9:06 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added support for `slave/gc.cpp'.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt cfe9d26c45ba42852fd1af958549954e7b04d448 
>   src/slave/gc.cpp 7a8c69b4410df46ca8fd6ac009cc14e8fe5ff6d3 
> 
> Diff: https://reviews.apache.org/r/40115/diff/
> 
> 
> Testing
> ---
> 
> Windows 10: make.bat
> OSX: make check
> Ubuntu: 15.1 make check
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 40849: [WIP] Fix flaky MemoryPressureMesosTests

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40849]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 2, 2015, 12:58 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40849/
> ---
> 
> (Updated Dec. 2, 2015, 12:58 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Artem Harutyunyan, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3586
> https://issues.apache.org/jira/browse/MESOS-3586
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The existing tests will check that "low" pressure events occur at least as 
> often as "medium" pressure events (this is the documented behavior).  
> However, the order of events and the order in which we process said events is 
> not guaranteed.  When we collect the pressure events via a counter, there may 
> be some events that are in-flight, and thereby not accounted for in the 
> counters.
> 
> This patch modifies MemoryPressureMesosTests to wait for memory pressure 
> events to stop before checking the counts for correctness.
> The tests now stop the memory-pressure-triggering task and then wait for all 
> events to be processed before checking the counters.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> e18b971c4df26c9b9c103ca73bdad4fd400d6c02 
> 
> Diff: https://reviews.apache.org/r/40849/diff/
> 
> 
> Testing
> ---
> 
> On Debian 8:
> `make check`
> `sudo bin/mesos-tests.sh --gtest_filter="*MemoryPressure*" --gtest_repeat=-1 
> --gtest_break_on_failure`
> 
> ^ Ran the above for a couple minutes (it was previously failing ~1/5 times).
> 
> TODO: Need to run the test on other platforms it is known to be failing on.
> i.e. Ubuntu 14, Centos 6, Centos 7, others?
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 40818: Fixed flaky test (AvailableResourcesAfterRescinding).

[Alexander Rukletsov]

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

(Updated Dec. 1, 2015, 4:52 p.m.)


Review request for mesos, Joris Van Remoortere and Vinod Kone.


Changes
---

Removed non-ascii chars. Courtesy of Neil and 
https://reviews.apache.org/r/40799/


Repository: mesos


Description
---

Addendum to b2b0eedc7371cd294b715fc91312989919d57c7a


Diffs (updated)
-

  src/tests/master_quota_tests.cpp 481298361574008b2cf9d213c908deb633393e31 

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


Testing
---

Two test setups on Mac OS X 10.10.4.

`GTEST_FILTER="*AvailableResourcesAfterRescinding*" ./bin/mesos-tests.sh 
--gtest_repeat=1000` *with* `allocation_interval` set to `50ms`
The probability of a batch allocation between rescinding and `Shutdown()` is 
low.


`GTEST_FILTER="*AvailableResourcesAfterRescinding*" ./bin/mesos-tests.sh 
--gtest_repeat=1000` *with* simulation of slow CI
A batch allocation between rescinding and `Shutdown()` is triggered by 
inserting the following lines after resources are recovered:
```
  Clock::pause();
  Clock::advance(flags.allocation_timeout);
  Clock::settle();
  Clock::resume();
```


Thanks,

Alexander Rukletsov

Re: Review Request 40799: Removed non-ASCII characters from test case comment.

[Alexander Rukletsov]

> On Dec. 1, 2015, 1:49 p.m., Alexander Rukletsov wrote:
> > I propose to replace the comment in https://reviews.apache.org/r/40818/
> > 
> > Just curious, how did you spot this? it looks fine in my editor. Also, is 
> > it something we should add as a commit hook?
> 
> Neil Conway wrote:
> Hi Alex -- note that the revised comment in 
> https://reviews.apache.org/r/40818/ still has non-ASCII characters.
> 
> I noticed the characters when looking at "git log -p" for some unrelated 
> (now forgotten) purpose.
> 
> I agree this is something we should add as a commit hook.
> 
> Alexander Rukletsov wrote:
> You are right — I've updated the patch. I'll also file a JIRA ticket for 
> adding a hook.

Anyway, I think this patch can be committed, since I do not know what path we 
will choose for fixing the flaky test.


- Alexander


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


On Nov. 30, 2015, 6:43 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40799/
> ---
> 
> (Updated Nov. 30, 2015, 6:43 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed non-ASCII characters from test case comment.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp 481298361574008b2cf9d213c908deb633393e31 
> 
> Diff: https://reviews.apache.org/r/40799/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40799: Removed non-ASCII characters from test case comment.

[Alexander Rukletsov]

> On Dec. 1, 2015, 1:49 p.m., Alexander Rukletsov wrote:
> > I propose to replace the comment in https://reviews.apache.org/r/40818/
> > 
> > Just curious, how did you spot this? it looks fine in my editor. Also, is 
> > it something we should add as a commit hook?
> 
> Neil Conway wrote:
> Hi Alex -- note that the revised comment in 
> https://reviews.apache.org/r/40818/ still has non-ASCII characters.
> 
> I noticed the characters when looking at "git log -p" for some unrelated 
> (now forgotten) purpose.
> 
> I agree this is something we should add as a commit hook.

You are right — I've updated the patch. I'll also file a JIRA ticket for adding 
a hook.


- Alexander


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


On Nov. 30, 2015, 6:43 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40799/
> ---
> 
> (Updated Nov. 30, 2015, 6:43 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed non-ASCII characters from test case comment.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp 481298361574008b2cf9d213c908deb633393e31 
> 
> Diff: https://reviews.apache.org/r/40799/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40631]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 1, 2015, 1:13 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40631/
> ---
> 
> (Updated Dec. 1, 2015, 1:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-3963
> https://issues.apache.org/jira/browse/MESOS-3963
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> According to the google code style, the using should be used in internal 
> namespace in header files. Grep the header files, only fetcher.hpp deserved a 
> path.
> 
> 
> > You may use a using-declaration anywhere in a .cc file (including in the 
> > global namespace), and in functions, methods, classes, or within internal 
> > namespaces in .h files.
> 
> >Do not use using-declarations in .h files except in explicitly marked 
> >internal-only namespaces, because anything imported into a namespace in a .h 
> >file becomes part of the public API exported by that file.
> 
> ```
> // OK in .cc files.
> // Must be in a function, method, internal namespace, or
> // class in .h files.
> using ::foo::bar;
> ```
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.hpp 78e7d14 
>   src/tests/mesos.hpp a2a76f5 
> 
> Diff: https://reviews.apache.org/r/40631/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 40746: Quota: Removed quota from registry for remove request.

[Joseph Wu]

> On Nov. 30, 2015, 10:47 a.m., Joseph Wu wrote:
> > src/master/quota_handler.cpp, lines 393-394
> > 
> >
> > `RemoveQuota` appears idempotent, so what is the problem with trying to 
> > remove a quota that is currently being removed?
> > 
> > Is it the fact that you check this?
> > ```
> >   // Check that we are removing an existing quota.
> >   if (!master->quotas.contains(role)) { ... }
> > ```
> 
> Alexander Rukletsov wrote:
> `RemoveQuota` is idempotent, right, but 
> `HierarchicalAllocatorProcess::removeQuota()` is not. I think we can change 
> the whole function to be idempotent, but is this really necessary?

No, not really.  

I was just wondering what the decision behind the non-idempotency, and if it 
would simplify any logic by doing so :)


- Joseph


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


On Nov. 26, 2015, 4:28 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40746/
> ---
> 
> (Updated Nov. 26, 2015, 4:28 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-4021
> https://issues.apache.org/jira/browse/MESOS-4021
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/quota_handler.cpp e9c8791d4bd00e6c1be1844f27d9bee26f722a9b 
> 
> Diff: https://reviews.apache.org/r/40746/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.

[Jie Yu]

> On Dec. 1, 2015, 2:24 p.m., Michael Park wrote:
> > src/master/master.cpp, line 2771
> > 
> >
> > Why is it that we need to perform validation within authorization? We 
> > perform `authorizeTask` with a `TaskInfo` that has not been validated yet, 
> > and take care of it later on. Does that pattern not work here?
> 
> Greg Mann wrote:
> That pattern would work here, and the original patches I posted worked 
> that way. We ended up switching to this solution because it makes for a 
> cleaner implementation and eliminates some redundant code. However, it does 
> come at the cost of making error messaging more difficult (as you noted in 
> another patch) and decreasing the separation of functionality that can make 
> debugging easier. I don't have a strong preference for either path at the 
> moment. Perhaps Jie could chime in with his thoughts?

I looked at the code again. I guess the reason we validate the operation before 
authorization is because we want to get all the principals from resources in 
UNRESERVE, and we want to make sure resources in UNRESERVE operation is valid. 
As MPark pointed out, coupling authorization with validation does cause some 
confusion when generating error messages. How about the following:

1) we don't perform validation in authorization. In 
authorizeUnreserveResources, when we iterate all resources, we can do the 
following as you previosly did (sorry about the back and forth, my bad).
```
foreach (const Resource& resource, unreserve.resources()) {
  if (Resources::isDynamicallyReserved(resource)) {
request.mutable_reserver_principals()->add_values(
resource.reservation().principal());
  }
}
```

2) in Http::reserve and Http::unreserve, we still perform validation first 
before calling master->authroizeXXX.

3) in `Master::_accept`, we perform operation validation on RESERVE/UNRESERVE.

Let me know if that's better or not?


- Jie


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


On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> ---
> 
> (Updated Nov. 30, 2015, 8:35 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
> 
> Diff: https://reviews.apache.org/r/39987/diff/
> 
> 
> Testing
> ---
> 
> This is the third in a chain of 5 patches. `make check` was used to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40821: Quota: Ensured the sorter for quota'ed roles does not contain revocable resources.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40586, 40551, 40332, 40795, 39450, 40797, 40821]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 1, 2015, 2:09 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40821/
> ---
> 
> (Updated Dec. 1, 2015, 2:09 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 1cd8d16661568010901e74705375e7719cdfb8a0 
>   src/master/allocator/mesos/hierarchical.cpp 
> b8f3be8e65f9be68a78c61a958903d3e36910efb 
> 
> Diff: https://reviews.apache.org/r/40821/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.

[Greg Mann]

> On Dec. 1, 2015, 2:24 p.m., Michael Park wrote:
> > src/master/master.cpp, line 2771
> > 
> >
> > Why is it that we need to perform validation within authorization? We 
> > perform `authorizeTask` with a `TaskInfo` that has not been validated yet, 
> > and take care of it later on. Does that pattern not work here?
> 
> Greg Mann wrote:
> That pattern would work here, and the original patches I posted worked 
> that way. We ended up switching to this solution because it makes for a 
> cleaner implementation and eliminates some redundant code. However, it does 
> come at the cost of making error messaging more difficult (as you noted in 
> another patch) and decreasing the separation of functionality that can make 
> debugging easier. I don't have a strong preference for either path at the 
> moment. Perhaps Jie could chime in with his thoughts?
> 
> Jie Yu wrote:
> I looked at the code again. I guess the reason we validate the operation 
> before authorization is because we want to get all the principals from 
> resources in UNRESERVE, and we want to make sure resources in UNRESERVE 
> operation is valid. As MPark pointed out, coupling authorization with 
> validation does cause some confusion when generating error messages. How 
> about the following:
> 
> 1) we don't perform validation in authorization. In 
> authorizeUnreserveResources, when we iterate all resources, we can do the 
> following as you previosly did (sorry about the back and forth, my bad).
> ```
> foreach (const Resource& resource, unreserve.resources()) {
>   if (Resources::isDynamicallyReserved(resource)) {
> request.mutable_reserver_principals()->add_values(
> resource.reservation().principal());
>   }
> }
> ```
> 
> 2) in Http::reserve and Http::unreserve, we still perform validation 
> first before calling master->authroizeXXX.
> 
> 3) in `Master::_accept`, we perform operation validation on 
> RESERVE/UNRESERVE.
> 
> Let me know if that's better or not?

No worries, Jie! I learned about `Future<>::repair()` in the process so I 
consider it worthwhile :-)

I have one question about your plan: for `LAUNCH` operations, tasks are 
authorized before they get validated, and I'm wondering if it's worth making 
this part of the interface consistent across all types of operations, i.e. 
perhaps we should change the code for `LAUNCH` to validate before authorization 
as well. If we decide to do this, I could do it here or in another review.


- Greg


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


On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> ---
> 
> (Updated Nov. 30, 2015, 8:35 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
> 
> Diff: https://reviews.apache.org/r/39987/diff/
> 
> 
> Testing
> ---
> 
> This is the third in a chain of 5 patches. `make check` was used to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40822: Updated the pattern for `_teardown` authorization continuation.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Dec. 1, 2015, 3:16 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40822/
> ---
> 
> (Updated Dec. 1, 2015, 3:16 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Motivated by https://reviews.apache.org/r/39988/#comment167856
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 9d729ef7f7d7ad6185934648f833e4f8a4f0a123 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
> 
> Diff: https://reviews.apache.org/r/40822/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 40332: Quota: Implemented recovery in hierarchical allocator.

[Qian Zhang]

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

Ship it!


Ship It!

- Qian Zhang


On Nov. 30, 2015, 11:32 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40332/
> ---
> 
> (Updated Nov. 30, 2015, 11:32 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3981
> https://issues.apache.org/jira/browse/MESOS-3981
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 1cd8d16661568010901e74705375e7719cdfb8a0 
>   src/master/allocator/mesos/hierarchical.cpp 
> 31ed62efb5b1a2edb567f43d37559c5914e0665e 
> 
> Diff: https://reviews.apache.org/r/40332/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40795: Quota: Properly initialized the sorter for quota'ed roles in the allocator.

[Qian Zhang]

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

Ship it!


Ship It!

- Qian Zhang


On Nov. 30, 2015, 11:32 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40795/
> ---
> 
> (Updated Nov. 30, 2015, 11:32 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 31ed62efb5b1a2edb567f43d37559c5914e0665e 
> 
> Diff: https://reviews.apache.org/r/40795/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40829: Improved authorization documentation.

[Neil Conway]

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

(Updated Dec. 1, 2015, 7:10 p.m.)


Review request for mesos and Adam B.


Summary (updated)
-

Improved authorization documentation.


Repository: mesos


Description
---

Improved authorization documentation.

Removed reference to deprecated endpoint, avoided line wrapping in JSON
examples, and made various other fixes.


Diffs
-

  docs/authorization.md f5ed75fcd0785fde38058917354fcf6d668dcccb 

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


Testing
---

Previewed using site-docker.


Thanks,

Neil Conway

Re: Review Request 40811: Fixed flakiness in tests using the Scheduler library

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40811]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 1, 2015, 3:38 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40811/
> ---
> 
> (Updated Dec. 1, 2015, 3:38 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4029
> https://issues.apache.org/jira/browse/MESOS-4029
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the `event` queue object was defined after the `callbacks` 
> object. However, the `callbacks` object held on a reference to the queue 
> object. This led to a crash in some scenarios when we received another event 
> from the master after the `event` object was destroyed. (Note that objects on 
> the stack are destroyed in the reverse order of definition)
> 
> The short term fix is to move the `event` queue object above the `callbacks` 
> object. This ensures that it is destroyed after the `callbacks` object. The 
> long term fix would be worked on via `MESOS-3339` that moves the tests to a 
> callback interface model similar to what we have for the driver tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> ade05f932020013ced19c1573be756a029396fac 
>   src/tests/scheduler_tests.cpp 67432109c7df6be0aa76e94a03bd5b2e9c96d14e 
> 
> Diff: https://reviews.apache.org/r/40811/diff/
> 
> 
> Testing
> ---
> 
> make check + Reproduced this by introducing a sleep in the `Mesos` destructor 
> + did not filter out the `HEARTBEAT` event.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Review Request 40829: Fixed reference to deprecated endpoint in docs.

[Neil Conway]

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

Review request for mesos and Adam B.


Repository: mesos


Description
---

Improved authorization documentation.

Removed reference to deprecated endpoint, avoided line wrapping in JSON
examples, and made various other fixes.


Diffs
-

  docs/authorization.md f5ed75fcd0785fde38058917354fcf6d668dcccb 

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


Testing
---

Previewed using site-docker.


Thanks,

Neil Conway

Re: Review Request 37853: Overlay filesystem provisioning backend

[Jie Yu]

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



src/Makefile.am (line 776)


Please align the backslashes. We use tabs (8 spaces) in Makefiles



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (lines 59 - 68)


This code is copy-pasted in test as well. Can we add a utility funciton in 
src/linux/fs.hpp|cpp instead?

```
namespace fs {
Try supported(const string& filesystem);
}
```



src/tests/containerizer/provisioner_backend_tests.cpp (lines 56 - 65)


See my comments above. Let's add a new test filter and skip this test if 
overlayfs is not supported. See examples in src/tests/environment.cpp



src/tests/containerizer/provisioner_backend_tests.cpp (line 109)


2 lines apart.


- Jie Yu


On Dec. 1, 2015, 5:26 a.m., Mei Wan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37853/
> ---
> 
> (Updated Dec. 1, 2015, 5:26 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2971
> https://issues.apache.org/jira/browse/MESOS-2971
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented the overlay filesystem backend by layering the images as a 
> read-only filesystem.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt cfe9d26 
>   src/Makefile.am fd38cfa 
>   src/slave/containerizer/mesos/provisioner/backend.cpp 01d06eb 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_backend_tests.cpp 25b28ef 
> 
> Diff: https://reviews.apache.org/r/37853/diff/
> 
> 
> Testing
> ---
> 
> Basic test in provisioner_backend_tests.cpp.
> 
> 
> Thanks,
> 
> Mei Wan
> 
>

Re: Review Request 40806: Made HDFS::exists asynchronous.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40250, 40251, 40252, 40274, 40253, 40305, 40403, 40418, 
40556, 40461, 40462, 40463, 40464, 40498, 40559, 40806]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 1, 2015, 1:21 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40806/
> ---
> 
> (Updated Dec. 1, 2015, 1:21 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made HDFS::exists asynchronous.
> 
> This also fixed a bug in the orignal code: the original code never returns 
> false.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp ac933b6a8636af3356f3d6a99656c0185077c262 
>   src/hdfs/hdfs.hpp 6bdeedf8934a681b32fca8edc251488f4213dd07 
>   src/hdfs/hdfs.cpp c32c2ae9d2ace5deb09f56dcec2bd366c2d07f6b 
> 
> Diff: https://reviews.apache.org/r/40806/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 40808: Made the order of `EXPECT_CALL()`s match the expected order of events.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40808]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 1, 2015, 1:58 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40808/
> ---
> 
> (Updated Dec. 1, 2015, 1:58 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Similar to 5820cd3627ed01e5ede7; unfortunately, that commit missed a few
> instances of this pattern.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 
> 
> Diff: https://reviews.apache.org/r/40808/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40806: Made HDFS::exists asynchronous.

[Bernd Mathiske]

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



src/hdfs/hdfs.cpp (line 82)


s/exec/execute/

'to exec' seems to indicate that the exec syscall failed. That may not 
always be the cause for the failure.



src/hdfs/hdfs.cpp (line 100)


This block seems to be reusable in a lot of situations. Any chance we could 
abstract it and make it part of class Subprocess?

In a separate ticket, I suppose. ==> Place a TODO here?


- Bernd Mathiske


On Nov. 30, 2015, 5:21 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40806/
> ---
> 
> (Updated Nov. 30, 2015, 5:21 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made HDFS::exists asynchronous.
> 
> This also fixed a bug in the orignal code: the original code never returns 
> false.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp ac933b6a8636af3356f3d6a99656c0185077c262 
>   src/hdfs/hdfs.hpp 6bdeedf8934a681b32fca8edc251488f4213dd07 
>   src/hdfs/hdfs.cpp c32c2ae9d2ace5deb09f56dcec2bd366c2d07f6b 
> 
> Diff: https://reviews.apache.org/r/40806/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 40379: MESOS-3930: Set resource type as USAGE_SLACK for Oversubscription

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40375, 40379]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Nov. 25, 2015, 6:41 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40379/
> ---
> 
> (Updated Nov. 25, 2015, 6:41 a.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-3930
> https://issues.apache.org/jira/browse/MESOS-3930
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In Optimistic Offer Phase 1, it introduce `RevocableInfo::type`: USAGE_SLACK 
> for Oversubscription and ALLOCATION_SLACK for Optimistic Offer. Slave helps 
> to update `RevocableInfo::type` for Oversubscription.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 9055f2a 
> 
> Diff: https://reviews.apache.org/r/40379/diff/
> 
> 
> Testing
> ---
> 
> make (make check is on-going)
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 37853: Overlay filesystem provisioning backend

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37853]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 1, 2015, 5:26 a.m., Mei Wan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37853/
> ---
> 
> (Updated Dec. 1, 2015, 5:26 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2971
> https://issues.apache.org/jira/browse/MESOS-2971
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented the overlay filesystem backend by layering the images as a 
> read-only filesystem.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt cfe9d26 
>   src/Makefile.am fd38cfa 
>   src/slave/containerizer/mesos/provisioner/backend.cpp 01d06eb 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_backend_tests.cpp 25b28ef 
> 
> Diff: https://reviews.apache.org/r/37853/diff/
> 
> 
> Testing
> ---
> 
> Basic test in provisioner_backend_tests.cpp.
> 
> 
> Thanks,
> 
> Mei Wan
> 
>

Review Request 40822: Updated the pattern for `_teardown` authorization continuation.

[Michael Park]

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

Review request for mesos, Greg Mann and Jie Yu.


Repository: mesos


Description
---

Motivated by https://reviews.apache.org/r/39988/#comment167856


Diffs
-

  src/master/http.cpp 9d729ef7f7d7ad6185934648f833e4f8a4f0a123 
  src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 

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


Testing
---

`make check`


Thanks,

Michael Park

Re: Review Request 39988: [4/5] Added authorization for dynamic reservation master endpoints.

[Michael Park]

> On Nov. 30, 2015, 9:44 p.m., Jie Yu wrote:
> > src/master/http.cpp, line 2313
> > 
> >
> > Instead of adding a new parameter to `_operation`. Can we put the 
> > logics of checking authorization result to `_reserve` and `_unreserve`? 
> > It's wiered to me that `_operation` needs to check authorization result.
> > 
> > ```
> > auto _unreserve = [=](bool authorized) {
> >   if (!authorized) {
> > return Unauthorized("Mesos master");
> >   }
> >   
> >   return _operation(slaveId, resources, operation);
> > };
> > 
> > return master->authorizeUnreserveResources(...)
> >   .then(defer(master->self(), _unreserve))
> >   .repair([](const Future& future) {
> > return BadRequest("Invalid UNRESERVE operation: " + 
> > future.failure());
> >   });
> > ```

I think `teardown` can benefit from this pattern as well. I've tried it out to 
see how it would fit: https://reviews.apache.org/r/40822/


- Michael


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


On Nov. 30, 2015, 8:46 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39988/
> ---
> 
> (Updated Nov. 30, 2015, 8:46 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization for dynamic reservation master endpoints.
> Note: this review is continued from https://reviews.apache.org/r/37126/
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 9d729ef7f7d7ad6185934648f833e4f8a4f0a123 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
>   src/tests/reservation_endpoints_tests.cpp 
> f30ff8bc6a3e9773437fa7fd7c8f569b7d7e2d9d 
> 
> Diff: https://reviews.apache.org/r/39988/diff/
> 
> 
> Testing
> ---
> 
> This is the fourth in a chain of 5 patches. Added new reservation endpoints 
> tests to validate authorization of reserve and unreserve operations using 
> ACLs. `make check` was run to test after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40799: Removed non-ASCII characters from test case comment.

[Neil Conway]

> On Dec. 1, 2015, 1:49 p.m., Alexander Rukletsov wrote:
> > I propose to replace the comment in https://reviews.apache.org/r/40818/
> > 
> > Just curious, how did you spot this? it looks fine in my editor. Also, is 
> > it something we should add as a commit hook?

Hi Alex -- note that the revised comment in https://reviews.apache.org/r/40818/ 
still has non-ASCII characters.

I noticed the characters when looking at "git log -p" for some unrelated (now 
forgotten) purpose.

I agree this is something we should add as a commit hook.


- Neil


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


On Nov. 30, 2015, 6:43 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40799/
> ---
> 
> (Updated Nov. 30, 2015, 6:43 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed non-ASCII characters from test case comment.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp 481298361574008b2cf9d213c908deb633393e31 
> 
> Diff: https://reviews.apache.org/r/40799/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40818: Fixed flaky test (AvailableResourcesAfterRescinding).

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40818]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 1, 2015, 11:42 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40818/
> ---
> 
> (Updated Dec. 1, 2015, 11:42 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addendum to b2b0eedc7371cd294b715fc91312989919d57c7a
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp 481298361574008b2cf9d213c908deb633393e31 
> 
> Diff: https://reviews.apache.org/r/40818/diff/
> 
> 
> Testing
> ---
> 
> Two test setups on Mac OS X 10.10.4.
> 
> `GTEST_FILTER="*AvailableResourcesAfterRescinding*" ./bin/mesos-tests.sh 
> --gtest_repeat=1000` *with* `allocation_interval` set to `50ms`
> The probability of a batch allocation between rescinding and `Shutdown()` is 
> low.
> 
> 
> `GTEST_FILTER="*AvailableResourcesAfterRescinding*" ./bin/mesos-tests.sh 
> --gtest_repeat=1000` *with* simulation of slow CI
> A batch allocation between rescinding and `Shutdown()` is triggered by 
> inserting the following lines after resources are recovered:
> ```
>   Clock::pause();
>   Clock::advance(flags.allocation_timeout);
>   Clock::settle();
>   Clock::resume();
> ```
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Klaus Ma]

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

(Updated Dec. 1, 2015, 8:51 p.m.)


Review request for mesos, Benjamin Bannier, Joseph Wu, and Michael Park.


Changes
---

Add comments for `RevocableInfo::Type`


Bugs: MESOS-3963
https://issues.apache.org/jira/browse/MESOS-3963


Repository: mesos


Description
---

According to the google code style, the using should be used in internal 
namespace in header files. Grep the header files, only fetcher.hpp deserved a 
path.


> You may use a using-declaration anywhere in a .cc file (including in the 
> global namespace), and in functions, methods, classes, or within internal 
> namespaces in .h files.

>Do not use using-declarations in .h files except in explicitly marked 
>internal-only namespaces, because anything imported into a namespace in a .h 
>file becomes part of the public API exported by that file.

```
// OK in .cc files.
// Must be in a function, method, internal namespace, or
// class in .h files.
using ::foo::bar;
```


Diffs (updated)
-

  include/mesos/mesos.proto 27971fe 
  include/mesos/v1/mesos.proto 9acefd5 
  src/common/resources.cpp 98804a4 
  src/tests/resources_tests.cpp dbd39cd 
  src/v1/resources.cpp 8488c31 

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


Testing
---

make && make check


Thanks,

Klaus Ma

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Michael Park]

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



src/slave/containerizer/fetcher.hpp (line 41)


I agree with the intent to remove the using declaration out of 
`fetcher.hpp`, but I don't agree with moving it into an internal namespace. 
Fetcher-related cpp files should pull it in itself; `src/launcher/fetcher.cpp` 
for example already does so.

I strongly disagree with introducing a using declaration to 
`src/tests/mesos.hpp`. It doesn't seem to be something that should be common 
across all tests. It should be local to fetcher-related tests; 
`src/tests/fetcher_tests.cpp` for example already pulls it in itself.

My suggestion would be to use using declarations in cpp files, and use 
qualified names in hpp files. We end up with the following changes:

(1) Add `using mesos::fetcher::FetcherInfo;` in 
`src/slave/containerizer/fetcher.cpp` and `src/tests/fetcher_cache_tests.cpp`.
(2) `s/FetcherInfo/fetcher::FetcherInfo/` for 1 instance in 
`src/slave/containerizer/fetcher.hpp` and 3 instances in `src/tests/mesos.cpp`.


- Michael Park


On Dec. 1, 2015, 12:51 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40631/
> ---
> 
> (Updated Dec. 1, 2015, 12:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-3963
> https://issues.apache.org/jira/browse/MESOS-3963
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> According to the google code style, the using should be used in internal 
> namespace in header files. Grep the header files, only fetcher.hpp deserved a 
> path.
> 
> 
> > You may use a using-declaration anywhere in a .cc file (including in the 
> > global namespace), and in functions, methods, classes, or within internal 
> > namespaces in .h files.
> 
> >Do not use using-declarations in .h files except in explicitly marked 
> >internal-only namespaces, because anything imported into a namespace in a .h 
> >file becomes part of the public API exported by that file.
> 
> ```
> // OK in .cc files.
> // Must be in a function, method, internal namespace, or
> // class in .h files.
> using ::foo::bar;
> ```
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 27971fe 
>   include/mesos/v1/mesos.proto 9acefd5 
>   src/common/resources.cpp 98804a4 
>   src/tests/resources_tests.cpp dbd39cd 
>   src/v1/resources.cpp 8488c31 
> 
> Diff: https://reviews.apache.org/r/40631/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 40379: MESOS-3930: Set resource type as USAGE_SLACK for Oversubscription

[Klaus Ma]

> On Dec. 1, 2015, 10:10 a.m., Joseph Wu wrote:
> > src/slave/slave.cpp, lines 4464-4466
> > 
> >
> > Can you add a comment about why this is purposefully and unilaterally 
> > done here (potentially overwriting what the ResourceEstimator has given 
> > you)?
> > 
> > It may also be a good idea to update the documentation in 
> > "include/mesos/slave/resource_estimator.hpp".

Sure, I'll add some comments for it, for the document, can we address them in 
MESOS-3889?
There're two reason to add this code:
1. I'd like to `set_type()` accordingly, so we can make `RevocalbeInfo::type` 
to be `required`
2. I'd like `ResourceEstimator` return the right type


- Klaus


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


On Nov. 25, 2015, 2:41 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40379/
> ---
> 
> (Updated Nov. 25, 2015, 2:41 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-3930
> https://issues.apache.org/jira/browse/MESOS-3930
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In Optimistic Offer Phase 1, it introduce `RevocableInfo::type`: USAGE_SLACK 
> for Oversubscription and ALLOCATION_SLACK for Optimistic Offer. Slave helps 
> to update `RevocableInfo::type` for Oversubscription.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 9055f2a 
> 
> Diff: https://reviews.apache.org/r/40379/diff/
> 
> 
> Testing
> ---
> 
> make (make check is on-going)
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 39985: [1/5] Introduced ACL protobuf definitions for dynamic reservation.

[Michael Park]

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

Ship it!


Ship It!

- Michael Park


On Nov. 30, 2015, 8:32 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39985/
> ---
> 
> (Updated Nov. 30, 2015, 8:32 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced ACL protobuf definitions for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37002/
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 8e72003f405770f00c5d87f318a9e1a8ed7430ee 
> 
> Diff: https://reviews.apache.org/r/39985/diff/
> 
> 
> Testing
> ---
> 
> This is the first in a chain of 5 patches. `make check` was used to test at 
> the end of the chain.
> 
> Note: documentation for these changes is included in 
> https://reviews.apache.org/r/40271/
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40754: Fixed flaky test (AvailableResourcesAfterRescinding).

[Alexander Rukletsov]

> On Nov. 27, 2015, 10:28 p.m., Joris Van Remoortere wrote:
> > src/tests/master_quota_tests.cpp, line 964
> > 
> >
> > Can we settle, and then resume here?
> > I think with Joseph's cleanups we shouldn't exit a test with the clock 
> > paused.

I think `Clock::resume()` here can be problematic, since it introduces the 
chance a batch allocation happens before the `Shutdown()`.


- Alexander


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


On Nov. 26, 2015, 5:34 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40754/
> ---
> 
> (Updated Nov. 26, 2015, 5:34 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed flaky test (AvailableResourcesAfterRescinding).
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp 15edb27f3ea5d3813c937614aa217ca999fd2481 
> 
> Diff: https://reviews.apache.org/r/40754/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="*AvailableResourcesAfterRescinding*" ./bin/mesos-tests.sh 
> --gtest_repeat=1000 with `allocation_interval` set to 1ms
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40706: Clarified a comment for `Accept` call.

[Michael Park]

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



include/mesos/scheduler/scheduler.proto (line 229)


Sorry to be nit-picky about such a trivial change, but could we keep this 
comment consistent with other places (e.g. `include/mesos/scheduler.hpp`)?

```
Available resources are aggregated when multiple offers are provided. Note 
that all offers must belong to the same slave.
```

Maybe we could just do:

```
NOTE: All offers must belong to the same agent.
```


- Michael Park


On Nov. 30, 2015, 10:47 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40706/
> ---
> 
> (Updated Nov. 30, 2015, 10:47 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> 7bf447e07665dd33646edd0c684ab559712631cc 
> 
> Diff: https://reviews.apache.org/r/40706/diff/
> 
> 
> Testing
> ---
> 
> not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Klaus Ma]

> On Nov. 24, 2015, 5:42 p.m., Benjamin Bannier wrote:
> > src/tests/mesos.hpp, line 87
> > 
> >
> > This seems like a weird addition for this patch: while the existing 
> > `using` decls above could be justified (it's hard to imagine testing test 
> > infrastructure w/o having to perform some environment cleanup), 
> > unconditionally pulling in `mesos::fetcher::FetcherInfo` here seems way too 
> > general; also, it probably would make any attempt of e.g., mocking 
> > `FetcherInfo` anywhere much harder.
> > 
> > Either pull this into the internal namespace, or just do the extra 
> > typing here and pull it in in the corresponding `mesos.cpp`.
> 
> Klaus Ma wrote:
> @Guangya/Ben, I'm thinking mesos.hpp as internal header :). Anyweay, 
> maybe we move `using mesos::internal::slave::FetherInfo` into 
> `mesos::internal::test` in `mesos.hpp`.
> 
> Benjamin Bannier wrote:
> I think the guideline here is to limit the scope of using declarations as 
> much as possible, so moving it to the smallest possible scope (which here 
> unfortunately is still large) is preferred.
> 
> Michael Park wrote:
> Sorry, I still don't understand why this was introduced to begin with. 
> Could you explain?
> 
> Benjamin Bannier wrote:
> A using decl was dropped in `src/slave/containerizer/fetcher.hpp`, and 
> since we are often confused by long names (even though we like namespaces) we 
> need to add this here for a function decl below.

@mcypark, according to our code style document, we should not use global 
`using` in header file. So I start this RR to move 
`mesos::fetcher::FetcherInfo` into an internal namespace.


- Klaus


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


On Nov. 25, 2015, 9:54 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40631/
> ---
> 
> (Updated Nov. 25, 2015, 9:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-3963
> https://issues.apache.org/jira/browse/MESOS-3963
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> According to the google code style, the using should be used in internal 
> namespace in header files. Grep the header files, only fetcher.hpp deserved a 
> path.
> 
> 
> > You may use a using-declaration anywhere in a .cc file (including in the 
> > global namespace), and in functions, methods, classes, or within internal 
> > namespaces in .h files.
> 
> >Do not use using-declarations in .h files except in explicitly marked 
> >internal-only namespaces, because anything imported into a namespace in a .h 
> >file becomes part of the public API exported by that file.
> 
> ```
> // OK in .cc files.
> // Must be in a function, method, internal namespace, or
> // class in .h files.
> using ::foo::bar;
> ```
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.hpp 78e7d14 
>   src/tests/mesos.hpp a2a76f5 
> 
> Diff: https://reviews.apache.org/r/40631/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 40524: Used string directly in resources.cpp.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40524]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 1, 2015, 2:13 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40524/
> ---
> 
> (Updated Dec. 1, 2015, 2:13 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, 
> and Klaus Ma.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used string directly in resources.cpp.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 98804a456b0faecc4aa56929bd1b83e5900db5ae 
>   src/v1/resources.cpp 8488c318a987a150fc5fde26b54246e8effb0428 
> 
> Diff: https://reviews.apache.org/r/40524/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.

[Michael Park]

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



src/master/master.hpp (line 706)


If the intention is to be similar to the `authorizeTask` interface, we 
should take `Resources` here instead? Although I think taking 
`Offer::Operation::Reserve` would be best here.



src/master/master.hpp (line 730)


Similar question as above.



src/master/master.cpp (line 2771)


Why is it that we need to perform validation within authorization? We 
perform `authorizeTask` with a `TaskInfo` that has not been validated yet, and 
take care of it later on. Does that pattern not work here?


- Michael Park


On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> ---
> 
> (Updated Nov. 30, 2015, 8:35 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
> 
> Diff: https://reviews.apache.org/r/39987/diff/
> 
> 
> Testing
> ---
> 
> This is the third in a chain of 5 patches. `make check` was used to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40631: Move "using mesos::fetcher::FetcherInfo" into internal namespace in "fetcher.hpp"

[Klaus Ma]

> On Dec. 1, 2015, 9:08 p.m., Michael Park wrote:
> > src/slave/containerizer/fetcher.hpp, line 43
> > 
> >
> > I agree with the intent to remove the using declaration out of 
> > `fetcher.hpp`, but I don't agree with moving it into an internal namespace. 
> > Fetcher-related cpp files should pull it in itself; 
> > `src/launcher/fetcher.cpp` for example already does so.
> > 
> > I strongly disagree with introducing a using declaration to 
> > `src/tests/mesos.hpp`. It doesn't seem to be something that should be 
> > common across all tests. It should be local to fetcher-related tests; 
> > `src/tests/fetcher_tests.cpp` for example already pulls it in itself.
> > 
> > My suggestion would be to use using declarations in cpp files, and use 
> > qualified names in hpp files. We end up with the following changes:
> > 
> > (1) Add `using mesos::fetcher::FetcherInfo;` in 
> > `src/slave/containerizer/fetcher.cpp` and 
> > `src/tests/fetcher_cache_tests.cpp`.
> > (2) `s/FetcherInfo/fetcher::FetcherInfo/` for 1 instance in 
> > `src/slave/containerizer/fetcher.hpp` and 3 instances in 
> > `src/tests/mesos.cpp`.

Agree :). I'll update them accordingly.


- Klaus


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


On Dec. 1, 2015, 9:13 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40631/
> ---
> 
> (Updated Dec. 1, 2015, 9:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-3963
> https://issues.apache.org/jira/browse/MESOS-3963
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> According to the google code style, the using should be used in internal 
> namespace in header files. Grep the header files, only fetcher.hpp deserved a 
> path.
> 
> 
> > You may use a using-declaration anywhere in a .cc file (including in the 
> > global namespace), and in functions, methods, classes, or within internal 
> > namespaces in .h files.
> 
> >Do not use using-declarations in .h files except in explicitly marked 
> >internal-only namespaces, because anything imported into a namespace in a .h 
> >file becomes part of the public API exported by that file.
> 
> ```
> // OK in .cc files.
> // Must be in a function, method, internal namespace, or
> // class in .h files.
> using ::foo::bar;
> ```
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.hpp 78e7d14 
>   src/tests/mesos.hpp a2a76f5 
> 
> Diff: https://reviews.apache.org/r/40631/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 39986: [2/5] Enabled the Authorizer to handle Reserve/Unreserve ACLs.

[Michael Park]

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

Ship it!


Ship It!

- Michael Park


On Nov. 30, 2015, 8:33 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39986/
> ---
> 
> (Updated Nov. 30, 2015, 8:33 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled the Authorizer to handle Reserve/Unreserve ACLs.
> Note: this review is continued from https://reviews.apache.org/r/37110/
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> c228c2b40ff5fdb007153101938f91ff269370a5 
>   src/authorizer/local/authorizer.hpp 
> bc3e4e12a35f222783159e35c4662e182ba0c107 
>   src/authorizer/local/authorizer.cpp 
> 853678b8fd3151d02fc77d5eadaf9948f33caebb 
>   src/tests/authorization_tests.cpp 32bcac731a6e8f8adb7298ed6c7174f6eb0499cb 
>   src/tests/mesos.hpp f097c683fed02240e9f54ac4491ab007f4652736 
>   src/tests/mesos.cpp a75182ec7f6d0c6063bbb36a6ccf4e8a4a81c8dd 
> 
> Diff: https://reviews.apache.org/r/39986/diff/
> 
> 
> Testing
> ---
> 
> The is the second in a chain of 5 patches. This diff includes new tests for 
> the authorization of reserve and unreserve operations via ACLs. `make check` 
> was used to test after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40799: Removed non-ASCII characters from test case comment.

[Alexander Rukletsov]

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


I propose to replace the comment in https://reviews.apache.org/r/40818/

Just curious, how did you spot this? it looks fine in my editor. Also, is it 
something we should add as a commit hook?

- Alexander Rukletsov


On Nov. 30, 2015, 6:43 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40799/
> ---
> 
> (Updated Nov. 30, 2015, 6:43 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed non-ASCII characters from test case comment.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp 481298361574008b2cf9d213c908deb633393e31 
> 
> Diff: https://reviews.apache.org/r/40799/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40811: Fixed flakiness in tests using the Scheduler library

[Alexander Rojas]

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

Ship it!


Ship It!

- Alexander Rojas


On Dec. 1, 2015, 4:38 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40811/
> ---
> 
> (Updated Dec. 1, 2015, 4:38 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4029
> https://issues.apache.org/jira/browse/MESOS-4029
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the `event` queue object was defined after the `callbacks` 
> object. However, the `callbacks` object held on a reference to the queue 
> object. This led to a crash in some scenarios when we received another event 
> from the master after the `event` object was destroyed. (Note that objects on 
> the stack are destroyed in the reverse order of definition)
> 
> The short term fix is to move the `event` queue object above the `callbacks` 
> object. This ensures that it is destroyed after the `callbacks` object. The 
> long term fix would be worked on via `MESOS-3339` that moves the tests to a 
> callback interface model similar to what we have for the driver tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> ade05f932020013ced19c1573be756a029396fac 
>   src/tests/scheduler_tests.cpp 67432109c7df6be0aa76e94a03bd5b2e9c96d14e 
> 
> Diff: https://reviews.apache.org/r/40811/diff/
> 
> 
> Testing
> ---
> 
> make check + Reproduced this by introducing a sleep in the `Mesos` destructor 
> + did not filter out the `HEARTBEAT` event.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 40706: Clarified a comment for `Accept` call.

[Alexander Rukletsov]

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

(Updated Dec. 1, 2015, 1:33 p.m.)


Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone.


Changes
---

Rephrased.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/scheduler/scheduler.proto 
7bf447e07665dd33646edd0c684ab559712631cc 

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


Testing
---

not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 40746: Quota: Removed quota from registry for remove request.

[Alexander Rukletsov]

> On Nov. 30, 2015, 6:47 p.m., Joseph Wu wrote:
> > src/master/quota_handler.cpp, lines 393-394
> > 
> >
> > `RemoveQuota` appears idempotent, so what is the problem with trying to 
> > remove a quota that is currently being removed?
> > 
> > Is it the fact that you check this?
> > ```
> >   // Check that we are removing an existing quota.
> >   if (!master->quotas.contains(role)) { ... }
> > ```

`RemoveQuota` is idempotent, right, but 
`HierarchicalAllocatorProcess::removeQuota()` is not. I think we can change the 
whole function to be idempotent, but is this really necessary?


- Alexander


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


On Nov. 26, 2015, 12:28 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40746/
> ---
> 
> (Updated Nov. 26, 2015, 12:28 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-4021
> https://issues.apache.org/jira/browse/MESOS-4021
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/quota_handler.cpp e9c8791d4bd00e6c1be1844f27d9bee26f722a9b 
> 
> Diff: https://reviews.apache.org/r/40746/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 40821: Quota: Ensured the sorter for quota'ed roles does not contain revocable resources.

[Alexander Rukletsov]

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

Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
Joseph Wu, and Qian Zhang.


Bugs: MESOS-3718
https://issues.apache.org/jira/browse/MESOS-3718


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
1cd8d16661568010901e74705375e7719cdfb8a0 
  src/master/allocator/mesos/hierarchical.cpp 
b8f3be8e65f9be68a78c61a958903d3e36910efb 

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


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 40811: Fixed flakiness in tests using the Scheduler library

[Alexander Rojas]

> On Dec. 1, 2015, 2:56 p.m., Alexander Rojas wrote:
> > Ship It!

I have to revert this ship-it, after playing with it longer, I manage to 
reproduce at least two types of crashes after applying this patch.


- Alexander


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


On Dec. 1, 2015, 4:38 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40811/
> ---
> 
> (Updated Dec. 1, 2015, 4:38 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4029
> https://issues.apache.org/jira/browse/MESOS-4029
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the `event` queue object was defined after the `callbacks` 
> object. However, the `callbacks` object held on a reference to the queue 
> object. This led to a crash in some scenarios when we received another event 
> from the master after the `event` object was destroyed. (Note that objects on 
> the stack are destroyed in the reverse order of definition)
> 
> The short term fix is to move the `event` queue object above the `callbacks` 
> object. This ensures that it is destroyed after the `callbacks` object. The 
> long term fix would be worked on via `MESOS-3339` that moves the tests to a 
> callback interface model similar to what we have for the driver tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> ade05f932020013ced19c1573be756a029396fac 
>   src/tests/scheduler_tests.cpp 67432109c7df6be0aa76e94a03bd5b2e9c96d14e 
> 
> Diff: https://reviews.apache.org/r/40811/diff/
> 
> 
> Testing
> ---
> 
> make check + Reproduced this by introducing a sleep in the `Mesos` destructor 
> + did not filter out the `HEARTBEAT` event.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

[Michael Park]

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



src/master/master.cpp (lines 3190 - 3191)


As I asked and pointed out in https://reviews.apache.org/r/39987/, this is 
not true for launch task operations.

We can see below that launch task validation is performed after 
authorization. Couldn't we do the same thing?

```cpp
Future authorization = authorizations.front();
authorizations.pop_front();

...

CHECK(!authorization.isDiscarded());

if (authorization.isFailed() || !authorization.get()) {
  // authorization failed; drop or send status update.
}

Option error = <...>::validate(...);
if (error.isSome()) {
  // validation failed: drop or send status update.
}

...
```



src/master/master.cpp (lines 3201 - 3206)


This case is actually a validation error, yet our error message is 
"authorization failure" which is a bit inaccurate. Seems to me like a result of 
bundling validation within authorization.


- Michael Park


On Nov. 30, 2015, 9:20 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 30, 2015, 9:20 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
>   src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39988: [4/5] Added authorization for dynamic reservation master endpoints.

[Michael Park]

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



src/master/http.cpp (lines 1037 - 1040)


Let's not lose the comment on the left.



src/master/http.cpp (lines 1037 - 1041)


It looks like the `repair` here is to catch the case where 
`authorizeReserveResources` __fails__ on an invalid reserve request.

In https://reviews.apache.org/r/39987/ I asked whether the validation step 
needs to be within the authorization step. I still think it should be 
separated. If we were to separate them out, with Jie's suggestion below in 
mind, this could look like:

```cpp
  return master->authorizeReserveResources(operation, principal)
.then(defer(master->self(), [=](bool authorized) -> Future {
  if (!authorized) {
return Unauthorized("Mesos master");
  }
  Option error =
validation::operation::validate(operation.reserve(), None(), 
principal);
  if (error.isSome()) {
return BadRequest("Invalid RESERVE operation: " + 
error.get().message);
  }
  return _operation(slaveId, resources.flatten(), operation);
}));
```



src/master/http.cpp (lines 2307 - 2317)


With Jie's suggestion, we don't need this anymore. However, I would like to 
point out some weirdness here for future reference.

Since the `_operation` is only being used with `.then` or in a direct call, 
the `authorized` can/should be `bool` rather than `Future`. Take a look 
at `_teardown`. The reason this works as-is is because `bool` is implicitly 
convertible to `Future`.

With that in mind, we can then observe that the `if 
(!authorized.isReady())` is actually dead code. Whatever error handling that 
was intended to be done in that if-clause actually gets caught by the `repair` 
above instead.


- Michael Park


On Nov. 30, 2015, 8:46 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39988/
> ---
> 
> (Updated Nov. 30, 2015, 8:46 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization for dynamic reservation master endpoints.
> Note: this review is continued from https://reviews.apache.org/r/37126/
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 9d729ef7f7d7ad6185934648f833e4f8a4f0a123 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
>   src/tests/reservation_endpoints_tests.cpp 
> f30ff8bc6a3e9773437fa7fd7c8f569b7d7e2d9d 
> 
> Diff: https://reviews.apache.org/r/39988/diff/
> 
> 
> Testing
> ---
> 
> This is the fourth in a chain of 5 patches. Added new reservation endpoints 
> tests to validate authorization of reserve and unreserve operations using 
> ACLs. `make check` was run to test after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40445: Added linter for license headers in some file types.

[Michael Park]

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


Why do we want to introduce a `mesos-license.py` rather than simply augmenting 
`mesos-style.py`?


support/mesos-style.py (line 106)


Isn't `run_license_check` undefined here?


- Michael Park


On Nov. 27, 2015, 10:52 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40445/
> ---
> 
> (Updated Nov. 27, 2015, 10:52 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Marco Massenzio, and Michael Park.
> 
> 
> Bugs: MESOS-3581
> https://issues.apache.org/jira/browse/MESOS-3581
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added linter for license headers in some file types.
> 
> 
> Diffs
> -
> 
>   support/mesos-license.py PRE-CREATION 
>   support/mesos-style.py 66b45692c3c04f68358b63d52e4d87934f241bd7 
> 
> Diff: https://reviews.apache.org/r/40445/diff/
> 
> 
> Testing
> ---
> 
> Ran the a whole clean checkout through the linter with only one expected 
> failure (`3rdparty/libprocess/stout/tests/protobuf_tests.proto` which lacks a 
> license).
> 
> 
> NOTE TO THE COMMITTER
> -
> 
> Before committing this, it is probably a good idea to check the whole code 
> base again and fix any new files which do not follow the current license 
> style. The commits which originally fixed this were
> 
> * fa36917 (mesos),
> * dc23756 (stout), and
> * 3539b7a (libprocess).
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.

[Greg Mann]

> On Dec. 1, 2015, 2:24 p.m., Michael Park wrote:
> > src/master/master.cpp, line 2771
> > 
> >
> > Why is it that we need to perform validation within authorization? We 
> > perform `authorizeTask` with a `TaskInfo` that has not been validated yet, 
> > and take care of it later on. Does that pattern not work here?

That pattern would work here, and the original patches I posted worked that 
way. We ended up switching to this solution because it makes for a cleaner 
implementation and eliminates some redundant code. However, it does come at the 
cost of making error messaging more difficult (as you noted in another patch) 
and decreasing the separation of functionality that can make debugging easier. 
I don't have a strong preference for either path at the moment. Perhaps Jie 
could chime in with his thoughts?


- Greg


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


On Nov. 30, 2015, 8:35 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> ---
> 
> (Updated Nov. 30, 2015, 8:35 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
> 
> Diff: https://reviews.apache.org/r/39987/diff/
> 
> 
> Testing
> ---
> 
> This is the third in a chain of 5 patches. `make check` was used to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>