Re: Review Request 39250: Puller refactor: moved untar to a common place

[Jojy Varghese]

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

(Updated Nov. 9, 2015, 7:55 a.m.)


Review request for mesos and Timothy Chen.


Changes
---

rebased


Repository: mesos


Description
---

Puller refactor: moved untar to a common place


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
9b2b8126f39d2b4014cfb637202670e6a06cdf56 
  src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
e4202550af4deb0d355ca9ae6d71b40623f9 
  src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
70501f1811fde5b636e5dcf751ef9356b8c7caef 
  src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
b80213d6090d1e8ec5635f0bca98a848bfe4e940 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39285: Added Quota Request Validation.

[Joerg Schad]

> On Nov. 6, 2015, 4:19 p.m., Klaus Ma wrote:
> > src/master/quota_handler.cpp, line 180
> > 
> >
> > Should we move it into `validateQuotaRequest`? If any role is exist in 
> > master, we did not need to continue to check others. And in QuotaHandler, 
> > we had the pointer to master `Master* master`.
> 
> Alexander Rukletsov wrote:
> I'd keep it here, because it's related to how we currently process the 
> request, rather than whether the request is valid.

There was an earlier comment from Joris mentioning that this isn't really part 
of validatating the request (as it also involves state of the master).


> On Nov. 6, 2015, 4:19 p.m., Klaus Ma wrote:
> > src/master/quota_handler.cpp, line 140
> > 
> >
> > Suggest to move it into the loop; if any role is not known by master, 
> > we did not need to continue to check others.
> 
> Alexander Rukletsov wrote:
> I think the flow is more readable how it's now: in the loop we 
> reconstruct the "reference" role, afterwards we check whether it is known to 
> the master. Also, my gut feeling is that typos in roles will not be that 
> frequent, so checking it once instead of per resource makes sense to me.

There should just be a single role per request, why should I check that in the 
loop?


- Joerg


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


On Nov. 6, 2015, 3:23 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 6, 2015, 3:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

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

(Updated Nov. 9, 2015, 7:29 a.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

see Summary.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing (updated)
---

make check


Thanks,

Joerg Schad

Re: Review Request 39250: Puller refactor: moved untar to a common place

[Jojy Varghese]

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

(Updated Nov. 9, 2015, 7:23 a.m.)


Review request for mesos and Timothy Chen.


Changes
---

rebased


Repository: mesos


Description
---

Puller refactor: moved untar to a common place


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
f314f20d989ed0125e15d2a14d0f8e56c56976d5 
  src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
8010b8aa75a2fc2e4b537f915f9dca028e407b63 
  src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 
  src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 39250: Puller refactor: moved untar to a common place

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [38747]

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

Error:
 2015-11-09 07:23:54 URL:https://reviews.apache.org/r/38747/diff/raw/ 
[16724/16724] -> "38747.patch" [1]
error: patch failed: 3rdparty/libprocess/Makefile.am:141
error: 3rdparty/libprocess/Makefile.am: patch does not apply
error: patch failed: 3rdparty/libprocess/src/tests/CMakeLists.txt:20
error: 3rdparty/libprocess/src/tests/CMakeLists.txt: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Nov. 9, 2015, 7:23 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39250/
> ---
> 
> (Updated Nov. 9, 2015, 7:23 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Puller refactor: moved untar to a common place
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> f314f20d989ed0125e15d2a14d0f8e56c56976d5 
>   src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
> 8010b8aa75a2fc2e4b537f915f9dca028e407b63 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39250/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 40070: Refactored registry puller pull to smaller functions.

[Jojy Varghese]

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

Ship it!


Ship It!


src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp (line 58)


i thought you wanted to add more to this comment. That it downloads 
multiple layers for the image.


- Jojy Varghese


On Nov. 9, 2015, 6:24 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40070/
> ---
> 
> (Updated Nov. 9, 2015, 6:24 a.m.)
> 
> 
> Review request for mesos and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored registry puller pull to smaller blocks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40070/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 40070: Refactored registry puller pull to smaller functions.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [39832, 39882, 39839, 38579, 39015, 39016, 39017, 39053, 
39112, 39340, 39840, 38580, 40069]

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

Error:
 2015-11-09 07:01:23 URL:https://reviews.apache.org/r/40069/diff/raw/ 
[16792/16792] -> "40069.patch" [1]
error: patch failed: src/Makefile.am:562
error: src/Makefile.am: patch does not apply
error: patch failed: 
src/slave/containerizer/mesos/provisioner/docker/puller.cpp:19
error: src/slave/containerizer/mesos/provisioner/docker/puller.cpp: patch does 
not apply
error: src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp: does 
not exist in index
error: src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp: does 
not exist in index
error: patch failed: src/tests/containerizer/provisioner_docker_tests.cpp:42
error: src/tests/containerizer/provisioner_docker_tests.cpp: patch does not 
apply
Failed to apply patch

- Mesos ReviewBot


On Nov. 9, 2015, 6:24 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40070/
> ---
> 
> (Updated Nov. 9, 2015, 6:24 a.m.)
> 
> 
> Review request for mesos and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored registry puller pull to smaller blocks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40070/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 39018: Added JSON parsing for Resources.

[Adam B]

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

Ship it!


Much better error handling, but we're still dropping an error message 
unnecessarily. Fix it, then I'll ship it.


src/common/resources.cpp (lines 353 - 354)


Doesn't Resources::validate() return an Error? Why not use that Error and 
its message with your return?


- Adam B


On Nov. 8, 2015, 9:57 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> ---
> 
> (Updated Nov. 8, 2015, 9:57 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-2467
> https://issues.apache.org/jira/browse/MESOS-2467
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes code changes necessary for JSON parsing of Resources. 
> Documentation changes will be posted soon in another review 
> (https://reviews.apache.org/r/39102/).
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 
>   include/mesos/v1/resources.hpp fe8925ac851b74d1b37919f00afc7ed816f47ea5 
>   src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 
>   src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
>   src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 
> 
> Diff: https://reviews.apache.org/r/39018/diff/
> 
> 
> Testing
> ---
> 
> New code was added to `ResourcesTest.ParsingFromJSON`, and two new tests were 
> added: `ResourcesTest.ParsingFromJSONWithRoles` and 
> `ResourcesTest.ParsingFromJSONError`. These attempt to test common error 
> scenarios that might show up in user-supplied JSON.
> 
> `make check` was used to confirm that all tests pass.
> 
> The original resources parsing format is used throughout many other tests 
> (check `src/tests/sorter_tests.cpp`, for example), so it's clear that the 
> original parsing continues to work correctly.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40069: Renamed RemotePuller to RegistryPuller.

[Jojy Varghese]

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

Ship it!


Ship It!

- Jojy Varghese


On Nov. 9, 2015, 6:21 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40069/
> ---
> 
> (Updated Nov. 9, 2015, 6:21 a.m.)
> 
> 
> Review request for mesos and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed RemotePuller to RegistryPuller.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10 
>   src/Makefile.am c479acafc92b7e9d078729d443b0d2f00d40f353 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> bc487d34e4087fa53452d63416d0f31e77060eb0 
>   src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp 
> PRE-CREATION 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> fe6a90fe32364eec8ef923a000db19183603c338 
> 
> Diff: https://reviews.apache.org/r/40069/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37168]

All tests passed.

- Mesos ReviewBot


On Nov. 9, 2015, 5:29 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37168/
> ---
> 
> (Updated Nov. 9, 2015, 5:29 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3063
> https://issues.apache.org/jira/browse/MESOS-3063
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Provide example for dynamic reservation features.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c479aca 
>   src/examples/dynamic_reservation_framework.cpp PRE-CREATION 
>   src/tests/dynamic_reservation_framework_test.sh PRE-CREATION 
>   src/tests/examples_tests.cpp 3f56b30 
>   src/tests/flags.hpp 984cd4a 
>   src/tests/script.cpp d2280c2 
> 
> Diff: https://reviews.apache.org/r/37168/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Review Request 40070: Refactored registry puller pull to smaller functions.

[Timothy Chen]

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

Review request for mesos and Jojy Varghese.


Repository: mesos


Description
---

Refactored registry puller pull to smaller blocks.


Diffs
-

  src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
PRE-CREATION 

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


Testing
---


Thanks,

Timothy Chen

Review Request 40069: Renamed RemotePuller to RegistryPuller.

[Timothy Chen]

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

Review request for mesos and Jojy Varghese.


Repository: mesos


Description
---

Renamed RemotePuller to RegistryPuller.


Diffs
-

  src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10 
  src/Makefile.am c479acafc92b7e9d078729d443b0d2f00d40f353 
  src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
bc487d34e4087fa53452d63416d0f31e77060eb0 
  src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp 
PRE-CREATION 
  src/tests/containerizer/provisioner_docker_tests.cpp 
fe6a90fe32364eec8ef923a000db19183603c338 

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


Testing
---


Thanks,

Timothy Chen

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

[Greg Mann]

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

(Updated Nov. 9, 2015, 6:13 a.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Addressed comments.


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 (updated)
-

  src/master/master.cpp 25b94c8b41f65599327e0a0459ba86c6078895a8 
  src/tests/reservation_tests.cpp 6b7c43c8b5c64618249dbee926383242320c111e 

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 39987: [3/5] Added 'Master::authorize' for Reserve/Unreserve.

[Greg Mann]

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

(Updated Nov. 9, 2015, 6:10 a.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Addressed comment.


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


Repository: mesos


Description
---

Added 'Master::authorize' for Reserve/Unreserve.
Note: this review is continued from https://reviews.apache.org/r/37125/


Diffs (updated)
-

  src/master/master.hpp ead8520b7108a0f2c3a0bb11ae7b543897d111a2 
  src/master/master.cpp 25b94c8b41f65599327e0a0459ba86c6078895a8 

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 39986: [2/5] Enabled the Authorizer to handle Reserve/Unreserve ACLs.

[Greg Mann]

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

(Updated Nov. 9, 2015, 6:09 a.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Addressed comments.


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 (updated)
-

  include/mesos/authorizer/authorizer.hpp 
d667a52f90f970a313580446a5a006cec4b5e25b 
  src/authorizer/local/authorizer.hpp 32de102fd588f029882ef121ca83a7410c65 
  src/authorizer/local/authorizer.cpp 6d7da87731a438c2180cf91003e09d4aa5a1c773 
  src/tests/authorization_tests.cpp 4940450193d89a8f11a15d31723119fa26cdab1b 
  src/tests/mesos.hpp 71eba8bb2689061e3bea3a5093e3e9c445362c68 

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 39018: Added JSON parsing for Resources.

[Greg Mann]

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

(Updated Nov. 9, 2015, 5:57 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.


Changes
---

Changed comments.


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


Repository: mesos


Description
---

This includes code changes necessary for JSON parsing of Resources. 
Documentation changes will be posted soon in another review 
(https://reviews.apache.org/r/39102/).


Diffs (updated)
-

  include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 
  include/mesos/v1/resources.hpp fe8925ac851b74d1b37919f00afc7ed816f47ea5 
  src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 
  src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
  src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 

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


Testing
---

New code was added to `ResourcesTest.ParsingFromJSON`, and two new tests were 
added: `ResourcesTest.ParsingFromJSONWithRoles` and 
`ResourcesTest.ParsingFromJSONError`. These attempt to test common error 
scenarios that might show up in user-supplied JSON.

`make check` was used to confirm that all tests pass.

The original resources parsing format is used throughout many other tests 
(check `src/tests/sorter_tests.cpp`, for example), so it's clear that the 
original parsing continues to work correctly.


Thanks,

Greg Mann

Re: Review Request 39018: Added JSON parsing for Resources.

[Greg Mann]

> On Nov. 8, 2015, 9:32 a.m., Adam B wrote:
> > src/common/resources.cpp, line 271
> > 
> >
> > Red flag: "silently ignored" scares me. Don't you mean that an error in 
> > validate() will yield an empty Resource, which should have been checked 
> > before/after/during this call? Why can't we check 
> > Resources.isEmpty(Resource) here?

Sorry, yea I was tempted by the 
`Resources::Resources(RepeatedPtrField)` constructor, which is 
perfect for this conversion but prevents us from catching any errors. I think 
in the long run it would be better to have a factory function for `Resources` 
which returns `Tryhttps://reviews.apache.org/r/39018/#review105607
---


On Nov. 9, 2015, 5:39 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> ---
> 
> (Updated Nov. 9, 2015, 5:39 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-2467
> https://issues.apache.org/jira/browse/MESOS-2467
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes code changes necessary for JSON parsing of Resources. 
> Documentation changes will be posted soon in another review 
> (https://reviews.apache.org/r/39102/).
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 
>   include/mesos/v1/resources.hpp fe8925ac851b74d1b37919f00afc7ed816f47ea5 
>   src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 
>   src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
>   src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 
> 
> Diff: https://reviews.apache.org/r/39018/diff/
> 
> 
> Testing
> ---
> 
> New code was added to `ResourcesTest.ParsingFromJSON`, and two new tests were 
> added: `ResourcesTest.ParsingFromJSONWithRoles` and 
> `ResourcesTest.ParsingFromJSONError`. These attempt to test common error 
> scenarios that might show up in user-supplied JSON.
> 
> `make check` was used to confirm that all tests pass.
> 
> The original resources parsing format is used throughout many other tests 
> (check `src/tests/sorter_tests.cpp`, for example), so it's clear that the 
> original parsing continues to work correctly.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39018: Added JSON parsing for Resources.

[Greg Mann]

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

(Updated Nov. 9, 2015, 5:39 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

This includes code changes necessary for JSON parsing of Resources. 
Documentation changes will be posted soon in another review 
(https://reviews.apache.org/r/39102/).


Diffs (updated)
-

  include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 
  include/mesos/v1/resources.hpp fe8925ac851b74d1b37919f00afc7ed816f47ea5 
  src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 
  src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
  src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 

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


Testing
---

New code was added to `ResourcesTest.ParsingFromJSON`, and two new tests were 
added: `ResourcesTest.ParsingFromJSONWithRoles` and 
`ResourcesTest.ParsingFromJSONError`. These attempt to test common error 
scenarios that might show up in user-supplied JSON.

`make check` was used to confirm that all tests pass.

The original resources parsing format is used throughout many other tests 
(check `src/tests/sorter_tests.cpp`, for example), so it's clear that the 
original parsing continues to work correctly.


Thanks,

Greg Mann

Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)

[Klaus Ma]

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

(Updated Nov. 9, 2015, 1:29 p.m.)


Review request for mesos and Michael Park.


Changes
---

Address comments


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


Repository: mesos


Description
---

Provide example for dynamic reservation features.


Diffs (updated)
-

  src/Makefile.am c479aca 
  src/examples/dynamic_reservation_framework.cpp PRE-CREATION 
  src/tests/dynamic_reservation_framework_test.sh PRE-CREATION 
  src/tests/examples_tests.cpp 3f56b30 
  src/tests/flags.hpp 984cd4a 
  src/tests/script.cpp d2280c2 

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


Testing
---

make
make check


Thanks,

Klaus Ma

Re: Review Request 39459: Added docs for using delay() and clocks in libprocess.

[Benjamin Hindman]

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

Ship it!


Thanks Neil! I made a few minor edits so folks that copy this code before 
changing it for their needs will better match our style. Committing now.

- Benjamin Hindman


On Oct. 19, 2015, 11:06 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39459/
> ---
> 
> (Updated Oct. 19, 2015, 11:06 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Hindman, and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Lots more to add, but this is a start.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/README.md 769b79f9a9f816e618dfde62b653e3194386908d 
> 
> Diff: https://reviews.apache.org/r/39459/diff/
> 
> 
> Testing
> ---
> 
> Previewed via Github Gist.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 39453: Added HTTP docs to libprocess README.md.

[Benjamin Hindman]

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

Ship it!


Made a few minor edits to make this code compilable (e.g., `initialize` was 
missing a closing right parenthesis) as well as making some things better match 
our style. Thanks Greg! Committing this now.

- Benjamin Hindman


On Nov. 4, 2015, 5:28 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39453/
> ---
> 
> (Updated Nov. 4, 2015, 5:28 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Hindman, and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added HTTP docs to libprocess README.md.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/README.md 769b79f9a9f816e618dfde62b653e3194386908d 
> 
> Diff: https://reviews.apache.org/r/39453/diff/
> 
> 
> Testing
> ---
> 
> https://gist.github.com/greggomann/b5b35a164a006bec9357
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 38218: Quota: Extended the Allocator interface with quota-related methods.

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On Nov. 8, 2015, 11:06 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38218/
> ---
> 
> (Updated Nov. 8, 2015, 11:06 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3716
> https://issues.apache.org/jira/browse/MESOS-3716
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp dbceb53a3accd32762d09785ecae06667c3cb611 
>   src/master/allocator/mesos/allocator.hpp 
> c5375aa89b210e46c41ac7d68d119749de15d2f5 
>   src/master/allocator/mesos/hierarchical.hpp 
> cfd937ba306273c24fb5337dfeb1a15e1545169b 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
>   src/tests/mesos.hpp f731ac3284a5793b6bf510d3a5b742cbe0938217 
> 
> Diff: https://reviews.apache.org/r/38218/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38218: Quota: Extended the Allocator interface with quota-related methods.

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On Nov. 8, 2015, 11:06 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38218/
> ---
> 
> (Updated Nov. 8, 2015, 11:06 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3716
> https://issues.apache.org/jira/browse/MESOS-3716
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp dbceb53a3accd32762d09785ecae06667c3cb611 
>   src/master/allocator/mesos/allocator.hpp 
> c5375aa89b210e46c41ac7d68d119749de15d2f5 
>   src/master/allocator/mesos/hierarchical.hpp 
> cfd937ba306273c24fb5337dfeb1a15e1545169b 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
>   src/tests/mesos.hpp f731ac3284a5793b6bf510d3a5b742cbe0938217 
> 
> Diff: https://reviews.apache.org/r/38218/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Guangya Liu]

> On Nov. 8, 2015, 2:24 a.m., Guangya Liu wrote:
> > src/tests/master_quota_tests.cpp, lines 209-210
> > 
> >
> > Can you please make this comments limit in 70 per line and ditto for 
> > the following?
> 
> Alexander Rukletsov wrote:
> Why do you suggest to limit to 70?

Well, I do not have strong opinion on this but as we are now still discussing 
http://search-hadoop.com/m/0Vlr6MlNkc1FGzCj2/Mesos+Style+Guideline+Adjustments%25E2%2580%258F+Ben+Mahler+This+has+come+up+in+a+couple+of+reviews%252C+seems+like+we+should+add+some+soft&subj=Re+Mesos+Style+Guideline+Adjustments
 this.

But keeping the comments limit in 70 chars can make the comments more readable 
here as the two lines will almost have same length.


- Guangya


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


On Nov. 6, 2015, 5:02 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated Nov. 6, 2015, 5:02 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40062: MESOS-2315 Deprecate/Remove CommandInfo::ContainerInfo

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40062]

All tests passed.

- Mesos ReviewBot


On Nov. 9, 2015, 1:16 a.m., Vaibhav Khanduja wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40062/
> ---
> 
> (Updated Nov. 9, 2015, 1:16 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-2315
> https://issues.apache.org/jira/browse/MESOS-2315
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESIS-2315 Deprecate/Remove CommandInfo::ContainerInfo
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5ad48bd376c34dd495399b62fa0bd37ddcc5518b 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 08243b61c1c277da7609bc910323cc1e27ff5cd4 
>   src/slave/slave.cpp 535adc3b17d5af3fe811a8e2505f126a28212dbf 
>   src/tests/slave_tests.cpp ddd01104d044b54664b3b1eefdde3f73b8f6d598 
> 
> Diff: https://reviews.apache.org/r/40062/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Vaibhav Khanduja
> 
>

Re: Review Request 40062: MESOS-2315 Deprecate/Remove CommandInfo::ContainerInfo

[Vaibhav Khanduja]

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

(Updated Nov. 9, 2015, 1:16 a.m.)


Review request for mesos and Adam B.


Changes
---

Fixed defect number in summary


Summary (updated)
-

MESOS-2315 Deprecate/Remove CommandInfo::ContainerInfo


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


Repository: mesos


Description
---

MESIS-2315 Deprecate/Remove CommandInfo::ContainerInfo


Diffs (updated)
-

  include/mesos/mesos.proto 5ad48bd376c34dd495399b62fa0bd37ddcc5518b 
  src/slave/containerizer/mesos/containerizer.cpp 
08243b61c1c277da7609bc910323cc1e27ff5cd4 
  src/slave/slave.cpp 535adc3b17d5af3fe811a8e2505f126a28212dbf 
  src/tests/slave_tests.cpp ddd01104d044b54664b3b1eefdde3f73b8f6d598 

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


Testing
---

sudo make check


Thanks,

Vaibhav Khanduja

Re: Review Request 40062: MESIS-2315 Deprecate/Remove CommandInfo::ContainerInfo

[Vaibhav Khanduja]

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

(Updated Nov. 9, 2015, 1:06 a.m.)


Review request for mesos and Adam B.


Changes
---

Added bug number


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


Repository: mesos


Description
---

MESIS-2315 Deprecate/Remove CommandInfo::ContainerInfo


Diffs
-

  include/mesos/mesos.proto 5ad48bd376c34dd495399b62fa0bd37ddcc5518b 
  src/slave/containerizer/mesos/containerizer.cpp 
08243b61c1c277da7609bc910323cc1e27ff5cd4 
  src/slave/slave.cpp 535adc3b17d5af3fe811a8e2505f126a28212dbf 
  src/tests/slave_tests.cpp ddd01104d044b54664b3b1eefdde3f73b8f6d598 

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


Testing
---

sudo make check


Thanks,

Vaibhav Khanduja

Re: Review Request 40062: MESIS-2315 Deprecate/Remove CommandInfo::ContainerInfo

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [40062]

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

Error:
 2015-11-09 00:53:55 URL:https://reviews.apache.org/r/40062/diff/raw/ 
[3762/3762] -> "40062.patch" [1]
error: patch failed: src/slave/slave.cpp:3341
error: src/slave/slave.cpp: patch does not apply
error: patch failed: src/tests/slave_tests.cpp:618
error: src/tests/slave_tests.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Nov. 9, 2015, 12:26 a.m., Vaibhav Khanduja wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40062/
> ---
> 
> (Updated Nov. 9, 2015, 12:26 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESIS-2315 Deprecate/Remove CommandInfo::ContainerInfo
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5ad48bd376c34dd495399b62fa0bd37ddcc5518b 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 08243b61c1c277da7609bc910323cc1e27ff5cd4 
>   src/slave/slave.cpp 535adc3b17d5af3fe811a8e2505f126a28212dbf 
>   src/tests/slave_tests.cpp ddd01104d044b54664b3b1eefdde3f73b8f6d598 
> 
> Diff: https://reviews.apache.org/r/40062/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Vaibhav Khanduja
> 
>

Review Request 40062: MESIS-2315 Deprecate/Remove CommandInfo::ContainerInfo

[Vaibhav Khanduja]

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

Review request for mesos and Adam B.


Repository: mesos


Description
---

MESIS-2315 Deprecate/Remove CommandInfo::ContainerInfo


Diffs
-

  include/mesos/mesos.proto 5ad48bd376c34dd495399b62fa0bd37ddcc5518b 
  src/slave/containerizer/mesos/containerizer.cpp 
08243b61c1c277da7609bc910323cc1e27ff5cd4 
  src/slave/slave.cpp 535adc3b17d5af3fe811a8e2505f126a28212dbf 
  src/tests/slave_tests.cpp ddd01104d044b54664b3b1eefdde3f73b8f6d598 

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


Testing
---

sudo make check


Thanks,

Vaibhav Khanduja

Re: Review Request 39223: Added Quota Request Validation Tests.

[Alexander Rukletsov]

> On Nov. 8, 2015, 2:24 a.m., Guangya Liu wrote:
> > src/tests/master_quota_tests.cpp, lines 209-210
> > 
> >
> > Can you please make this comments limit in 70 per line and ditto for 
> > the following?

Why do you suggest to limit to 70?


- Alexander


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


On Nov. 6, 2015, 5:02 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated Nov. 6, 2015, 5:02 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Alexander Rukletsov]

> On Nov. 6, 2015, 4:19 p.m., Klaus Ma wrote:
> > src/master/quota_handler.cpp, line 119
> > 
> >
> > Should we also check whether `resource.get().role()` is empty? There 
> > should be the case that assign empty string to `resource.role`.

Do we explicitly prohibit empty roles?


> On Nov. 6, 2015, 4:19 p.m., Klaus Ma wrote:
> > src/master/quota_handler.cpp, line 140
> > 
> >
> > Suggest to move it into the loop; if any role is not known by master, 
> > we did not need to continue to check others.

I think the flow is more readable how it's now: in the loop we reconstruct the 
"reference" role, afterwards we check whether it is known to the master. Also, 
my gut feeling is that typos in roles will not be that frequent, so checking it 
once instead of per resource makes sense to me.


- Alexander


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


On Nov. 6, 2015, 3:23 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 6, 2015, 3:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39285: Added Quota Request Validation.

[Alexander Rukletsov]

> On Nov. 6, 2015, 4:19 p.m., Klaus Ma wrote:
> > src/master/quota_handler.cpp, line 180
> > 
> >
> > Should we move it into `validateQuotaRequest`? If any role is exist in 
> > master, we did not need to continue to check others. And in QuotaHandler, 
> > we had the pointer to master `Master* master`.

I'd keep it here, because it's related to how we currently process the request, 
rather than whether the request is valid.


- Alexander


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


On Nov. 6, 2015, 3:23 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39285/
> ---
> 
> (Updated Nov. 6, 2015, 3:23 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Quota Request Validation.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp b76d30197b3decda0a742e03ce01a17a64b633ac 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39285/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38218: Quota: Extended the Allocator interface with quota-related methods.

[Alexander Rukletsov]

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

(Updated Nov. 8, 2015, 11:06 p.m.)


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


Changes
---

Comments.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/master/allocator.hpp dbceb53a3accd32762d09785ecae06667c3cb611 
  src/master/allocator/mesos/allocator.hpp 
c5375aa89b210e46c41ac7d68d119749de15d2f5 
  src/master/allocator/mesos/hierarchical.hpp 
cfd937ba306273c24fb5337dfeb1a15e1545169b 
  src/master/allocator/mesos/hierarchical.cpp 
f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
  src/tests/mesos.hpp f731ac3284a5793b6bf510d3a5b742cbe0938217 

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


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 39317: Quota: Moved `QuotaInfo` protobuf into a separate package.

[Alexander Rukletsov]

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

(Updated Nov. 8, 2015, 11 p.m.)


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


Changes
---

+Backticks


Summary (updated)
-

Quota: Moved `QuotaInfo` protobuf into a separate package.


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


Repository: mesos


Description (updated)
---

See summary.


Diffs
-

  include/mesos/master/quota.hpp 5f7822f40af6fb23cdafdd0c205bcdc67e596935 
  include/mesos/master/quota.proto d2e3a45735e4ebcf257682556aff5075e6e3bf79 
  src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10 
  src/Makefile.am f0715386732b93936de6c8524957c6a6726fe9df 

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 39317: Quota: Moved QuotaInfo protobuf into a separate package.

[Alexander Rukletsov]

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

(Updated Nov. 8, 2015, 10:53 p.m.)


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


Changes
---

Commets.


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


Repository: mesos


Description
---

Quota: Moved QuotaInfo protobuf into a separate package.


Diffs (updated)
-

  include/mesos/master/quota.hpp 5f7822f40af6fb23cdafdd0c205bcdc67e596935 
  include/mesos/master/quota.proto d2e3a45735e4ebcf257682556aff5075e6e3bf79 
  src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10 
  src/Makefile.am f0715386732b93936de6c8524957c6a6726fe9df 

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 38218: Quota: Extended the Allocator interface with quota-related methods.

[Alexander Rukletsov]

> On Nov. 5, 2015, 7:09 p.m., Joseph Wu wrote:
> > include/mesos/master/allocator.hpp, lines 355-356
> > 
> >
> > This bit is somewhat unclear: 
> > "the group of roles with quota set" sounds like it is referring to an 
> > argument, but it's actually referring to the implementation, right?

Yeah, let me try to re-phrase it.


- Alexander


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


On Nov. 5, 2015, 7:29 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38218/
> ---
> 
> (Updated Nov. 5, 2015, 7:29 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3716
> https://issues.apache.org/jira/browse/MESOS-3716
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp dbceb53a3accd32762d09785ecae06667c3cb611 
>   src/master/allocator/mesos/allocator.hpp 
> c5375aa89b210e46c41ac7d68d119749de15d2f5 
>   src/master/allocator/mesos/hierarchical.hpp 
> cfd937ba306273c24fb5337dfeb1a15e1545169b 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
>   src/tests/mesos.hpp 3e58b454c75a2ab9f8b4a29785fa823afefd0c8a 
> 
> Diff: https://reviews.apache.org/r/38218/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38218: Quota: Extended the Allocator interface with quota-related methods.

[Alexander Rukletsov]

> On Nov. 4, 2015, 11:24 p.m., Joris Van Remoortere wrote:
> > include/mesos/master/allocator.hpp, lines 332-347
> > 
> >
> > do you want to re-order these (here or in the implementations) so that 
> > the function name order stay consistent between classes / interface?
> > Not your fault :-)

Good catch! I'll re-order here, as it seems all others are in sync.


- Alexander


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


On Nov. 5, 2015, 7:29 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38218/
> ---
> 
> (Updated Nov. 5, 2015, 7:29 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3716
> https://issues.apache.org/jira/browse/MESOS-3716
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp dbceb53a3accd32762d09785ecae06667c3cb611 
>   src/master/allocator/mesos/allocator.hpp 
> c5375aa89b210e46c41ac7d68d119749de15d2f5 
>   src/master/allocator/mesos/hierarchical.hpp 
> cfd937ba306273c24fb5337dfeb1a15e1545169b 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
>   src/tests/mesos.hpp 3e58b454c75a2ab9f8b4a29785fa823afefd0c8a 
> 
> Diff: https://reviews.apache.org/r/38218/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38218: Quota: Extended the Allocator interface with quota-related methods.

[Alexander Rukletsov]

> On Nov. 2, 2015, 2:22 p.m., Joerg Schad wrote:
> > src/master/allocator/mesos/allocator.hpp, line 268
> > 
> >
> > Could we add a todo for updateQuota()? In my opinion this would make it 
> > easier to understand both the MVP semantic as well as the post mvp plans. 
> > Same below.

I'm not sure we plan a triad `set`, `update`, `remove` is an agreed design for 
the post MVP. I can envision the situation we agree to use, say, `update` and 
`remove`. Hence I'm reluctant to add a todo here.


- Alexander


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


On Nov. 5, 2015, 7:29 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38218/
> ---
> 
> (Updated Nov. 5, 2015, 7:29 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3716
> https://issues.apache.org/jira/browse/MESOS-3716
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp dbceb53a3accd32762d09785ecae06667c3cb611 
>   src/master/allocator/mesos/allocator.hpp 
> c5375aa89b210e46c41ac7d68d119749de15d2f5 
>   src/master/allocator/mesos/hierarchical.hpp 
> cfd937ba306273c24fb5337dfeb1a15e1545169b 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
>   src/tests/mesos.hpp 3e58b454c75a2ab9f8b4a29785fa823afefd0c8a 
> 
> Diff: https://reviews.apache.org/r/38218/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38218: Quota: Extended the Allocator interface with quota-related methods.

[Alexander Rukletsov]

> On Nov. 6, 2015, 2:40 p.m., Joerg Schad wrote:
> > include/mesos/master/allocator.hpp, line 357
> > 
> >
> > s/given role not set/nonexisting role?

This sentence refers to an existing role without quota. Do you think it's not 
clear that the role exists?


- Alexander


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


On Nov. 5, 2015, 7:29 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38218/
> ---
> 
> (Updated Nov. 5, 2015, 7:29 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3716
> https://issues.apache.org/jira/browse/MESOS-3716
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp dbceb53a3accd32762d09785ecae06667c3cb611 
>   src/master/allocator/mesos/allocator.hpp 
> c5375aa89b210e46c41ac7d68d119749de15d2f5 
>   src/master/allocator/mesos/hierarchical.hpp 
> cfd937ba306273c24fb5337dfeb1a15e1545169b 
>   src/master/allocator/mesos/hierarchical.cpp 
> f4e4a123d3da0442e8b0b0ad14d1ee760752ba36 
>   src/tests/mesos.hpp 3e58b454c75a2ab9f8b4a29785fa823afefd0c8a 
> 
> Diff: https://reviews.apache.org/r/38218/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40054: Fix fetch parsing problem for URL with query.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40053, 40054]

All tests passed.

- Mesos ReviewBot


On Nov. 8, 2015, 3:57 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40054/
> ---
> 
> (Updated Nov. 8, 2015, 3:57 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Bugs: MESOS-3367
> https://issues.apache.org/jira/browse/MESOS-3367
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix fetch parsing problem for URL with query.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 0b676f43e8529c6790824c8cf82e21a81d885ef1 
>   src/tests/fetcher_tests.cpp 46d0690887b6b6af4c7102753cae8db0a375fa08 
> 
> Diff: https://reviews.apache.org/r/40054/diff/
> 
> 
> Testing
> ---
> 
> # Add `FetcherTest.OSNetUriWithQueryTest` test case.
> # @bernd-mesos, @bmahler We could do a quick wround in old code. But I think 
> add a parsing function for URL maybe better, it also could fix the TODO item. 
> I think we also could update the code in fetcher to use process::http::URL, 
> so far I only change the Fetcher::basename function.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 38580: Added docker registry RemotePuller

[Jojy Varghese]

> On Nov. 7, 2015, 9:36 a.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 246
> > 
> >
> > Can there be layers that actually have zero content? Docker images 
> > AFAIK has the last layer left empty as a mutable layer, so I'm wondering if 
> > you could actually get empty layer from an image.

Not sure. I tried ubuntu and busybox images and couldnt get a "empty" layer.


- Jojy


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


On Nov. 8, 2015, 4:58 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> ---
> 
> (Updated Nov. 8, 2015, 4:58 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Integrated remote puller with store. Tests will follow.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10 
>   src/Makefile.am c479acafc92b7e9d078729d443b0d2f00d40f353 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
> 87d80026939a326bd1169f46906e36d6ef19f546 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> f314f20d989ed0125e15d2a14d0f8e56c56976d5 
>   src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
> 8010b8aa75a2fc2e4b537f915f9dca028e407b63 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> bc487d34e4087fa53452d63416d0f31e77060eb0 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> bb02d650e16d45fcf337a7954f7a26143fb2c69f 
>   src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73 
>   src/slave/flags.cpp 825b6127dfbc39cb3aea852875eaaab95bac0516 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> fe6a90fe32364eec8ef923a000db19183603c338 
> 
> Diff: https://reviews.apache.org/r/38580/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38580: Added docker registry RemotePuller

[Jojy Varghese]

> On Nov. 7, 2015, 9:36 a.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 51
> > 
> >
> > This is no longer true since we pulled these out of RemotePuller right? 
> > Same as all the other ones.

No its part of RegistryPuller


- Jojy


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


On Nov. 8, 2015, 4:58 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> ---
> 
> (Updated Nov. 8, 2015, 4:58 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Integrated remote puller with store. Tests will follow.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10 
>   src/Makefile.am c479acafc92b7e9d078729d443b0d2f00d40f353 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
> 87d80026939a326bd1169f46906e36d6ef19f546 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> f314f20d989ed0125e15d2a14d0f8e56c56976d5 
>   src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
> 8010b8aa75a2fc2e4b537f915f9dca028e407b63 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> bc487d34e4087fa53452d63416d0f31e77060eb0 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> bb02d650e16d45fcf337a7954f7a26143fb2c69f 
>   src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73 
>   src/slave/flags.cpp 825b6127dfbc39cb3aea852875eaaab95bac0516 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> fe6a90fe32364eec8ef923a000db19183603c338 
> 
> Diff: https://reviews.apache.org/r/38580/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38580: Added docker registry RemotePuller

[Jojy Varghese]

> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 51
> > 
> >
> > Didn''t you pull these out?
> 
> Jojy Varghese wrote:
> Not sure what you mean.

No its part of the RegistryPuller class.


- Jojy


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


On Nov. 8, 2015, 4:58 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> ---
> 
> (Updated Nov. 8, 2015, 4:58 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Integrated remote puller with store. Tests will follow.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10 
>   src/Makefile.am c479acafc92b7e9d078729d443b0d2f00d40f353 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
> 87d80026939a326bd1169f46906e36d6ef19f546 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> f314f20d989ed0125e15d2a14d0f8e56c56976d5 
>   src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
> 8010b8aa75a2fc2e4b537f915f9dca028e407b63 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> bc487d34e4087fa53452d63416d0f31e77060eb0 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> bb02d650e16d45fcf337a7954f7a26143fb2c69f 
>   src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73 
>   src/slave/flags.cpp 825b6127dfbc39cb3aea852875eaaab95bac0516 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> fe6a90fe32364eec8ef923a000db19183603c338 
> 
> Diff: https://reviews.apache.org/r/38580/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38580: Added docker registry RemotePuller

[Jojy Varghese]

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

(Updated Nov. 8, 2015, 4:58 p.m.)


Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.


Changes
---

review


Repository: mesos


Description
---

Integrated remote puller with store. Tests will follow.


Diffs (updated)
-

  src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10 
  src/Makefile.am c479acafc92b7e9d078729d443b0d2f00d40f353 
  src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 
87d80026939a326bd1169f46906e36d6ef19f546 
  src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
f314f20d989ed0125e15d2a14d0f8e56c56976d5 
  src/slave/containerizer/mesos/provisioner/docker/puller.hpp 
8010b8aa75a2fc2e4b537f915f9dca028e407b63 
  src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
bc487d34e4087fa53452d63416d0f31e77060eb0 
  src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
bb02d650e16d45fcf337a7954f7a26143fb2c69f 
  src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73 
  src/slave/flags.cpp 825b6127dfbc39cb3aea852875eaaab95bac0516 
  src/tests/containerizer/provisioner_docker_tests.cpp 
fe6a90fe32364eec8ef923a000db19183603c338 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 40054: Fix fetch parsing problem for URL with query.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [40053]

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

Error:
 2015-11-08 16:23:58 URL:https://reviews.apache.org/r/40053/diff/raw/ 
[3177448/3177448] -> "40053.patch" [1]
error: patch failed: 3rdparty/libprocess/include/process/http.hpp:68
error: 3rdparty/libprocess/include/process/http.hpp: patch does not apply
error: patch failed: 3rdparty/libprocess/src/http.cpp:31
error: 3rdparty/libprocess/src/http.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Nov. 8, 2015, 3:57 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40054/
> ---
> 
> (Updated Nov. 8, 2015, 3:57 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Bugs: MESOS-3367
> https://issues.apache.org/jira/browse/MESOS-3367
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix fetch parsing problem for URL with query.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 0b676f43e8529c6790824c8cf82e21a81d885ef1 
>   src/tests/fetcher_tests.cpp 46d0690887b6b6af4c7102753cae8db0a375fa08 
> 
> Diff: https://reviews.apache.org/r/40054/diff/
> 
> 
> Testing
> ---
> 
> # Add `FetcherTest.OSNetUriWithQueryTest` test case.
> # @bernd-mesos, @bmahler We could do a quick wround in old code. But I think 
> add a parsing function for URL maybe better, it also could fix the TODO item. 
> I think we also could update the code in fetcher to use process::http::URL, 
> so far I only change the Fetcher::basename function.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 40054: Fix fetch parsing problem for URL with query.

[haosdent huang]

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

(Updated Nov. 8, 2015, 3:57 p.m.)


Review request for mesos, Bernd Mathiske and Ben Mahler.


Changes
---

Rebase


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


Repository: mesos


Description
---

Fix fetch parsing problem for URL with query.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 0b676f43e8529c6790824c8cf82e21a81d885ef1 
  src/tests/fetcher_tests.cpp 46d0690887b6b6af4c7102753cae8db0a375fa08 

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


Testing
---

# Add `FetcherTest.OSNetUriWithQueryTest` test case.
# @bernd-mesos, @bmahler We could do a quick wround in old code. But I think 
add a parsing function for URL maybe better, it also could fix the TODO item. I 
think we also could update the code in fetcher to use process::http::URL, so 
far I only change the Fetcher::basename function.


Thanks,

haosdent huang

Re: Review Request 40053: Add URL::parse function.

[haosdent huang]

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

(Updated Nov. 8, 2015, 3:56 p.m.)


Review request for mesos, Bernd Mathiske and Ben Mahler.


Changes
---

Rebase


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


Repository: mesos


Description
---

Add URL::parse function.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/boost-1.53.0.tar.gz 
b867ae93a169b42773cab097acc6065f34f95e6e 
  3rdparty/libprocess/include/process/http.hpp 
90c9be122ee0c402b806d70fc818e3c03b15101a 
  3rdparty/libprocess/src/http.cpp 19eca9c718e15d5e7cb32be4bd8a3471299d9311 
  3rdparty/libprocess/src/tests/http_tests.cpp 
7eb4ef187b2cb358c370d0381db65b8e18668bab 

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


Testing
---

# Add `URLTest.Parsing` test case


Thanks,

haosdent huang

Re: Review Request 40054: Fix fetch parsing problem for URL with query.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [40053]

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

Error:
 2015-11-08 13:53:54 URL:https://reviews.apache.org/r/40053/diff/raw/ 
[3177448/3177448] -> "40053.patch" [1]
error: patch failed: 3rdparty/libprocess/include/process/http.hpp:68
error: 3rdparty/libprocess/include/process/http.hpp: patch does not apply
error: patch failed: 3rdparty/libprocess/src/http.cpp:31
error: 3rdparty/libprocess/src/http.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Nov. 8, 2015, 1:34 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40054/
> ---
> 
> (Updated Nov. 8, 2015, 1:34 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Bugs: MESOS-3367
> https://issues.apache.org/jira/browse/MESOS-3367
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix fetch parsing problem for URL with query.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 0b676f43e8529c6790824c8cf82e21a81d885ef1 
>   src/tests/fetcher_tests.cpp 46d0690887b6b6af4c7102753cae8db0a375fa08 
> 
> Diff: https://reviews.apache.org/r/40054/diff/
> 
> 
> Testing
> ---
> 
> # Add `FetcherTest.OSNetUriWithQueryTest` test case.
> # @bernd-mesos, @bmahler We could do a quick wround in old code. But I think 
> add a parsing function for URL maybe better, it also could fix the TODO item. 
> I think we also could update the code in fetcher to use process::http::URL, 
> so far I only change the Fetcher::basename function.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 40054: Fix fetch parsing problem for URL with query.

[haosdent huang]

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

(Updated Nov. 8, 2015, 1:34 p.m.)


Review request for mesos, Bernd Mathiske and Ben Mahler.


Changes
---

When uri not start with protocol specifier, treat it as a normal path directly.


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


Repository: mesos


Description
---

Fix fetch parsing problem for URL with query.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 0b676f43e8529c6790824c8cf82e21a81d885ef1 
  src/tests/fetcher_tests.cpp 46d0690887b6b6af4c7102753cae8db0a375fa08 

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


Testing
---

# Add `FetcherTest.OSNetUriWithQueryTest` test case.
# @bernd-mesos, @bmahler We could do a quick wround in old code. But I think 
add a parsing function for URL maybe better, it also could fix the TODO item. I 
think we also could update the code in fetcher to use process::http::URL, so 
far I only change the Fetcher::basename function.


Thanks,

haosdent huang

Re: Review Request 40056: Make hook execution order deterministic.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [40056]

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

Error:
 2015-11-08 13:03:54 URL:https://reviews.apache.org/r/40056/diff/raw/ 
[4469/4469] -> "40056.patch" [1]
error: patch failed: src/hook/manager.cpp:135
error: src/hook/manager.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Nov. 8, 2015, 12:58 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40056/
> ---
> 
> (Updated Nov. 8, 2015, 12:58 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Kapil Arya, and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3485
> https://issues.apache.org/jira/browse/MESOS-3485
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make hook execution order deterministic.
> 
> 
> Diffs
> -
> 
>   src/hook/manager.cpp d9e660a3b6f6d13d9f6b59f53bfc8a4f65af6df4 
> 
> Diff: https://reviews.apache.org/r/40056/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Review Request 40056: Make hook execution order deterministic.

[haosdent huang]

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

Review request for mesos, Ben Mahler and Kapil Arya.


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


Repository: mesos


Description
---

Make hook execution order deterministic.


Diffs
-

  src/hook/manager.cpp d9e660a3b6f6d13d9f6b59f53bfc8a4f65af6df4 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 39102: Added documentation for JSON resources.

[Adam B]

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

Ship it!


Ship It!

- Adam B


On Nov. 6, 2015, 4:18 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39102/
> ---
> 
> (Updated Nov. 6, 2015, 4:18 p.m.)
> 
> 
> Review request for mesos, Adam B and Neil Conway.
> 
> 
> Bugs: MESOS-2467
> https://issues.apache.org/jira/browse/MESOS-2467
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for JSON resources.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md 5dfc7fce286a23a10c8683122175b9bad858b2c1 
>   docs/configuration.md 195814cf918e018d8287113299163415b94ab09f 
>   src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8 
> 
> Diff: https://reviews.apache.org/r/39102/diff/
> 
> 
> Testing
> ---
> 
> Viewed the relevant documentation sections ('Attributes and Resources' & 
> 'Configuration') using the mesos-website-container: 
> https://github.com/mesosphere/mesos-website-container
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39018: Added JSON parsing for Resources.

[Adam B]

> On Nov. 6, 2015, 10:13 a.m., Greg Mann wrote:
> > src/tests/resources_tests.cpp, lines 593-613
> > 
> >
> > I fixed an unintended error in this jsonString, and then found that the 
> > parsing didn't return an error. It turns out that since 
> > Resources::validate() is called in the Resources constructor, this 
> > particular error gets silently ignored and the constructor returns an empty 
> > Resources object. This seems less than ideal to me, but as long as we want 
> > to continue error checking in the constructors, it's not clear to me 
> > exactly how we would propagate any errors produced there. In any case, with 
> > the existing constructors, the proper way to test this error is to check 
> > that we have an empty Resources object after parsing.

> the proper way to test this error is to check that we have an empty Resources 
> object after parsing.

Do it.


- Adam


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


On Nov. 6, 2015, 4:17 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> ---
> 
> (Updated Nov. 6, 2015, 4:17 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-2467
> https://issues.apache.org/jira/browse/MESOS-2467
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes code changes necessary for JSON parsing of Resources. 
> Documentation changes will be posted soon in another review 
> (https://reviews.apache.org/r/39102/).
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 
>   include/mesos/v1/resources.hpp fe8925ac851b74d1b37919f00afc7ed816f47ea5 
>   src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 
>   src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
>   src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 
> 
> Diff: https://reviews.apache.org/r/39018/diff/
> 
> 
> Testing
> ---
> 
> New code was added to `ResourcesTest.ParsingFromJSON`, and two new tests were 
> added: `ResourcesTest.ParsingFromJSONWithRoles` and 
> `ResourcesTest.ParsingFromJSONError`. These attempt to test common error 
> scenarios that might show up in user-supplied JSON.
> 
> `make check` was used to confirm that all tests pass.
> 
> The original resources parsing format is used throughout many other tests 
> (check `src/tests/sorter_tests.cpp`, for example), so it's clear that the 
> original parsing continues to work correctly.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39018: Added JSON parsing for Resources.

[Adam B]

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


Almost there, but I won't let you "silently ignore" any errors on my watch.


src/common/resources.cpp (line 271)


Red flag: "silently ignored" scares me. Don't you mean that an error in 
validate() will yield an empty Resource, which should have been checked 
before/after/during this call? Why can't we check Resources.isEmpty(Resource) 
here?



src/tests/resources_tests.cpp (line 603)


Ooh, that looks like the unintended JSON error you fixed.


- Adam B


On Nov. 6, 2015, 4:17 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> ---
> 
> (Updated Nov. 6, 2015, 4:17 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-2467
> https://issues.apache.org/jira/browse/MESOS-2467
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes code changes necessary for JSON parsing of Resources. 
> Documentation changes will be posted soon in another review 
> (https://reviews.apache.org/r/39102/).
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 
>   include/mesos/v1/resources.hpp fe8925ac851b74d1b37919f00afc7ed816f47ea5 
>   src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 
>   src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 
>   src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 
> 
> Diff: https://reviews.apache.org/r/39018/diff/
> 
> 
> Testing
> ---
> 
> New code was added to `ResourcesTest.ParsingFromJSON`, and two new tests were 
> added: `ResourcesTest.ParsingFromJSONWithRoles` and 
> `ResourcesTest.ParsingFromJSONError`. These attempt to test common error 
> scenarios that might show up in user-supplied JSON.
> 
> `make check` was used to confirm that all tests pass.
> 
> The original resources parsing format is used throughout many other tests 
> (check `src/tests/sorter_tests.cpp`, for example), so it's clear that the 
> original parsing continues to work correctly.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>