Re: Review Request 37703: Add docker exec command.

[haosdent huang]

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

(Updated Sept. 9, 2015, 9:15 a.m.)


Review request for mesos and Timothy Chen.


Changes
---

Fix compile error.


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


Repository: mesos


Description
---

Add docker exec command.


Diffs (updated)
-

  src/docker/docker.hpp aaf8884a20901ad3a440d545d4b6316b87fc3403 
  src/docker/docker.cpp 553e831029454d6d423842915b8bbfcaf19fa7f9 
  src/tests/containerizer/docker_tests.cpp 
babc7d8da4ed9d13b14bd69decd7f27fc7dfde89 

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


Testing
---

make check


Thanks,

haosdent huang

Re: Review Request 38197: Increment a metric when master drop a message

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38197]

All tests passed.

- Mesos ReviewBot


On Sept. 9, 2015, 2:02 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38197/
> ---
> 
> (Updated Sept. 9, 2015, 2:02 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Increment a metric when master drop a message
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac 
> 
> Diff: https://reviews.apache.org/r/38197/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38201: [MESOS-1187] precision errors with allocation calculations

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38201]

All tests passed.

- Mesos ReviewBot


On Sept. 9, 2015, 6:04 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38201/
> ---
> 
> (Updated Sept. 9, 2015, 6:04 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-1187
> https://issues.apache.org/jira/browse/MESOS-1187
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> As allocations are stored/transmitted as doubles many a times precision 
> errors creep in.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
> 
> Diff: https://reviews.apache.org/r/38201/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 37703: Add docker exec command.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37703]

All tests passed.

- Mesos ReviewBot


On Sept. 9, 2015, 9:15 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37703/
> ---
> 
> (Updated Sept. 9, 2015, 9:15 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-3291
> https://issues.apache.org/jira/browse/MESOS-3291
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add docker exec command.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp aaf8884a20901ad3a440d545d4b6316b87fc3403 
>   src/docker/docker.cpp 553e831029454d6d423842915b8bbfcaf19fa7f9 
>   src/tests/containerizer/docker_tests.cpp 
> babc7d8da4ed9d13b14bd69decd7f27fc7dfde89 
> 
> Diff: https://reviews.apache.org/r/37703/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 38160: Documented how to expedite event firing.

[Alexander Rukletsov]

> On Sept. 8, 2015, 3:44 p.m., Guangya Liu wrote:
> > docs/mesos-testing-patterns.md, line 10
> > 
> >
> > What does "Naïve" means?
> 
> haosdent huang wrote:
> Naïve waiting means just waiting until timeout. Like:
> ```
> AWAIT_READY(Future)
> ```
> would waiting `Future` to complete or timeout after 15 seconds.
> 
> Guangya Liu wrote:
> Should not it be "Naive"? The "Naïve" does not like to be a english word?
> 
> haosdent huang wrote:
> https://en.wiktionary.org/wiki/na%C3%AFve

@gyliu: do you think I should expand the comment or change wording here? if 
not, mind dropping the issue? Thanks!


- Alexander


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


On Sept. 7, 2015, 4:04 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38160/
> ---
> 
> (Updated Sept. 7, 2015, 4:04 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/mesos-testing-patterns.md d0d92ff569012677043a03412462cf4384096ba9 
> 
> Diff: https://reviews.apache.org/r/38160/diff/
> 
> 
> Testing
> ---
> 
> doc rendered in Markdown
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38160: Documented how to expedite event firing.

[Alexander Rukletsov]

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

(Updated Sept. 9, 2015, 2:28 p.m.)


Review request for mesos, Bernd Mathiske and Ben Mahler.


Changes
---

Bernd's comments.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/mesos-testing-patterns.md d0d92ff569012677043a03412462cf4384096ba9 

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


Testing
---

doc rendered in Markdown


Thanks,

Alexander Rukletsov

Re: Review Request 38161: Replaced a hard-coded number for registration backoff with a proper constant and extended comments.

[Alexander Rukletsov]

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

(Updated Sept. 9, 2015, 2:27 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Bernd's comments.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/fault_tolerance_tests.cpp 89cb18be96cd60fb77fbcc4acd08cebdcf1ba075 

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


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 38161: Replaced a hard-coded number for registration backoff with a proper constant and extended comments.

[Alexander Rukletsov]

> On Sept. 8, 2015, 2:30 p.m., Bernd Mathiske wrote:
> > src/tests/fault_tolerance_tests.cpp, line 705
> > 
> >
> > s/re-/
> > s/retry/

Let's keep "retry" for clarity (we send a second registration request).


- Alexander


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


On Sept. 7, 2015, 4:04 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38161/
> ---
> 
> (Updated Sept. 7, 2015, 4:04 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/fault_tolerance_tests.cpp 
> 89cb18be96cd60fb77fbcc4acd08cebdcf1ba075 
> 
> Diff: https://reviews.apache.org/r/38161/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38160: Documented how to expedite event firing.

[Alexander Rukletsov]

> On Sept. 8, 2015, 2:34 p.m., Bernd Mathiske wrote:
> > docs/mesos-testing-patterns.md, line 55
> > 
> >
> > s/re-/

Good catch, thanks!


- Alexander


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


On Sept. 7, 2015, 4:04 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38160/
> ---
> 
> (Updated Sept. 7, 2015, 4:04 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/mesos-testing-patterns.md d0d92ff569012677043a03412462cf4384096ba9 
> 
> Diff: https://reviews.apache.org/r/38160/diff/
> 
> 
> Testing
> ---
> 
> doc rendered in Markdown
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37282: Maintenance Primitives: Added InverseOffer to V1 API.

[Qian Zhang]

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

Ship it!


Ship It!

- Qian Zhang


On Sept. 3, 2015, 3:32 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37282/
> ---
> 
> (Updated Sept. 3, 2015, 3:32 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/internal/devolve.hpp 6e4306df78b9b8d2054e6550209341fd7b0972d6 
>   src/internal/devolve.cpp 0a069e51053b572a8d5dc95380732119504dd0c9 
>   src/internal/evolve.hpp 13e9f52da98567038ec717f394f79e526a1521e9 
>   src/internal/evolve.cpp 11ce9e77490e93f781ceebc33063d13953a11765 
>   src/messages/messages.proto 8977d8e0f3b16003128b6b9cab556a7b224f083c 
> 
> Diff: https://reviews.apache.org/r/37282/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 37177: Maintenance Primitives: Added inverse offers.

[Yong Qiao Wang]

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

Ship it!


Ship It!

- Yong Qiao Wang


On Sept. 2, 2015, 7:32 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37177/
> ---
> 
> (Updated Sept. 2, 2015, 7:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/maintenance/maintenance.hpp 
> 7fec3ffe063e766dcec4952001fa5c6e20d9eec8 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
>   src/master/allocator/mesos/allocator.hpp 
> aa55755a9c3250579e9366bdbc17a2449e95d659 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
>   src/master/master.hpp 594dd25f9aa9b6147680d0a838a77c3222941f4b 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
>   src/tests/hierarchical_allocator_tests.cpp 
> 9748ca0b3fee25dcec51c64d8ba84dbd4aaf 
>   src/tests/master_allocator_tests.cpp 
> 89331965553505f6b7eebf39ad27d943df816a24 
>   src/tests/mesos.hpp 906948d459b5a88a4ad7952801eb8c540b58c569 
>   src/tests/reservation_tests.cpp aeee36752573e3f401d3dca7d2d69c90d0e8bd6b 
>   src/tests/resource_offers_tests.cpp 
> 882a9ff4d09aace486182828bf43b643b0d0c519 
>   src/tests/slave_recovery_tests.cpp 4d137e0f1278fdacf71f101b1967df35bfbcdd23 
> 
> Diff: https://reviews.apache.org/r/37177/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 38046: Replace hard-coded reap interval with a constant

[Guangya Liu]

> On 九月 7, 2015, 4:22 p.m., Alexander Rukletsov wrote:
> > I would encourage you to check the reaping concept in 
> > `libprocess/reap.{hpp|cpp}`. Comments in those files should be sufficient 
> > to understand what's reaping and how we use it in Mesos. Though some of the 
> > changes you've made may be related to the reaping interval (I haven't deep 
> > dived into reviewing yet), some of them are definitely not (see examples 
> > below).
> > 
> > How did you choose candidates for replacing? I thought you did `grep`, but 
> > surprisingly there is no `reap_tests.cpp` in your RR. I would love us to 
> > carefully look at each `Seconds(1)` timeout in tests and either replace it 
> > by the reap interval or leave a comment about the nature of that "one 
> > second". What do you think?
> 
> Guangya Liu wrote:
> I found that we cannot upload one patch with both mesos core part and 
> 3rdparty code, I have just uploaded a patch for 3rd party for this: 
> https://reviews.apache.org/r/38168/
> 
> it is good to add comments for "Seconds(1)", can we handle this in 
> another patch? I want to focus on reap part for this patch, hope it is OK. ;-)
> 
> Alexander Rukletsov wrote:
> Sure, separate patch is even better.

I see that you are already working on those issues ;-)


- Guangya


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


On 九月 9, 2015, 3:01 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38046/
> ---
> 
> (Updated 九月 9, 2015, 3:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-1935
> https://issues.apache.org/jira/browse/MESOS-1935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace hard-coded reap interval with a constant
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/launch_tests.cpp 
> d211fc0f665988068c67836ef80916828a0df2bd 
>   src/tests/gc_tests.cpp ec27ac7f1fca7f425abcea0eafaac4fae905fb8f 
>   src/tests/slave_recovery_tests.cpp 6aae14a3c39b9aee76147b691b0170946e1120b5 
>   src/tests/slave_tests.cpp 5c1a3d36a5f67629aef275eeae12956c524e8102 
> 
> Diff: https://reviews.apache.org/r/38046/diff/
> 
> 
> Testing
> ---
> 
> make distcheck
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38046: Replace hard-coded reap interval with a constant

[Alexander Rukletsov]

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


Looks like we are converging here. One unclear thing I would like to ask you to 
clarify is the timeout in the garbage collection test.


src/tests/gc_tests.cpp (lines 805 - 807)


Why do you think this is related to reaping? Could you please explain?



src/tests/slave_recovery_tests.cpp (line 600)


I believe this one is not related to reaping, but rather to resources 
turnaround and allocation timeout. Let's restore the original here. Executor 
has been already reaped at this point.



src/tests/slave_recovery_tests.cpp (line 718)


Ditto.



src/tests/slave_recovery_tests.cpp (line 1312)


Ditto.



src/tests/slave_recovery_tests.cpp (line 2283)


Ditto.



src/tests/slave_recovery_tests.cpp (line 2436)


Ditto.


- Alexander Rukletsov


On Sept. 9, 2015, 3:01 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38046/
> ---
> 
> (Updated Sept. 9, 2015, 3:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-1935
> https://issues.apache.org/jira/browse/MESOS-1935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace hard-coded reap interval with a constant
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/launch_tests.cpp 
> d211fc0f665988068c67836ef80916828a0df2bd 
>   src/tests/gc_tests.cpp ec27ac7f1fca7f425abcea0eafaac4fae905fb8f 
>   src/tests/slave_recovery_tests.cpp 6aae14a3c39b9aee76147b691b0170946e1120b5 
>   src/tests/slave_tests.cpp 5c1a3d36a5f67629aef275eeae12956c524e8102 
> 
> Diff: https://reviews.apache.org/r/38046/diff/
> 
> 
> Testing
> ---
> 
> make distcheck
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38046: Replace hard-coded reap interval with a constant

[Guangya Liu]

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

(Updated 九月 9, 2015, 4:09 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


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


Repository: mesos


Description
---

Replace hard-coded reap interval with a constant


Diffs (updated)
-

  src/tests/containerizer/launch_tests.cpp 
d211fc0f665988068c67836ef80916828a0df2bd 
  src/tests/slave_recovery_tests.cpp 6aae14a3c39b9aee76147b691b0170946e1120b5 
  src/tests/slave_tests.cpp 5c1a3d36a5f67629aef275eeae12956c524e8102 

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


Testing
---

make distcheck


Thanks,

Guangya Liu

Re: Review Request 37826: Introduced conversion of JSON arrays to repeated protobufs.

[Alexander Rukletsov]

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

(Updated Sept. 9, 2015, 2:46 p.m.)


Review request for mesos, Joseph Wu and Michael Park.


Changes
---

Rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
57d5fdf45273c620655b44b5f5572290cffa4bf6 

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


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 38154: Switched to type traits for checking whether a type is a protobuf message.

[Alexander Rukletsov]

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

(Updated Sept. 9, 2015, 2:45 p.m.)


Review request for mesos and Michael Park.


Changes
---

Joseph's comment.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
57d5fdf45273c620655b44b5f5572290cffa4bf6 

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


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 37827: Added a test for converting JSON arrays to repeated protobufs.

[Alexander Rukletsov]

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

(Updated Sept. 9, 2015, 2:46 p.m.)


Review request for mesos, haosdent huang, Joseph Wu, and Michael Park.


Changes
---

Rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
c56d6a3098293eb3659b3066f10e875927ec3ac3 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 
cfc2803e42284f641879fb24bce1282215c8ea52 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 
a1d4084661345f9367c75f9db61279f032b93e69 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 
bbd36d39e9588eb8eea6d739451ad3bab029ca08 

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


Testing
---

make check (Mac OS 10.10.4)

**NOTE**: Filed [MESOS-3323](https://issues.apache.org/jira/browse/MESOS-3323) 
to clean up protobuf generation.


Thanks,

Alexander Rukletsov

Re: Review Request 38161: Replaced a hard-coded number for registration backoff with a proper constant and extended comments.

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 九月 9, 2015, 2:27 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38161/
> ---
> 
> (Updated 九月 9, 2015, 2:27 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/fault_tolerance_tests.cpp 
> 89cb18be96cd60fb77fbcc4acd08cebdcf1ba075 
> 
> Diff: https://reviews.apache.org/r/38161/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38168: Replace hard-coded reap interval with a constant for 3rdparty

[Guangya Liu]

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

(Updated 九月 9, 2015, 2:59 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


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


Repository: mesos


Description
---

Replace hard-coded reap interval with a constant for 3rdparty


Diffs
-

  3rdparty/libprocess/src/tests/reap_tests.cpp 
642ab97b28a6085dc9837f14ff36a3124387a03b 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 
ab7515325e5db0a4fd222bb982f51243d7b7e39d 

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


Testing (updated)
---

make distcheck


Thanks,

Guangya Liu

Re: Review Request 38046: Replace hard-coded reap interval with a constant

[Guangya Liu]

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

(Updated 九月 9, 2015, 3:01 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


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


Repository: mesos


Description
---

Replace hard-coded reap interval with a constant


Diffs
-

  src/tests/containerizer/launch_tests.cpp 
d211fc0f665988068c67836ef80916828a0df2bd 
  src/tests/gc_tests.cpp ec27ac7f1fca7f425abcea0eafaac4fae905fb8f 
  src/tests/slave_recovery_tests.cpp 6aae14a3c39b9aee76147b691b0170946e1120b5 
  src/tests/slave_tests.cpp 5c1a3d36a5f67629aef275eeae12956c524e8102 

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


Testing (updated)
---

make distcheck


Thanks,

Guangya Liu

Re: Review Request 37180: Maintenance Primitives: Implemented Master::inverseOffer.

[Yong Qiao Wang]

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

Ship it!


Ship It!

- Yong Qiao Wang


On Sept. 2, 2015, 7:32 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37180/
> ---
> 
> (Updated Sept. 2, 2015, 7:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 594dd25f9aa9b6147680d0a838a77c3222941f4b 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
> 
> Diff: https://reviews.apache.org/r/37180/diff/
> 
> 
> Testing
> ---
> 
> The tests break as expected.
> With the scheduler API change there are CHECKs that fail.
> Once we update the API these will be resolved.
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 38046: Replace hard-coded reap interval with a constant

[Alexander Rukletsov]

> On Sept. 7, 2015, 4:22 p.m., Alexander Rukletsov wrote:
> > I would encourage you to check the reaping concept in 
> > `libprocess/reap.{hpp|cpp}`. Comments in those files should be sufficient 
> > to understand what's reaping and how we use it in Mesos. Though some of the 
> > changes you've made may be related to the reaping interval (I haven't deep 
> > dived into reviewing yet), some of them are definitely not (see examples 
> > below).
> > 
> > How did you choose candidates for replacing? I thought you did `grep`, but 
> > surprisingly there is no `reap_tests.cpp` in your RR. I would love us to 
> > carefully look at each `Seconds(1)` timeout in tests and either replace it 
> > by the reap interval or leave a comment about the nature of that "one 
> > second". What do you think?
> 
> Guangya Liu wrote:
> I found that we cannot upload one patch with both mesos core part and 
> 3rdparty code, I have just uploaded a patch for 3rd party for this: 
> https://reviews.apache.org/r/38168/
> 
> it is good to add comments for "Seconds(1)", can we handle this in 
> another patch? I want to focus on reap part for this patch, hope it is OK. ;-)

Sure, separate patch is even better.


- Alexander


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


On Sept. 8, 2015, 6:16 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38046/
> ---
> 
> (Updated Sept. 8, 2015, 6:16 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-1935
> https://issues.apache.org/jira/browse/MESOS-1935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace hard-coded reap interval with a constant
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/launch_tests.cpp 
> d211fc0f665988068c67836ef80916828a0df2bd 
>   src/tests/gc_tests.cpp ec27ac7f1fca7f425abcea0eafaac4fae905fb8f 
>   src/tests/slave_recovery_tests.cpp 6aae14a3c39b9aee76147b691b0170946e1120b5 
>   src/tests/slave_tests.cpp 5c1a3d36a5f67629aef275eeae12956c524e8102 
> 
> Diff: https://reviews.apache.org/r/38046/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38168: Replace hard-coded reap interval with a constant for 3rdparty

[Alexander Rukletsov]

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


This looks good, thanks! Have you run `make distcheck`? I believe so, mind 
mentioning this in the testing section?

- Alexander Rukletsov


On Sept. 8, 2015, 2:41 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38168/
> ---
> 
> (Updated Sept. 8, 2015, 2:41 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-1935
> https://issues.apache.org/jira/browse/MESOS-1935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace hard-coded reap interval with a constant for 3rdparty
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/reap_tests.cpp 
> 642ab97b28a6085dc9837f14ff36a3124387a03b 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> ab7515325e5db0a4fd222bb982f51243d7b7e39d 
> 
> Diff: https://reviews.apache.org/r/38168/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38168: Replace hard-coded reap interval with a constant for 3rdparty

[Alexander Rukletsov]

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

Ship it!


Ship It!

- Alexander Rukletsov


On Sept. 9, 2015, 2:59 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38168/
> ---
> 
> (Updated Sept. 9, 2015, 2:59 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-1935
> https://issues.apache.org/jira/browse/MESOS-1935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace hard-coded reap interval with a constant for 3rdparty
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/reap_tests.cpp 
> 642ab97b28a6085dc9837f14ff36a3124387a03b 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> ab7515325e5db0a4fd222bb982f51243d7b7e39d 
> 
> Diff: https://reviews.apache.org/r/38168/diff/
> 
> 
> Testing
> ---
> 
> make distcheck
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38160: Documented how to expedite event firing.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38161, 38160]

All tests passed.

- Mesos ReviewBot


On Sept. 9, 2015, 2:28 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38160/
> ---
> 
> (Updated Sept. 9, 2015, 2:28 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/mesos-testing-patterns.md d0d92ff569012677043a03412462cf4384096ba9 
> 
> Diff: https://reviews.apache.org/r/38160/diff/
> 
> 
> Testing
> ---
> 
> doc rendered in Markdown
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Alexander Rukletsov]

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



include/mesos/values.hpp (line 21)


```
#include 
#include 
```



include/mesos/values.hpp (lines 58 - 59)


Empty line, please.



src/common/values.cpp (line 91)


s/Remove/Removes
s/indexes/indices

Mind refining a comment a bit? It looks like it evolved during time and 
contains artifacts from previous versions : ).



src/common/values.cpp (line 95)


I would pass set by pointer here to explicitly document that it may be 
changed in the function. In that case the caller will have to `` 
which hopefully make them look closer to the signature. Also, let's mention 
that in the comment.



src/common/values.cpp (line 105)


`nextRemove` cannot be greater than `nextElement`, because you do 
`deleteSet.insert(nextElement);` at the end. The only exception is the first 
iteration. How about initializing `nextElement = *deleteSet.begin() + 1` and 
remove this line?



src/common/values.cpp (line 136)


s/next/nested ?



src/common/values.cpp (lines 157 - 178)


I think we can conflate this two cases, how about this:
```
if (range->end() + 1 >= current.begin()) {
  range->set_end(std::max(range.end(), current.end()));
  deleteSet.insert(y);
  ++i;
} else {
  break;
}
```



src/common/values.cpp (lines 268 - 273)


Can we insert a new range into an appropriate position? This should be 
`O(n)` instead of `O(n log n)`


- Alexander Rukletsov


On Sept. 8, 2015, 7:25 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 8, 2015, 7:25 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38046: Replace hard-coded reap interval with a constant

[Guangya Liu]

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



src/tests/gc_tests.cpp (lines 805 - 807)


My bad, this does not seems related to reap because it is not waiting for a 
process, will rollback this one.


- Guangya Liu


On Sept. 9, 2015, 4:09 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38046/
> ---
> 
> (Updated Sept. 9, 2015, 4:09 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-1935
> https://issues.apache.org/jira/browse/MESOS-1935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace hard-coded reap interval with a constant
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/launch_tests.cpp 
> d211fc0f665988068c67836ef80916828a0df2bd 
>   src/tests/slave_recovery_tests.cpp 6aae14a3c39b9aee76147b691b0170946e1120b5 
>   src/tests/slave_tests.cpp 5c1a3d36a5f67629aef275eeae12956c524e8102 
> 
> Diff: https://reviews.apache.org/r/38046/diff/
> 
> 
> Testing
> ---
> 
> make distcheck
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38214: Add a REASON for TASK_LOST with hard slave drain

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38214]

All tests passed.

- Mesos ReviewBot


On Sept. 9, 2015, 2:43 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38214/
> ---
> 
> (Updated Sept. 9, 2015, 2:43 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2077
> https://issues.apache.org/jira/browse/MESOS-2077
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensure that TASK_LOST for a hard slave drain (SIGUSR1) include a
> reason.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c 
>   src/master/master.hpp e1331851c19e3372a4a525dcfd7ba2a01c3e97a6 
>   src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac 
>   src/messages/messages.proto 8977d8e0f3b16003128b6b9cab556a7b224f083c 
>   src/slave/slave.cpp 5e5522e1254a5ed6084de36782753f9aad2894c4 
> 
> Diff: https://reviews.apache.org/r/38214/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 35984: Added tests for /reserve and /unreserve HTTP endpoints.

[Michael Park]

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

(Updated Sept. 9, 2015, 10:12 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, Joris 
Van Remoortere, and Vinod Kone.


Changes
---

Addressed Jie's comments. Introduced helpers and reduced tests.


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


Repository: mesos


Description
---

See summary


Diffs (updated)
-

  src/Makefile.am 5fdca0f574e7e08c4b1aebed0fac39140c19adfe 
  src/tests/reservation_endpoints_tests.cpp PRE-CREATION 

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


Testing
---

(1) Added `src/tests/reservation_endpoints_tests.cpp`
(2) `make check`


Thanks,

Michael Park

Re: Review Request 37445: Fix typos in style guide.

[Niklas Nielsen]

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

Ship it!


Ship It!

- Niklas Nielsen


On Aug. 13, 2015, 2:58 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37445/
> ---
> 
> (Updated Aug. 13, 2015, 2:58 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix typos in style guide.
> 
> 
> Diffs
> -
> 
>   docs/mesos-c++-style-guide.md 9c1a00c32043fa10038e38bd7cbc561aafcd6ea0 
> 
> Diff: https://reviews.apache.org/r/37445/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 38149: Add message_call_received metrics for master

[Guangya Liu]

> On 九月 9, 2015, 5:37 p.m., Vinod Kone wrote:
> > What's the use case for this metric? For old style messages, we had per 
> > framework "messages_received" because it was used for rate limiting.

I see there is a "// TODO(vinod): Add metrics for calls." in comments and also 
adding this metrics can help used to trace all of the call messages. Thanks.


- Guangya


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


On 九月 9, 2015, 1:42 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38149/
> ---
> 
> (Updated 九月 9, 2015, 1:42 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add message_call_received metrics for master
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac 
>   src/master/metrics.hpp 2d07a16f2dc6811973c82259c3cccff07401b542 
>   src/master/metrics.cpp d79206f08181c99641bef697be57b5d57b627285 
>   src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1 
>   src/tests/metrics_tests.cpp 3e9d7c2eadfcdd2522d65e4a55dc76808ba39159 
> 
> Diff: https://reviews.apache.org/r/38149/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38233: os: add swap information to memory().

[Cong Wang]

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



3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp (line 757)


Bytes(usage.xsu_total * pageSize)?


- Cong Wang


On Sept. 9, 2015, 9:32 p.m., Chi Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38233/
> ---
> 
> (Updated Sept. 9, 2015, 9:32 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: mesos-2918
> https://issues.apache.org/jira/browse/mesos-2918
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> os: add swap information to memory().
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/os.hpp 
> 82ccd766bd22393f48be4554c7da46338dd92d1f 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> 5d2f39d9a9d963225bf463572cb8fe99dd9aa6f5 
> 
> Diff: https://reviews.apache.org/r/38233/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chi Zhang
> 
>

Review Request 38239: Moved appc.{hpp|cpp} into slave/containerizer/provisioners/appc/ folder.

[Jiang Yan Xu]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Moved appc.{hpp|cpp} into slave/containerizer/provisioners/appc/ folder.


Diffs
-

  src/Makefile.am 0a8ef6d8551cf177cb565b2a443c05e8eea5ab1c 
  src/slave/containerizer/provisioner.cpp 
95894c0ed97388cf2830058c13804b6baf5daad4 
  src/slave/containerizer/provisioners/appc.hpp  
  src/slave/containerizer/provisioners/appc.cpp 
d54b6882346df7121ec63803810fab979ac0848a 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 38031: [3/5] Integer Precision for JSON <-> Protobuf conversions.

[Joseph Wu]

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

(Updated Sept. 9, 2015, 3:20 p.m.)


Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, 
Michael Park, and Vinod Kone.


Changes
---

Deal with `uint64_t` gracefully (i.e. without completely losing the value 
during conversion).


Summary (updated)
-

[3/5] Integer Precision for JSON <-> Protobuf conversions.


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


Repository: mesos


Description (updated)
---

* Changes JSON::Number to keep track of whether it is floating, signed 
integral, or unsigned integral.
* Changes how JSON::Number is stringified, to ensure that stringified doubles 
are parsed as JSON doubles.
* Added one test for integer precision between String <-> JSON <-> Protobuf 
conversions.
* Added one test for JSON::Number comparisons.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 
f28138c4682c41e94ab6c7641a78d66b2f9daa5f 
  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
57d5fdf45273c620655b44b5f5572290cffa4bf6 
  3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 
850650c269e9be24c0f1ae81b8aa8725f8a0c151 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
c56d6a3098293eb3659b3066f10e875927ec3ac3 

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


Testing (updated)
---

No testing done until the last patch in the chain.

However, this patch does breaks some libprocess and mesos tests (because 
JSON::Number is stored differently), which are fixed in the following two 
reviews.


Thanks,

Joseph Wu

Re: Review Request 38030: [2/5] Integer Precision for JSON <-> Protobuf conversions.

[Joseph Wu]

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

(Updated Sept. 9, 2015, 3:20 p.m.)


Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, 
and Vinod Kone.


Changes
---

Ready for wider review.


Summary (updated)
-

[2/5] Integer Precision for JSON <-> Protobuf conversions.


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


Repository: mesos


Description
---

Add PICOJSON_USE_INT64 flag to mesos compilation flags.


Diffs
-

  src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 

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


Testing (updated)
---

No testing done until the last patch in the chain.


Thanks,

Joseph Wu

Re: Review Request 38074: Calculate schedule latency with trace events

[Cong Wang]

> On Sept. 5, 2015, 2:31 a.m., Guangya Liu wrote:
> > src/slave/flags.cpp, line 306
> > 
> >
> > Interval between which two values?

Interval between two sched sampling. Same as perf events sampling.


> On Sept. 5, 2015, 2:31 a.m., Guangya Liu wrote:
> > src/slave/flags.cpp, line 307
> > 
> >
> > Does this also affect the event, so what is the difference of this flag 
> > and sched_sample_duration?

You need to read the help message, that one is the duration of sampling.


- Cong


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


On Sept. 4, 2015, 11:16 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38074/
> ---
> 
> (Updated Sept. 4, 2015, 11:16 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Finally, calculate schedule latency with the sched trace events, and add it 
> to our statistics
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c 
>   src/slave/containerizer/isolators/cgroups/perf_event.hpp 
> 1f722ef3ef7ab7fce5542d4affae961d6cec2406 
>   src/slave/containerizer/isolators/cgroups/perf_event.cpp 
> 7dc8b7a99074b74ade019ef4df296780650a2e4e 
>   src/slave/flags.hpp b8335aa585376d27b71897f8cbaefaa12f4b3a5c 
>   src/slave/flags.cpp 7539441c685828027db07173e62a4e5fc1e8b54d 
> 
> Diff: https://reviews.apache.org/r/38074/diff/
> 
> 
> Testing
> ---
> 
> manual tests
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 38028: [1/5] Integer Precision for JSON <-> Protobuf conversions.

[Joseph Wu]

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

(Updated Sept. 9, 2015, 3:20 p.m.)


Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, 
and Vinod Kone.


Changes
---

Ready for wider review.


Summary (updated)
-

[1/5] Integer Precision for JSON <-> Protobuf conversions.


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


Repository: mesos


Description
---

Updates PicoJSON to version 1.3.0 and adds a -DPICOJSON_USE_INT64 flag to the 
compilation flags.


Diffs
-

  3rdparty/libprocess/3rdparty/CMakeLists.txt 997cc0d 
  3rdparty/libprocess/3rdparty/Makefile.am eb34251 
  3rdparty/libprocess/3rdparty/picojson-1.3.0.tar.gz PRE-CREATION 
  3rdparty/libprocess/3rdparty/picojson-4f93734.tar.gz 
f52dacd1c51ef5c533d174f63539a9b25e3d9180 
  3rdparty/libprocess/3rdparty/versions.am f44c715 
  3rdparty/libprocess/Makefile.am 6361ac6 

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


Testing (updated)
---

No testing done until the last patch in the chain.


Thanks,

Joseph Wu

Re: Review Request 38031: [3/5] Integer Precision for JSON <-> Protobuf conversions.

[Joseph Wu]

> On Sept. 9, 2015, 3:24 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp, lines 103-110
> > 
> >
> > Let's make this overlapped storage private and add accessors. We can 
> > postfix `_` to distinguish the variable names of the storage so that we can 
> > write the functions like this:
> > 
> > ```
> > double value() const 
> > {
> >   CHECK(type == FLOATING) << error_msg;
> >   return value_;
> > }
> > ```
> > 
> > This has the advantage of forcing us to change all the usage sites and 
> > make sure we make the right decision at each point.

I thought we didn't want to change all the tests (which is where we find usages 
of `.value`).  That's why the double printing was changed instead.

Also, can you take another look at the accessor?


- Joseph


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


On Sept. 9, 2015, 3:20 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38031/
> ---
> 
> (Updated Sept. 9, 2015, 3:20 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Michael Park, and Vinod Kone.
> 
> 
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * Changes JSON::Number to keep track of whether it is floating, signed 
> integral, or unsigned integral.
> * Changes how JSON::Number is stringified, to ensure that stringified doubles 
> are parsed as JSON doubles.
> * Added one test for integer precision between String <-> JSON <-> Protobuf 
> conversions.
> * Added one test for JSON::Number comparisons.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 
> f28138c4682c41e94ab6c7641a78d66b2f9daa5f 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 57d5fdf45273c620655b44b5f5572290cffa4bf6 
>   3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 
> 850650c269e9be24c0f1ae81b8aa8725f8a0c151 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
> c56d6a3098293eb3659b3066f10e875927ec3ac3 
> 
> Diff: https://reviews.apache.org/r/38031/diff/
> 
> 
> Testing
> ---
> 
> No testing done until the last patch in the chain.
> 
> However, this patch does breaks some libprocess and mesos tests (because 
> JSON::Number is stored differently), which are fixed in the following two 
> reviews.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 37824: mesos: Updates for google-glog upgrade to 0.3.4

[Niklas Nielsen]

> On Aug. 27, 2015, 1:27 p.m., Mesos ReviewBot wrote:
> > Bad patch!
> > 
> > Reviews applied: [37823]
> > 
> > Failed command: ./support/apply-review.sh -n -r 37823
> > 
> > Error:
> >  2015-08-27 20:25:09 URL:https://reviews.apache.org/r/37823/diff/raw/ 
> > [9017/9017] -> "37823.patch" [1]
> > error: missing binary patch data for 
> > '3rdparty/libprocess/3rdparty/glog-0.3.4.tar.gz'
> > error: binary patch does not apply to 
> > '3rdparty/libprocess/3rdparty/glog-0.3.4.tar.gz'
> > error: 3rdparty/libprocess/3rdparty/glog-0.3.4.tar.gz: patch does not apply
> > Failed to apply patch

Hi Neil, maybe you need to make an explicit diff with --binary


- Niklas


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


On Aug. 27, 2015, 1:11 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37824/
> ---
> 
> (Updated Aug. 27, 2015, 1:11 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3322
> https://issues.apache.org/jira/browse/MESOS-3322
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See MESOS-3322.
> 
> 
> Diffs
> -
> 
>   LICENSE c3aaa437af10533132698df3348114195d338965 
>   src/python/native/ext_modules.py.in 
> 4682e5eed0f7be23fb48ef628e1bebc7741431d7 
>   support/mesos-style.py 66b45692c3c04f68358b63d52e4d87934f241bd7 
> 
> Diff: https://reviews.apache.org/r/37824/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 35984: Added tests for /reserve and /unreserve HTTP endpoints.

[Michael Park]

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

(Updated Sept. 9, 2015, 10:27 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, Joris 
Van Remoortere, and Vinod Kone.


Changes
---

Wrapped comments at 80.


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


Repository: mesos


Description
---

See summary


Diffs (updated)
-

  src/Makefile.am 5fdca0f574e7e08c4b1aebed0fac39140c19adfe 
  src/tests/reservation_endpoints_tests.cpp PRE-CREATION 

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


Testing
---

(1) Added `src/tests/reservation_endpoints_tests.cpp`
(2) `make check`


Thanks,

Michael Park

Re: Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.

[Niklas Nielsen]

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

Ship it!


Minor nits


3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp (line 401)


No need for the else block when you return at line 400



3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp (line 402)


An alternative could have been:

IP(prefix == 0 ? 0 : 0x << (32 - prefix))

Will let you decide what is easier to read.
I like it in terms of not writing IPNetwork twice


- Niklas Nielsen


On Aug. 28, 2015, 1:02 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37903/
> ---
> 
> (Updated Aug. 28, 2015, 1:02 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3328
> https://issues.apache.org/jira/browse/MESOS-3328
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous coding would try to shift a uint32_t value 32 bits to the left; 
> per
> C++ spec, this yields undefined behavior. On my machine, this resulted in
> treating a prefix of 0 as equivalent to a prefix of 32, which is obviously
> wrong.
> 
> Spotted via ubsan: see MESOS-3328.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
> 1ad119d54820e97497b1773518875be25ddbf98a 
>   3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
> b0cbcb38cfcb923ec7c185bacf139ceb0a28924f 
> 
> Diff: https://reviews.apache.org/r/37903/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

[Jie Yu]

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


First pass. Haven't looked at LocalStore and MetadataManager in details yet.


src/messages/docker_provisioner.proto (line 28)


I would put this message under 
`src/slave/containerizer/provisioners/docker/message.proto`

since this message is docker provisioner specific.

ALso, why not unify this with DockerImage and ImageName? I.e.,

```
message DockerImage {
  message Name {
...
  }
  
  required Name name = 1;
  repeated string layer_ids = 2;
}

message DockerImages {
  repeated DockerImage images = 1;
};
```



src/slave/containerizer/provisioners/docker.hpp (line 19)


```
__DOCKER_PROVISIONER_HPP__
```

I'll change the appc one as well.

Also, I think this should be put into
```
src/slave/containerizer/provisioners/docker/provisioner.hpp
```

I'll change the appc one accordingly as well.



src/slave/containerizer/provisioners/docker.hpp (line 54)


Two lines apart.



src/slave/containerizer/provisioners/docker.hpp (line 59)


Can you put ImageName and DockerImage into a separate file under 
src/slave/containerizer/provisioners/docker

I feels strange when provisioner header depends on store header and store 
header depends on provisioner header.

Also, a second thought.

Why not make 'ImageName' and 'DockerImage' a protobuf as well?



src/slave/containerizer/provisioners/docker.hpp (line 61)


s/create/parse/?

s/name/value/



src/slave/containerizer/provisioners/docker.hpp (line 88)


Any reason those fields are not const?



src/slave/containerizer/provisioners/docker.hpp (line 112)


Can you just call it Image given it's under docker namespace.



src/slave/containerizer/provisioners/docker.hpp (line 124)


Two lines apart.



src/slave/containerizer/provisioners/docker.hpp (line 127)


Two lines apart.



src/slave/containerizer/provisioners/docker.hpp (line 159)


Kill one blank line here.



src/slave/containerizer/provisioners/docker.cpp (lines 59 - 61)


Why do you need this function. Can you move all the logics in this function 
to DockerProvisioner::create?

Feels free to make DockerProvisionerProcess constructor public because it's 
in a source file (i.e., hidden).



src/slave/containerizer/provisioners/docker.cpp (line 100)


Please move this function to a separate source file.



src/slave/containerizer/provisioners/docker.cpp (line 120)


Two lines apart. Please do a sweep to fix all such occurances.



src/slave/containerizer/provisioners/docker.cpp (lines 243 - 248)


OK, this is problematic if default container info is used. What if the 
framework launches a task that does not have container info specified, but 
mesos slave provides a default container info. The container will be treated as 
unknown orphans and thus destroyed.

FYI, we checkpoint the ExecutorInfo/TaskInfo before filling the default 
container info.

What you need to do here is to remove the 'if' check here.



src/slave/containerizer/provisioners/docker.cpp (line 306)


s/VLOG/LOG(INFO)/

"Destroying rootfs '" << rootfs << "' from an unknown orphan container " << 
containerId;



src/slave/containerizer/provisioners/docker/local_store.hpp (lines 48 - 50)


Please reorder these two methods.



src/slave/containerizer/provisioners/docker/local_store.cpp (line 19)


Please fix the include order please.



src/slave/containerizer/provisioners/docker/local_store.cpp (lines 60 - 62)


Similar to provisioner, you can get rid of this 'create' function, instead 
you can expose LocalStoreProcess constructor.



src/slave/containerizer/provisioners/docker/local_store.cpp (lines 103 - 116)

Re: Review Request 38239: Moved appc.{hpp|cpp} into slave/containerizer/provisioners/appc/ folder.

[Jie Yu]

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



src/Makefile.am (line 485)


Please rename appc.hpp|cpp to provisioner.hpp|cpp


- Jie Yu


On Sept. 10, 2015, 12:27 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38239/
> ---
> 
> (Updated Sept. 10, 2015, 12:27 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved appc.{hpp|cpp} into slave/containerizer/provisioners/appc/ folder.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 0a8ef6d8551cf177cb565b2a443c05e8eea5ab1c 
>   src/slave/containerizer/provisioner.cpp 
> 95894c0ed97388cf2830058c13804b6baf5daad4 
>   src/slave/containerizer/provisioners/appc.hpp  
>   src/slave/containerizer/provisioners/appc.cpp 
> d54b6882346df7121ec63803810fab979ac0848a 
> 
> Diff: https://reviews.apache.org/r/38239/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 38240: Renamed appc_backend flag to appc_provisioner_backend.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Sept. 10, 2015, 12:28 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38240/
> ---
> 
> (Updated Sept. 10, 2015, 12:28 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed appc_backend flag to appc_provisioner_backend.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioners/appc/appc.cpp 
> d54b6882346df7121ec63803810fab979ac0848a 
>   src/slave/flags.hpp b8335aa585376d27b71897f8cbaefaa12f4b3a5c 
>   src/slave/flags.cpp 7539441c685828027db07173e62a4e5fc1e8b54d 
>   src/tests/containerizer/appc_provisioner_tests.cpp 
> f30e1a1dee9a68be01133117339d91771f15988c 
> 
> Diff: https://reviews.apache.org/r/38240/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Review Request 38240: Renamed appc_backend flag to appc_provisioner_backend.

[Jiang Yan Xu]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Renamed appc_backend flag to appc_provisioner_backend.


Diffs
-

  src/slave/containerizer/provisioners/appc/appc.cpp 
d54b6882346df7121ec63803810fab979ac0848a 
  src/slave/flags.hpp b8335aa585376d27b71897f8cbaefaa12f4b3a5c 
  src/slave/flags.cpp 7539441c685828027db07173e62a4e5fc1e8b54d 
  src/tests/containerizer/appc_provisioner_tests.cpp 
f30e1a1dee9a68be01133117339d91771f15988c 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 35984: Added tests for /reserve and /unreserve HTTP endpoints.

[Michael Park]

> On Aug. 26, 2015, 6:33 a.m., Jie Yu wrote:
> > src/tests/reservation_endpoints_tests.cpp, line 109
> > 
> >
> > Are we allowed to use cxx11 raw string literals now? This is definitely 
> > better!

I found out there are `operator<<` for `JSON::Object` and `JSON::Array`, I 
think converting `Resources` to `JSON::Array` and invoking `stringify` on it is 
the right way to do this.


- Michael


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


On Sept. 9, 2015, 10:12 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35984/
> ---
> 
> (Updated Sept. 9, 2015, 10:12 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Ben Mahler, Jie Yu, Joris 
> Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-2600
> https://issues.apache.org/jira/browse/MESOS-2600
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5fdca0f574e7e08c4b1aebed0fac39140c19adfe 
>   src/tests/reservation_endpoints_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/35984/diff/
> 
> 
> Testing
> ---
> 
> (1) Added `src/tests/reservation_endpoints_tests.cpp`
> (2) `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 38028: [1/5] Integer Precision for JSON <-> Protobuf conversions.

[Joris Van Remoortere]

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

Ship it!


- Joris Van Remoortere


On Sept. 9, 2015, 10:20 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38028/
> ---
> 
> (Updated Sept. 9, 2015, 10:20 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updates PicoJSON to version 1.3.0 and adds a -DPICOJSON_USE_INT64 flag to the 
> compilation flags.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/CMakeLists.txt 997cc0d 
>   3rdparty/libprocess/3rdparty/Makefile.am eb34251 
>   3rdparty/libprocess/3rdparty/picojson-1.3.0.tar.gz PRE-CREATION 
>   3rdparty/libprocess/3rdparty/picojson-4f93734.tar.gz 
> f52dacd1c51ef5c533d174f63539a9b25e3d9180 
>   3rdparty/libprocess/3rdparty/versions.am f44c715 
>   3rdparty/libprocess/Makefile.am 6361ac6 
> 
> Diff: https://reviews.apache.org/r/38028/diff/
> 
> 
> Testing
> ---
> 
> No testing done until the last patch in the chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 38030: [2/5] Integer Precision for JSON <-> Protobuf conversions.

[Joris Van Remoortere]

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

Ship it!


- Joris Van Remoortere


On Sept. 9, 2015, 10:20 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38030/
> ---
> 
> (Updated Sept. 9, 2015, 10:20 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-3345
> https://issues.apache.org/jira/browse/MESOS-3345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add PICOJSON_USE_INT64 flag to mesos compilation flags.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
> 
> Diff: https://reviews.apache.org/r/38030/diff/
> 
> 
> Testing
> ---
> 
> No testing done until the last patch in the chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 38077: [5/5] Integer Precision for JSON <-> Protobuf conversions.

[Joris Van Remoortere]

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

Ship it!


- Joris Van Remoortere


On Sept. 9, 2015, 10:20 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38077/
> ---
> 
> (Updated Sept. 9, 2015, 10:20 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Address a TODO on one test, which used a workaround for double-precision 
> comparison.
> 
> 
> Diffs
> -
> 
>   src/tests/monitor_tests.cpp 53fb53eeea7d444ee043eb4569a52fead7ad0da8 
> 
> Diff: https://reviews.apache.org/r/38077/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 37773: Docker: Adding registry client.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37871, 37427, 37773]

All tests passed.

- Mesos ReviewBot


On Sept. 9, 2015, 8:08 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 9, 2015, 8:08 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 4ef58cd89f4a92770a7e7a8624a7befc801c6d69 
>   src/slave/containerizer/provisioners/docker/registry_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/registry_client.cpp 
> PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp 
> ff29d562c7f1bd5f0579e97cdbd60c2b2f36329e 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

[Jojy Varghese]

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

(Updated Sept. 9, 2015, 11:47 p.m.)


Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.


Changes
---

Added more info to manifest response


Repository: mesos


Description
---

Added implementation for docker registry's Get Manifest and Get Blob APIs.


Diffs (updated)
-

  docs/persistent-volume.md b5dd6d8ec68d8ed7dd4787ffc9973bbe6c49965a 
  src/Makefile.am 4ef58cd89f4a92770a7e7a8624a7befc801c6d69 
  src/slave/containerizer/provisioners/docker/registry_client.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/registry_client.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp 
ff29d562c7f1bd5f0579e97cdbd60c2b2f36329e 

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


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

[Timothy Chen]

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



docs/persistent-volume.md (line 227)


?


- Timothy Chen


On Sept. 9, 2015, 11:47 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 9, 2015, 11:47 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md b5dd6d8ec68d8ed7dd4787ffc9973bbe6c49965a 
>   src/Makefile.am 4ef58cd89f4a92770a7e7a8624a7befc801c6d69 
>   src/slave/containerizer/provisioners/docker/registry_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/registry_client.cpp 
> PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp 
> ff29d562c7f1bd5f0579e97cdbd60c2b2f36329e 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37773: Docker: Adding registry client.

[Timothy Chen]

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



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 57)


Mannifest -> Manifest



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 81)


Remove extra space.


- Timothy Chen


On Sept. 9, 2015, 11:47 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 9, 2015, 11:47 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md b5dd6d8ec68d8ed7dd4787ffc9973bbe6c49965a 
>   src/Makefile.am 4ef58cd89f4a92770a7e7a8624a7befc801c6d69 
>   src/slave/containerizer/provisioners/docker/registry_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/registry_client.cpp 
> PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp 
> ff29d562c7f1bd5f0579e97cdbd60c2b2f36329e 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37714: Updated Multimap and multihashmap so their signatures resemble that of hashmap and hashset.

[Michael Park]

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



3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp (line 28)


Not yours, but `s/std::multimap/std::unordered_multimap/`.



3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp (line 55)


`s/other/multimap/` -- here and below



3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp (lines 60 - 
62)


Looks like this fits in one line?



3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp (lines 84 - 
86)


Looks like this fits in one line?



3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp (lines 96 - 
97)


`s/std::make_pair(key, value)/{key, value}/`



3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp (lines 107 - 
117)


Can we just `auto` all of this away?

```
auto range =
  std::unordered_multimap::equal_range(key);
for (auto it = range.first; it != range.second; ++it) {
  values.push_back(it->second);
}
```



3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp (lines 146 - 
159)


Let's also use `auto` here similar to the above suggestion?



3rdparty/libprocess/3rdparty/stout/include/stout/multimap.hpp (lines 34 - 37)


Can I ask why we want these? Presumably the purpose of these classes are to 
eliminate the need for their `std::` counterparts.

If we look at a class like `Set`, it inherits from `std::set` but doesn't 
construct off of `std::set`. Similarly, `hashmap` inherits from 
`std::unordered_map`, but doesn't construct off of `std::unordered_map`.


- Michael Park


On Aug. 28, 2015, 8:11 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37714/
> ---
> 
> (Updated Aug. 28, 2015, 8:11 a.m.)
> 
> 
> Review request for mesos, Joerg Schad, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-2924
> https://issues.apache.org/jira/browse/MESOS-2924
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds extra template parameters to `multihashmap` which offer control over the 
> hash function to use as well as the equality operator.
> 
> Implements initializer_list, copy and move constructors for both, 
> `multihashmap` and `Multimap` in a similar way as it was done for `hashmap` 
> and `hashset`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp 
> d9e4031cee64e48ad50541c04ca11e7861d0a17c 
>   3rdparty/libprocess/3rdparty/stout/include/stout/multimap.hpp 
> fb3e7a1d0377001389980302342813217f49cf5f 
>   3rdparty/libprocess/3rdparty/stout/tests/multimap_tests.cpp 
> 535cd2d10e3074c86c149ce85b205e73ca42ddd3 
> 
> Diff: https://reviews.apache.org/r/37714/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 37273: [1/2]Add CMake macro VsBuildCommand in libprocess.

[haosdent huang]

> On Sept. 8, 2015, 9:44 p.m., Joseph Wu wrote:
> > Just a bit more cleanup, and I think this would be good to go.
> > 
> > If possible, you should coordinate with Alex Clemmer to make sure this 
> > change doesn't conflict/break what he's working on 
> > (https://reviews.apache.org/r/37019/).
> 
> haosdent huang wrote:
> Thanks a lot.
> 
> Alex Clemmer wrote:
> I do have things to say about this review, but I need to get the other 
> stuff checked in with Joris first. There are some things here that I'll ask 
> about, like why we moved the protobuf stuff out of processtestconfigure.

Yes, when I build 3rdparty, I found protobuf is required(see some error like 
could not found protobuf header files and objects) not only when test stage. So 
I move it from processtestconfigure to 3rdparty.


- haosdent


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


On Sept. 9, 2015, 2:40 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37273/
> ---
> 
> (Updated Sept. 9, 2015, 2:40 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add CMake macro VsBuildCommand in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/CMakeLists.txt 
> 997cc0d0e316e316136d4746e50e9e292a82b36b 
>   3rdparty/libprocess/cmake/ProcessConfigure.cmake 
> 12506a1369de005285268f895f365aba0c560f78 
>   3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 
> 9e4dcb83a8cc4e95a2a38573944f6b38e2eac76e 
>   3rdparty/libprocess/cmake/macros/Noop.cmake PRE-CREATION 
>   3rdparty/libprocess/cmake/macros/VsBuildCommand.bat PRE-CREATION 
>   3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37273/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37273: [1/2]Add CMake macro VsBuildCommand in libprocess.

[Alex Clemmer]

> On Sept. 8, 2015, 9:44 p.m., Joseph Wu wrote:
> > Just a bit more cleanup, and I think this would be good to go.
> > 
> > If possible, you should coordinate with Alex Clemmer to make sure this 
> > change doesn't conflict/break what he's working on 
> > (https://reviews.apache.org/r/37019/).
> 
> haosdent huang wrote:
> Thanks a lot.

I do have things to say about this review, but I need to get the other stuff 
checked in with Joris first. There are some things here that I'll ask about, 
like why we moved the protobuf stuff out of processtestconfigure.


- Alex


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


On Sept. 9, 2015, 2:40 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37273/
> ---
> 
> (Updated Sept. 9, 2015, 2:40 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add CMake macro VsBuildCommand in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/CMakeLists.txt 
> 997cc0d0e316e316136d4746e50e9e292a82b36b 
>   3rdparty/libprocess/cmake/ProcessConfigure.cmake 
> 12506a1369de005285268f895f365aba0c560f78 
>   3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 
> 9e4dcb83a8cc4e95a2a38573944f6b38e2eac76e 
>   3rdparty/libprocess/cmake/macros/Noop.cmake PRE-CREATION 
>   3rdparty/libprocess/cmake/macros/VsBuildCommand.bat PRE-CREATION 
>   3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37273/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 38143: Adding executor HTTP API protobuf to V1

[Vinod Kone]

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


most (if not all) comments here pertain to the unversioned executor.proto as 
well. i would recommend sending a review for unversioned proto addressing these 
comments and make this review depend on it. that way, this review would be 
purely copy paste.


include/mesos/v1/executor/executor.proto (line 42)


There is nothing in comments on 'Type' about shutdown. Just explain it here.



include/mesos/v1/executor/executor.proto (lines 89 - 90)


Much like with the scheduler, invalid calls result in BadRequest. We should 
really update this wording in executor and scheduler protos.



include/mesos/v1/executor/executor.proto (line 128)


s/acknoledged/acknowledged/



include/mesos/v1/executor/executor.proto (line 165)


This should be required ExecutorID similar to what we did with scheduler.

Also means that we need a 'Subscribe' message. I would recommend renaming 
Resubscribe to Subscribe and adding ExecutorInfo to it.


- Vinod Kone


On Sept. 9, 2015, 11:09 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38143/
> ---
> 
> (Updated Sept. 9, 2015, 11:09 p.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-3375
> https://issues.apache.org/jira/browse/MESOS-3375
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Executor protobufswere introduced in Mesos for the HTTP API, they need to be 
> added to /v1 so they reflect changes made on v1/mesos.proto. This protobuf 
> ought to be changed as the executor HTTP API design evolves.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/executor/executor.hpp PRE-CREATION 
>   include/mesos/v1/executor/executor.proto PRE-CREATION 
>   src/Makefile.am 5fdca0f 
> 
> Diff: https://reviews.apache.org/r/38143/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

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

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38233, 38234]

All tests passed.

- Mesos ReviewBot


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 37773: Docker: Adding registry client.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37871, 37427, 37773]

All tests passed.

- Mesos ReviewBot


On Sept. 9, 2015, 11:47 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 9, 2015, 11:47 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md b5dd6d8ec68d8ed7dd4787ffc9973bbe6c49965a 
>   src/Makefile.am 4ef58cd89f4a92770a7e7a8624a7befc801c6d69 
>   src/slave/containerizer/provisioners/docker/registry_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/registry_client.cpp 
> PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp 
> ff29d562c7f1bd5f0579e97cdbd60c2b2f36329e 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 37714: Updated Multimap and multihashmap so their signatures resemble that of hashmap and hashset.

[Till Toenshoff]

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



3rdparty/libprocess/3rdparty/stout/tests/multimap_tests.cpp (line 40)


Let's please comment all tests briefly on top of their head.


- Till Toenshoff


On Aug. 28, 2015, 8:11 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37714/
> ---
> 
> (Updated Aug. 28, 2015, 8:11 a.m.)
> 
> 
> Review request for mesos, Joerg Schad, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-2924
> https://issues.apache.org/jira/browse/MESOS-2924
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds extra template parameters to `multihashmap` which offer control over the 
> hash function to use as well as the equality operator.
> 
> Implements initializer_list, copy and move constructors for both, 
> `multihashmap` and `Multimap` in a similar way as it was done for `hashmap` 
> and `hashset`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp 
> d9e4031cee64e48ad50541c04ca11e7861d0a17c 
>   3rdparty/libprocess/3rdparty/stout/include/stout/multimap.hpp 
> fb3e7a1d0377001389980302342813217f49cf5f 
>   3rdparty/libprocess/3rdparty/stout/tests/multimap_tests.cpp 
> 535cd2d10e3074c86c149ce85b205e73ca42ddd3 
> 
> Diff: https://reviews.apache.org/r/37714/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 37126: Added authorization for dynamic reservation master endpoints.

[Guangya Liu]

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



src/tests/reservation_endpoints_tests.cpp (lines 1026 - 1048)


This part has been refactored to some helper functions, so a refactor is 
also needed here before merge.


- Guangya Liu


On 八月 5, 2015, 10:46 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37126/
> ---
> 
> (Updated 八月 5, 2015, 10:46 a.m.)
> 
> 
> Review request for mesos, Adam B and Jie Yu.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 76e70801925041f08bc94f0ca18c86f1a573b2b3 
>   src/master/master.hpp e44174976aa64176916827bec4c911333c9a91db 
>   src/tests/reservation_endpoints_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37126/diff/
> 
> 
> Testing
> ---
> 
> Added tests to `src/tests/reservation_endpoints_tests.cpp` + `make check`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 37773: Docker: Adding registry client.

[Jojy Varghese]

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

(Updated Sept. 10, 2015, 4:49 a.m.)


Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.


Changes
---

added more logs


Repository: mesos


Description
---

Added implementation for docker registry's Get Manifest and Get Blob APIs.


Diffs (updated)
-

  src/Makefile.am cea470e14ed93a46fead9da8015df676e818e4bc 
  src/slave/containerizer/provisioners/docker/registry_client.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/registry_client.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp 
ff29d562c7f1bd5f0579e97cdbd60c2b2f36329e 

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


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

[Jojy Varghese]

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

(Updated Sept. 10, 2015, 4:53 a.m.)


Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.


Changes
---

removed extraneous change.


Repository: mesos


Description
---

Added implementation for docker registry's Get Manifest and Get Blob APIs.


Diffs (updated)
-

  src/Makefile.am cea470e14ed93a46fead9da8015df676e818e4bc 
  src/slave/containerizer/provisioners/docker/registry_client.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/registry_client.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp 
ff29d562c7f1bd5f0579e97cdbd60c2b2f36329e 

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


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 38075: Added an Appc provisioner recovery test.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [38239]

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

Error:
 2015-09-10 05:08:37 URL:https://reviews.apache.org/r/38239/diff/raw/ 
[2833/2833] -> "38239.patch" [1]
error: patch failed: src/Makefile.am:482
error: src/Makefile.am: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Sept. 10, 2015, 12:29 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38075/
> ---
> 
> (Updated Sept. 10, 2015, 12:29 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an Appc provisioner recovery test.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/appc_provisioner_tests.cpp 
> f30e1a1dee9a68be01133117339d91771f15988c 
> 
> Diff: https://reviews.apache.org/r/38075/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> This also utilizes the copy backend.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.

[Neil Conway]

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

(Updated Sept. 10, 2015, 5:07 a.m.)


Review request for mesos.


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


Repository: mesos


Description
---

The previous coding would try to shift a uint32_t value 32 bits to the left; per
C++ spec, this yields undefined behavior. On my machine, this resulted in
treating a prefix of 0 as equivalent to a prefix of 32, which is obviously
wrong.

Spotted via ubsan: see MESOS-3328.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
1ad119d54820e97497b1773518875be25ddbf98a 
  3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
b0cbcb38cfcb923ec7c185bacf139ceb0a28924f 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.

[Neil Conway]

> On Sept. 9, 2015, 11:08 p.m., Niklas Nielsen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp, line 402
> > 
> >
> > An alternative could have been:
> > 
> > IP(prefix == 0 ? 0 : 0x << (32 - prefix))
> > 
> > Will let you decide what is easier to read.
> > I like it in terms of not writing IPNetwork twice

Good suggestion -- I wasn't entirely happy with the previous syntax, either. I 
ended up opting for being more explicit:

  // Avoid left-shifting by 32 bits when prefix is 0
  uint32_t mask = 0;
  if (prefix > 0) {
mask = 0x << (32 - prefix);
  }
  return IPNetwork(address, IP(mask));


- Neil


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


On Aug. 28, 2015, 8:02 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37903/
> ---
> 
> (Updated Aug. 28, 2015, 8:02 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3328
> https://issues.apache.org/jira/browse/MESOS-3328
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous coding would try to shift a uint32_t value 32 bits to the left; 
> per
> C++ spec, this yields undefined behavior. On my machine, this resulted in
> treating a prefix of 0 as equivalent to a prefix of 32, which is obviously
> wrong.
> 
> Spotted via ubsan: see MESOS-3328.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
> 1ad119d54820e97497b1773518875be25ddbf98a 
>   3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp 
> b0cbcb38cfcb923ec7c185bacf139ceb0a28924f 
> 
> Diff: https://reviews.apache.org/r/37903/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 38077: [5/5] Integer Precision for JSON <-> Protobuf conversions.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38028, 38030, 38031, 38076, 38077]

All tests passed.

- Mesos ReviewBot


On Sept. 9, 2015, 10:20 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38077/
> ---
> 
> (Updated Sept. 9, 2015, 10:20 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Address a TODO on one test, which used a workaround for double-precision 
> comparison.
> 
> 
> Diffs
> -
> 
>   src/tests/monitor_tests.cpp 53fb53eeea7d444ee043eb4569a52fead7ad0da8 
> 
> Diff: https://reviews.apache.org/r/38077/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36125: Removing '.json' extension in master endpoints url

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [36125]

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

Error:
 2015-09-10 02:47:43 URL:https://reviews.apache.org/r/36125/diff/raw/ 
[13754/13754] -> "36125.patch" [1]
error: patch failed: src/master/master.hpp:807
error: src/master/master.hpp: patch does not apply
error: patch failed: src/master/master.cpp:779
error: src/master/master.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Sept. 9, 2015, 7:58 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36125/
> ---
> 
> (Updated Sept. 9, 2015, 7:58 p.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, Ben Mahler, Marco 
> Massenzio, and Vinod Kone.
> 
> 
> Bugs: MESOS-2719
> https://issues.apache.org/jira/browse/MESOS-2719
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Removing json extension for HTTP endpoints in master
> 
> 
> Diffs
> -
> 
>   src/cli/mesos-cat 73dc63e 
>   src/cli/mesos-ps ee14d51 
>   src/cli/mesos-scp 77b8557 
>   src/cli/mesos-tail 256a804 
>   src/master/constants.hpp c3fe140 
>   src/master/http.cpp 37d76ee 
>   src/master/master.hpp 0432842 
>   src/master/master.cpp 95207d2 
>   src/tests/fault_tolerance_tests.cpp 89cb18b 
>   src/tests/master_tests.cpp 8a6b98b 
>   src/webui/master/static/js/controllers.js 3445028 
> 
> Diff: https://reviews.apache.org/r/36125/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 37176: Maintenance Primitives: Added a new allocation overload to sorter.

[Benjamin Hindman]

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

Ship it!



src/master/allocator/sorter/drf/sorter.cpp (lines 173 - 177)


A suggestion to keep 'allocation' const& and avoid the double lookup:

foreachpair (const string& name, const Allocation& allocation, allocations) 
{
  Option resources = allocation.resources.get(slaveId);
  if (resources.isSome()) {
result.emplace(name, resources.get());
  }
}


- Benjamin Hindman


On Sept. 2, 2015, 7:31 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37176/
> ---
> 
> (Updated Sept. 2, 2015, 7:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 217c7c434874b3870668c69799d6b59ce1d83973 
>   src/master/allocator/sorter/drf/sorter.cpp 
> bfc273493419fe46a4d907f4f7fa282cff71b800 
>   src/master/allocator/sorter/sorter.hpp 
> 536a7ad9a2d661bc8aa352d2e0ae41115b1e8a04 
> 
> Diff: https://reviews.apache.org/r/37176/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 37002: Introduced ACL protobuf definitions for dynamic reservation.

[Guangya Liu]

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



include/mesos/mesos.proto (line 1120)


Yes, here should be reserve resource.

Another is that the ACL is now moved to v1 API, you may need  a rebase here.


- Guangya Liu


On 八月 5, 2015, 9:57 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37002/
> ---
> 
> (Updated 八月 5, 2015, 9:57 a.m.)
> 
> 
> Review request for mesos, Adam B and Jie Yu.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a6748d1cd82238f005c6a49c70d22d095462f1ba 
> 
> Diff: https://reviews.apache.org/r/37002/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 37177: Maintenance Primitives: Added inverse offers.

[Benjamin Hindman]

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

Ship it!



include/mesos/maintenance/maintenance.hpp (line 31)


s/Master/Allocator/



src/master/allocator/mesos/hierarchical.hpp (line 1119)


Can we use NOTE: here like in the other review? Thanks!



src/master/allocator/mesos/hierarchical.hpp (line 1155)


A similar suggestion to a previous review:

Option maintenanceStatus = 
slaves[slaveId].maintenanceStatus;

if (maintenanceStatus.isSome()) {
  ...;
  if (!maintenanceStatus->statuses.contains(frameworkId) || ...) {
...;
  }
  ...;
}

Or '!maintenanceStatus.get().statuses.contains(frameworkId)' if you prefer 
the '.get()' variant (ahhh! this is what I was worried about!).



src/tests/hierarchical_allocator_tests.cpp (lines 195 - 196)


Can we please s/offerQueue/allocations/ s/inverseOfferQueue/deallocations/ 
thanks!


- Benjamin Hindman


On Sept. 2, 2015, 7:32 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37177/
> ---
> 
> (Updated Sept. 2, 2015, 7:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/maintenance/maintenance.hpp 
> 7fec3ffe063e766dcec4952001fa5c6e20d9eec8 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
>   src/master/allocator/mesos/allocator.hpp 
> aa55755a9c3250579e9366bdbc17a2449e95d659 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
>   src/master/master.hpp 594dd25f9aa9b6147680d0a838a77c3222941f4b 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
>   src/tests/hierarchical_allocator_tests.cpp 
> 9748ca0b3fee25dcec51c64d8ba84dbd4aaf 
>   src/tests/master_allocator_tests.cpp 
> 89331965553505f6b7eebf39ad27d943df816a24 
>   src/tests/mesos.hpp 906948d459b5a88a4ad7952801eb8c540b58c569 
>   src/tests/reservation_tests.cpp aeee36752573e3f401d3dca7d2d69c90d0e8bd6b 
>   src/tests/resource_offers_tests.cpp 
> 882a9ff4d09aace486182828bf43b643b0d0c519 
>   src/tests/slave_recovery_tests.cpp 4d137e0f1278fdacf71f101b1967df35bfbcdd23 
> 
> Diff: https://reviews.apache.org/r/37177/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 37125: Added 'Master::authorize' for Reserve/Unreserve.

[Guangya Liu]

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


In the description of this patch, you mentioned that "These are used for 
authorization of frameworks as well as master endpoints.", not clear what does 
this mean, does it mean that the API in this patch can be also used to 
authorize framework? Can you please show more comments here? Thanks.


src/master/master.cpp (line 2354)


What about s/authorize/authorizeReserveResource



src/master/master.cpp (line 2378)


What about s/authorize/authorizeUnReserveResource


- Guangya Liu


On Aug. 5, 2015, 10 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37125/
> ---
> 
> (Updated Aug. 5, 2015, 10 a.m.)
> 
> 
> Review request for mesos, Adam B and Jie Yu.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `Master::authorize` function is overloaded for `Reserve` and `Unreserve`. 
> This will be extended for `Create` and `Destroy` in the future. These are 
> used for authorization of frameworks as well as master endpoints.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e44174976aa64176916827bec4c911333c9a91db 
>   src/master/master.cpp 5aa0a5410804fe16abd50b6953f1ffe46a019ecf 
> 
> Diff: https://reviews.apache.org/r/37125/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 37505: Fix broken health check in docker executor.

[haosdent huang]

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

(Updated Sept. 10, 2015, 2:20 a.m.)


Review request for mesos and Timothy Chen.


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


Repository: mesos


Description
---

Fix broken health check in docker executor.


Diffs (updated)
-

  src/docker/docker.hpp aaf8884a20901ad3a440d545d4b6316b87fc3403 
  src/docker/docker.cpp 553e831029454d6d423842915b8bbfcaf19fa7f9 
  src/docker/executor.cpp 6647075b55e5a79264e3556bb906a1f26a2d673e 
  src/slave/containerizer/docker.cpp 289d4ec0fba9071dfe0cbf5391b5391d4566dd9c 
  src/tests/health_check_tests.cpp 157a56aa06677d8b7a2cef53b29ed05cb4b5d8ea 

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


Testing
---

# Add two new test cases, HealthCheckTest.ROOT_DOCKER_DockerHealthyTask and 
HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange
# Docker health check command is run through "docker exec"
sudo ./bin/mesos-tests.sh 
--gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthyTask" --verbose
sudo ./bin/mesos-tests.sh 
--gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange" --verbose


Thanks,

haosdent huang

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Till Toenshoff]

> On Sept. 10, 2015, 2:48 a.m., Till Toenshoff wrote:
> > Looks really good to me, thanks Joerg.

Just noticed the tests fail at a point that seems relevant to this code - 
please make sure you can recreate the buildbot failure and then fix it please :)


- Till


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


On Sept. 9, 2015, 8:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 9, 2015, 8:48 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Till Toenshoff]

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

Ship it!


Looks really good to me, thanks Joerg.


src/common/values.cpp (line 125)


I like this explicit way of calculating the size via a scoped variable.



src/common/values.cpp (lines 149 - 150)


:D



src/common/values.cpp (line 162)


Why this blank line?



src/common/values.cpp (line 179)


US english would be s/neighbouring/neighboring/



src/common/values.cpp (lines 210 - 211)


I think this would look less "noisy":
```
void addRange(
Value::Ranges* ranges, uint64_t begin, uint64_t end, int insertPosition)
```



src/tests/values_tests.cpp (line 37)


Fits a single line:
```
  extern void coalesce(Value::Ranges* ranges, const Value::Range& range);
```



src/tests/values_tests.cpp (line 39)


Fits a single line:
```
  extern void removeRanges(Value::Ranges* ranges, std::set* deleteSet);
```



src/tests/values_tests.cpp (lines 41 - 42)


How about this instead?
```
  extern void addRange(
  Value::Ranges* ranges, uint64_t begin, uint64_t end, int 
insertPosition);
```



src/tests/values_tests.cpp (line 48)


Not yours but IIUC then this is a blank line too many.



src/tests/values_tests.cpp (line 218)


Insert blank please.



src/tests/values_tests.cpp (line 253)


s/Overlap/overlap/



src/tests/values_tests.cpp (line 305)


s/overlap/overlaps/ ?



src/tests/values_tests.cpp (line 319)


Not sure if we should aim for commonwealth or us english on the 
neighbour/neighbor? :)



src/tests/values_tests.cpp (line 326)


s/N/n/



src/tests/values_tests.cpp (line 333)


s/collescing/coalescing/



src/tests/values_tests.cpp (line 340)


see above



src/tests/values_tests.cpp (line 376)


Hah, now you chose the US-english "neighbor" :D



src/tests/values_tests.cpp (line 390)


Quick descriptive comment please.



src/tests/values_tests.cpp (line 430)


Quick descriptive comment please.


- Till Toenshoff


On Sept. 9, 2015, 8:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 9, 2015, 8:48 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 37110: Enabled the Authorizer to handle Reserve/Unreserve ACLs.

[Guangya Liu]

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



src/tests/authorization_tests.cpp (line 397)


// "bar" principal cannot unreserve anyone's resources.


Also you may want a rebase as the main logic is now moved to 
src/authorizer/local/authorizer.cpp

- Guangya Liu


On 八月 5, 2015, 9:58 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37110/
> ---
> 
> (Updated 八月 5, 2015, 9:58 a.m.)
> 
> 
> Review request for mesos, Adam B and Jie Yu.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/authorizer/authorizer.hpp c039d9412780aa199db169b31991bf9f45b07d0f 
>   src/authorizer/authorizer.cpp 21e97e315478a4ca9442af83732665f85eb2f8fc 
>   src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87 
>   src/tests/mesos.hpp 20418d4fbd2f4ae35ee0c707472cbf37125883b0 
> 
> Diff: https://reviews.apache.org/r/37110/diff/
> 
> 
> Testing
> ---
> 
> Added tests to `src/tests/authorization_tests.cpp` + `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 36126: Removing '.json' extension in slave endpoints url

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36126]

All tests passed.

- Mesos ReviewBot


On Sept. 9, 2015, 7:58 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36126/
> ---
> 
> (Updated Sept. 9, 2015, 7:58 p.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, Ben Mahler, Marco 
> Massenzio, and Vinod Kone.
> 
> 
> Bugs: MESOS-2983
> https://issues.apache.org/jira/browse/MESOS-2983
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Removing json extension for HTTP endpoints in slave
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2b23d48 
>   docs/network-monitoring.md acd70c5 
>   src/cli/mesos-cat 73dc63e 
>   src/cli/mesos-ps ee14d51 
>   src/cli/mesos-tail 256a804 
>   src/master/flags.cpp 230c1dc 
>   src/slave/flags.cpp b36710d 
>   src/slave/http.cpp b0fe5f5 
>   src/slave/monitor.cpp 82aa659 
>   src/slave/slave.hpp 09172f7 
>   src/slave/slave.cpp 2a99abc 
>   src/tests/fault_tolerance_tests.cpp 89cb18b 
>   src/tests/monitor_tests.cpp 53fb53e 
>   src/tests/slave_tests.cpp d55e9dd 
>   src/webui/master/static/js/controllers.js 3445028 
>   src/webui/master/static/js/services.js 2cd9d7d 
> 
> Diff: https://reviews.apache.org/r/36126/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38239: Renamed appc.{hpp|cpp} to slave/containerizer/provisioners/appc/provisioner.{hpp|cpp}.

[Jiang Yan Xu]

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

(Updated Sept. 9, 2015, 10:35 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Comments.


Summary (updated)
-

Renamed appc.{hpp|cpp} to 
slave/containerizer/provisioners/appc/provisioner.{hpp|cpp}.


Repository: mesos


Description (updated)
---

Renamed appc.{hpp|cpp} to 
slave/containerizer/provisioners/appc/provisioner.{hpp|cpp}.


Diffs (updated)
-

  src/Makefile.am 0a8ef6d8551cf177cb565b2a443c05e8eea5ab1c 
  src/slave/containerizer/provisioner.cpp 
95894c0ed97388cf2830058c13804b6baf5daad4 
  src/slave/containerizer/provisioners/appc.hpp  
  src/slave/containerizer/provisioners/appc.cpp 
d54b6882346df7121ec63803810fab979ac0848a 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 37427: Docker registry: adding TokenManager.

[Jojy Varghese]

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

(Updated Sept. 9, 2015, 4:50 p.m.)


Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.


Changes
---

rebased with master.


Repository: mesos


Description
---

Changes:
  - Added Token implementation (RFC 7519).
  - Added TokenManager implementation. This component keeps a cache of tokens
  requested for any future requests.


Diffs (updated)
-

  src/Makefile.am 0a8ef6d8551cf177cb565b2a443c05e8eea5ab1c 
  src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37773: Docker: Adding registry client.

[Jojy Varghese]

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

(Updated Sept. 9, 2015, 4:50 p.m.)


Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.


Changes
---

rebased with master.


Repository: mesos


Description
---

Added implementation for docker registry's Get Manifest and Get Blob APIs.


Diffs (updated)
-

  src/Makefile.am 0a8ef6d8551cf177cb565b2a443c05e8eea5ab1c 
  src/slave/containerizer/provisioners/docker/registry_client.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/registry_client.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 37878: mesos: Replace volatile with std::atomic.

[Joris Van Remoortere]

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

Ship it!


- Joris Van Remoortere


On Sept. 9, 2015, 4:01 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37878/
> ---
> 
> (Updated Sept. 9, 2015, 4:01 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and switched to 'mcypark'.
> 
> 
> Bugs: MESOS-3326
> https://issues.apache.org/jira/browse/MESOS-3326
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3326.
> 
> 
> Diffs
> -
> 
>   src/exec/exec.cpp 31e0c2f17a9092d18285828111d27628fb07bc02 
>   src/sched/sched.cpp 012af0508eeceeccd168b29f36fa258d20b28c21 
> 
> Diff: https://reviews.apache.org/r/37878/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 37877: libprocess: Replace GCC instrinsics and volatile with std::atomic.

[Joris Van Remoortere]

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

Ship it!



3rdparty/libprocess/include/process/metrics/counter.hpp (line 78)


Since we're touching this, can we use a more meaningful variable name? We 
use full words for variable names in Mesos. (Not yours, I know ;-))

Also pointing out the explicit specialization mentioned in a prior review.



3rdparty/libprocess/include/process/process.hpp (line 335)


Is this a place where we want to change the code to use a more suitable 
type? My thoughts are "If we're reference counting, we should be unsigned" and 
"If we're on a 64-bit architecture we might as well use a long to avoid chances 
of overflowing the ref count".

Thoughts?

We could do this in a subsequent review to separate the refactorings.



3rdparty/libprocess/src/latch.cpp (lines 41 - 44)


Please use full words for variable names.

Similar below.



3rdparty/libprocess/src/process.cpp (line 430)


Another one where I'm curious if it would be worth increasing the range 
(and possibly only expressing valid values, >=0) for safety.



3rdparty/libprocess/src/process.cpp (lines 757 - 773)


Would you agree that what is happening here is not immediately obvious?

Now that you understand it, would you mind adding a few comments around the 
different exit conditions and which stage of intitialization they represent?

In particular I think the exit condition around 769 could use a comment.



3rdparty/libprocess/src/process.cpp (line 767)


Can you wrap compare_exchange_strong in backticks ```



3rdparty/libprocess/src/process.cpp (line 768)


Full variable names.



3rdparty/libprocess/src/process.cpp (line 953)


let's use backticks ```



3rdparty/libprocess/src/process.cpp (line 1023)


nice catch.



3rdparty/libprocess/src/process.cpp (line 2826)


nice catch.


- Joris Van Remoortere


On Sept. 9, 2015, 4:01 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37877/
> ---
> 
> (Updated Sept. 9, 2015, 4:01 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and switched to 'mcypark'.
> 
> 
> Bugs: MESOS-3326
> https://issues.apache.org/jira/browse/MESOS-3326
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3326.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/latch.hpp 
> a1a2227a9edcc31fd82c6410262aa4565fd66cb2 
>   3rdparty/libprocess/include/process/metrics/counter.hpp 
> e51a8beb80b15dd64aa2e481036ae8ba37125640 
>   3rdparty/libprocess/include/process/process.hpp 
> 009f7c4167fa379ac6b1c267e1b4d5fcdf28d697 
>   3rdparty/libprocess/src/clock.cpp 09c60e5a5d9bd9fc5511e57f3209fad7dbf834d6 
>   3rdparty/libprocess/src/latch.cpp f7d94d92e85c58878d98e13757b6fc37837ca977 
>   3rdparty/libprocess/src/process.cpp 
> 755187c8761137cb2bf2f7295b29a63f63c68bc6 
>   3rdparty/libprocess/src/process_reference.hpp 
> f8df4a6dcf01bb7af750c1ed9e85c64cea2042c5 
> 
> Diff: https://reviews.apache.org/r/37877/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 37876: stout: Replace GCC intrinsics with std::atomic.

[Joris Van Remoortere]

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

Ship it!


Thanks for cleaning this all up Neil!

Let's add some style-guide info or reference the google style-guide if your 
changes are already covered.
A few concise examples would be great.


3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/fork.hpp (line 278)


Some places in this review chain you use the provided typedefs 
`std::atomic_XXX`, whereas elsewhere you provide the explicit specialization 
`std::atomic`.

Can you put a review at the front of this chain to provide guidance for 
consistency in the style guide regarding atomics? I would add examples and 
policies for:
1. Always using the explicit specializations; or when to use the typedef 
over the explicit (if you have a good argument for that)
2. Why we use the explicit functions such as `store(X)` as opposed to the 
`operator=` as we discussed in person.

Once that's done, please make any changes required in the chain to stay 
consistent.


- Joris Van Remoortere


On Sept. 9, 2015, 4:02 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37876/
> ---
> 
> (Updated Sept. 9, 2015, 4:02 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and switched to 'mcypark'.
> 
> 
> Bugs: MESOS-3326
> https://issues.apache.org/jira/browse/MESOS-3326
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3326.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/fork.hpp 
> d43433aeab5a1a68ff76eb75416672fae456c70d 
> 
> Diff: https://reviews.apache.org/r/37876/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 38191: Removing unused Executor protobuf

[Anand Mazumdar]

> On Sept. 9, 2015, 5:33 p.m., Vinod Kone wrote:
> > Why is this being removed? The plan is to have a v1/executor.proto and an 
> > unversioned executor.proto much like what we did with scheduler.proto.
> 
> Anand Mazumdar wrote:
> Vinod, Why can't the executor driver just directly use the V1 protobuf ? 
> The unversioned protobuf for scheduler was persisted since we had shipped the 
> scheduler driver with 0.23 to use the unversioned Call/Events or am I missing 
> something ?
> 
> PS: I asked Isabel to remove it in CR 38143

Oops, I see what you mean now. You were referring to using this as an internal 
protobuf for now i.e. devolve(...) - V1->internal. We should shelve this review 
then, my bad.


- Anand


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


On Sept. 8, 2015, 10:30 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38191/
> ---
> 
> (Updated Sept. 8, 2015, 10:30 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-3393
> https://issues.apache.org/jira/browse/MESOS-3393
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The executor protobuf definition living outside the v1/ directory is unused, 
> it should be removed to avoid confusion.
> 
> 
> Diffs
> -
> 
>   include/mesos/executor/executor.hpp 85f181c 
>   include/mesos/executor/executor.proto 52c84b3 
>   src/Makefile.am 5fdca0f 
> 
> Diff: https://reviews.apache.org/r/38191/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Review Request 38231: Fixed typos in modules documentation.

[Greg Mann]

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

Review request for mesos and Niklas Nielsen.


Repository: mesos


Description
---

Fixed typos in modules documentation.


Diffs
-

  docs/modules.md 12ee839702e9ad00a0d35ebb89870d05de4b8764 

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


Testing
---


Thanks,

Greg Mann

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 9, 2015, 8:48 p.m.)


Review request for mesos, Bernd Mathiske and Till Toenshoff.


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


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 37714: Updated Multimap and multihashmap so their signatures resemble that of hashmap and hashset.

[haosdent huang]

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

Ship it!


Ship It!

- haosdent huang


On Aug. 28, 2015, 8:11 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37714/
> ---
> 
> (Updated Aug. 28, 2015, 8:11 a.m.)
> 
> 
> Review request for mesos, Joerg Schad, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-2924
> https://issues.apache.org/jira/browse/MESOS-2924
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds extra template parameters to `multihashmap` which offer control over the 
> hash function to use as well as the equality operator.
> 
> Implements initializer_list, copy and move constructors for both, 
> `multihashmap` and `Multimap` in a similar way as it was done for `hashmap` 
> and `hashset`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp 
> d9e4031cee64e48ad50541c04ca11e7861d0a17c 
>   3rdparty/libprocess/3rdparty/stout/include/stout/multimap.hpp 
> fb3e7a1d0377001389980302342813217f49cf5f 
>   3rdparty/libprocess/3rdparty/stout/tests/multimap_tests.cpp 
> 535cd2d10e3074c86c149ce85b205e73ca42ddd3 
> 
> Diff: https://reviews.apache.org/r/37714/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38154: Switched to type traits for checking whether a type is a protobuf message.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37826, 37827, 37830, 38154]

All tests passed.

- Mesos ReviewBot


On Sept. 9, 2015, 2:45 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38154/
> ---
> 
> (Updated Sept. 9, 2015, 2:45 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 57d5fdf45273c620655b44b5f5572290cffa4bf6 
> 
> Diff: https://reviews.apache.org/r/38154/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37877: libprocess: Replace GCC instrinsics and volatile with std::atomic.

[Neil Conway]

> On Sept. 9, 2015, 5:16 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 757-773
> > 
> >
> > Would you agree that what is happening here is not immediately obvious?
> > 
> > Now that you understand it, would you mind adding a few comments around 
> > the different exit conditions and which stage of intitialization they 
> > represent?
> > 
> > In particular I think the exit condition around 769 could use a comment.

"Not immediately obvious" is an understatement :)

I can certainly add comments, although I wonder whether this logic can't be 
rethought and simplified; if we need to keep the existing logic, seems we 
should point out the assignment to "initializing" down below.

I'll take a look at this, but can we do it as a separate review?


- Neil


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


On Sept. 9, 2015, 4:01 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37877/
> ---
> 
> (Updated Sept. 9, 2015, 4:01 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and switched to 'mcypark'.
> 
> 
> Bugs: MESOS-3326
> https://issues.apache.org/jira/browse/MESOS-3326
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3326.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/latch.hpp 
> a1a2227a9edcc31fd82c6410262aa4565fd66cb2 
>   3rdparty/libprocess/include/process/metrics/counter.hpp 
> e51a8beb80b15dd64aa2e481036ae8ba37125640 
>   3rdparty/libprocess/include/process/process.hpp 
> 009f7c4167fa379ac6b1c267e1b4d5fcdf28d697 
>   3rdparty/libprocess/src/clock.cpp 09c60e5a5d9bd9fc5511e57f3209fad7dbf834d6 
>   3rdparty/libprocess/src/latch.cpp f7d94d92e85c58878d98e13757b6fc37837ca977 
>   3rdparty/libprocess/src/process.cpp 
> 755187c8761137cb2bf2f7295b29a63f63c68bc6 
>   3rdparty/libprocess/src/process_reference.hpp 
> f8df4a6dcf01bb7af750c1ed9e85c64cea2042c5 
> 
> Diff: https://reviews.apache.org/r/37877/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 38191: Removing unused Executor protobuf

[Anand Mazumdar]

> On Sept. 9, 2015, 5:33 p.m., Vinod Kone wrote:
> > Why is this being removed? The plan is to have a v1/executor.proto and an 
> > unversioned executor.proto much like what we did with scheduler.proto.

Vinod, Why can't the executor driver just directly use the V1 protobuf ? The 
unversioned protobuf for scheduler was persisted since we had shipped the 
scheduler driver with 0.23 to use the unversioned Call/Events or am I missing 
something ?

PS: I asked Isabel to remove it in CR 38143


- Anand


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


On Sept. 8, 2015, 10:30 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38191/
> ---
> 
> (Updated Sept. 8, 2015, 10:30 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-3393
> https://issues.apache.org/jira/browse/MESOS-3393
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The executor protobuf definition living outside the v1/ directory is unused, 
> it should be removed to avoid confusion.
> 
> 
> Diffs
> -
> 
>   include/mesos/executor/executor.hpp 85f181c 
>   include/mesos/executor/executor.proto 52c84b3 
>   src/Makefile.am 5fdca0f 
> 
> Diff: https://reviews.apache.org/r/38191/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

[Timothy Chen]

> On Sept. 9, 2015, 4:57 p.m., Jiang Yan Xu wrote:
> > Could you take out the paths.hpp|cpp changes that have already been 
> > committed?

done


- Timothy


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


On Sept. 9, 2015, 5:03 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 9, 2015, 5:03 p.m.)
> 
> 
> Review request for mesos, Jojy Varghese, Till Toenshoff, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Joining all the commits around provisioner and local store into one review so 
> it's easier to review, as patches
> are changing code on top of each other.
> 
> All the commits are going to committed together.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 0a8ef6d 
>   src/messages/docker_provisioner.hpp PRE-CREATION 
>   src/messages/docker_provisioner.proto PRE-CREATION 
>   src/slave/containerizer/provisioner.hpp 9e0e0b8 
>   src/slave/containerizer/provisioner.cpp 95894c0 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/local_store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/local_store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/flags.hpp b8335aa 
>   src/slave/flags.cpp 7539441 
>   src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

[Timothy Chen]

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

(Updated Sept. 9, 2015, 5:03 p.m.)


Review request for mesos, Jojy Varghese, Till Toenshoff, and Jiang Yan Xu.


Repository: mesos


Description
---

Joining all the commits around provisioner and local store into one review so 
it's easier to review, as patches
are changing code on top of each other.

All the commits are going to committed together.


Diffs (updated)
-

  src/Makefile.am 0a8ef6d 
  src/messages/docker_provisioner.hpp PRE-CREATION 
  src/messages/docker_provisioner.proto PRE-CREATION 
  src/slave/containerizer/provisioner.hpp 9e0e0b8 
  src/slave/containerizer/provisioner.cpp 95894c0 
  src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/local_store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/local_store.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/metadata_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/metadata_manager.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/flags.hpp b8335aa 
  src/slave/flags.cpp 7539441 
  src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 38149: Add message_call_received metrics for master

[Vinod Kone]

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


What's the use case for this metric? For old style messages, we had per 
framework "messages_received" because it was used for rate limiting.

- Vinod Kone


On Sept. 9, 2015, 1:42 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38149/
> ---
> 
> (Updated Sept. 9, 2015, 1:42 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add message_call_received metrics for master
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac 
>   src/master/metrics.hpp 2d07a16f2dc6811973c82259c3cccff07401b542 
>   src/master/metrics.cpp d79206f08181c99641bef697be57b5d57b627285 
>   src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1 
>   src/tests/metrics_tests.cpp 3e9d7c2eadfcdd2522d65e4a55dc76808ba39159 
> 
> Diff: https://reviews.apache.org/r/38149/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37773: Docker: Adding registry client.

[Timothy Chen]

> On Sept. 1, 2015, 9:35 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 359
> > 
> >
> > Should we make sure somewhere that we encode or check the tag so that 
> > we don't contain spaces?
> 
> Jojy Varghese wrote:
> I am not sure if http path can contain spaces. Queries can.

Therefore we shouldn't allow it right?


- Timothy


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


On Sept. 9, 2015, 4:50 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> ---
> 
> (Updated Sept. 9, 2015, 4:50 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 0a8ef6d8551cf177cb565b2a443c05e8eea5ab1c 
>   src/slave/containerizer/provisioners/docker/registry_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/registry_client.cpp 
> PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

[Jiang Yan Xu]

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


Could you take out the paths.hpp|cpp changes that have already been committed?

- Jiang Yan Xu


On Sept. 8, 2015, 12:52 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 8, 2015, 12:52 p.m.)
> 
> 
> Review request for mesos, Jojy Varghese, Till Toenshoff, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Joining all the commits around provisioner and local store into one review so 
> it's easier to review, as patches
> are changing code on top of each other.
> 
> All the commits are going to committed together.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5fdca0f 
>   src/messages/docker_provisioner.hpp PRE-CREATION 
>   src/messages/docker_provisioner.proto PRE-CREATION 
>   src/slave/containerizer/provisioner.hpp 9e0e0b8 
>   src/slave/containerizer/provisioner.cpp 95894c0 
>   src/slave/containerizer/provisioners/appc.cpp fc5ee19 
>   src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7 
>   src/slave/containerizer/provisioners/appc/paths.cpp e6be851 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/local_store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/local_store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/paths.cpp PRE-CREATION 
>   src/slave/flags.hpp b8335aa 
>   src/slave/flags.cpp 7539441 
>   src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 37876: stout: Replace GCC intrinsics with std::atomic.

[Neil Conway]

> On Sept. 9, 2015, 5:16 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/fork.hpp, line 278
> > 
> >
> > Some places in this review chain you use the provided typedefs 
> > `std::atomic_XXX`, whereas elsewhere you provide the explicit 
> > specialization `std::atomic`.
> > 
> > Can you put a review at the front of this chain to provide guidance for 
> > consistency in the style guide regarding atomics? I would add examples and 
> > policies for:
> > 1. Always using the explicit specializations; or when to use the 
> > typedef over the explicit (if you have a good argument for that)
> > 2. Why we use the explicit functions such as `store(X)` as opposed to 
> > the `operator=` as we discussed in person.
> > 
> > Once that's done, please make any changes required in the chain to stay 
> > consistent.

Thanks for the review, Joris!

This is a great point -- I'll update the style guide. As far as when to use the 
explicit specialization over the typedef, I only used an explicit 
specialization when C++11 doesn't provide a typedef. e.g., C++ doesn't provide 
std::atomic_int64_t, so I used std::atomic (the standard provides 
std::atomic_fast_int64_t, which seems a bit painful to type/read). I don't have 
strong feelings here, though: for example, you could argue that we should 
always use the explicit specializations for the sake of consistency.


- Neil


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


On Sept. 9, 2015, 4:02 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37876/
> ---
> 
> (Updated Sept. 9, 2015, 4:02 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and switched to 'mcypark'.
> 
> 
> Bugs: MESOS-3326
> https://issues.apache.org/jira/browse/MESOS-3326
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3326.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/fork.hpp 
> d43433aeab5a1a68ff76eb75416672fae456c70d 
> 
> Diff: https://reviews.apache.org/r/37876/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 38191: Removing unused Executor protobuf

[Vinod Kone]

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


Why is this being removed? The plan is to have a v1/executor.proto and an 
unversioned executor.proto much like what we did with scheduler.proto.

- Vinod Kone


On Sept. 8, 2015, 10:30 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38191/
> ---
> 
> (Updated Sept. 8, 2015, 10:30 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-3393
> https://issues.apache.org/jira/browse/MESOS-3393
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The executor protobuf definition living outside the v1/ directory is unused, 
> it should be removed to avoid confusion.
> 
> 
> Diffs
> -
> 
>   include/mesos/executor/executor.hpp 85f181c 
>   include/mesos/executor/executor.proto 52c84b3 
>   src/Makefile.am 5fdca0f 
> 
> Diff: https://reviews.apache.org/r/38191/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38191: Removing unused Executor protobuf

[Isabel Jimenez]

> On Sept. 9, 2015, 5:33 p.m., Vinod Kone wrote:
> > Why is this being removed? The plan is to have a v1/executor.proto and an 
> > unversioned executor.proto much like what we did with scheduler.proto.
> 
> Anand Mazumdar wrote:
> Vinod, Why can't the executor driver just directly use the V1 protobuf ? 
> The unversioned protobuf for scheduler was persisted since we had shipped the 
> scheduler driver with 0.23 to use the unversioned Call/Events or am I missing 
> something ?
> 
> PS: I asked Isabel to remove it in CR 38143
> 
> Anand Mazumdar wrote:
> Oops, I see what you mean now. You were referring to using this as an 
> internal protobuf for now i.e. devolve(...) - V1->internal. We should shelve 
> this review then, my bad.

Discarding it.


- Isabel


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


On Sept. 8, 2015, 10:30 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38191/
> ---
> 
> (Updated Sept. 8, 2015, 10:30 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-3393
> https://issues.apache.org/jira/browse/MESOS-3393
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The executor protobuf definition living outside the v1/ directory is unused, 
> it should be removed to avoid confusion.
> 
> 
> Diffs
> -
> 
>   include/mesos/executor/executor.hpp 85f181c 
>   include/mesos/executor/executor.proto 52c84b3 
>   src/Makefile.am 5fdca0f 
> 
> Diff: https://reviews.apache.org/r/38191/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 37773: Docker: Adding registry client.

[Jojy Varghese]

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

(Updated Sept. 9, 2015, 8:08 p.m.)


Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.


Changes
---

rebased with master. addressed review comments.


Repository: mesos


Description
---

Added implementation for docker registry's Get Manifest and Get Blob APIs.


Diffs (updated)
-

  src/Makefile.am 4ef58cd89f4a92770a7e7a8624a7befc801c6d69 
  src/slave/containerizer/provisioners/docker/registry_client.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/registry_client.cpp PRE-CREATION 
  src/tests/provisioners/docker_provisioner_tests.cpp 
ff29d562c7f1bd5f0579e97cdbd60c2b2f36329e 

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


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 38046: Replace hard-coded reap interval with a constant

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38046]

All tests passed.

- Mesos ReviewBot


On Sept. 9, 2015, 4:09 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38046/
> ---
> 
> (Updated Sept. 9, 2015, 4:09 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-1935
> https://issues.apache.org/jira/browse/MESOS-1935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace hard-coded reap interval with a constant
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/launch_tests.cpp 
> d211fc0f665988068c67836ef80916828a0df2bd 
>   src/tests/slave_recovery_tests.cpp 6aae14a3c39b9aee76147b691b0170946e1120b5 
>   src/tests/slave_tests.cpp 5c1a3d36a5f67629aef275eeae12956c524e8102 
> 
> Diff: https://reviews.apache.org/r/38046/diff/
> 
> 
> Testing
> ---
> 
> make distcheck
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>