Re: Review Request 41187: Adding labels field to Port information, for service discovery to associate arbitrary tags by applications to ports for a given task.

[Adam B]

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


Thanks! A few wording suggestions, and a clarification of 'visibility' scoping, 
but otherwise shippable.


include/mesos/mesos.proto (line 1564)


"Port number on which the framework exposes a service."



include/mesos/mesos.proto (line 1566)


"Name of the service hosted on this port."



include/mesos/mesos.proto (line 1568)


s/frameworks/framework/

And technically, you've got a dangling preposition. Grammar nazis would 
demand "protocol on which the framework exposes its services".



include/mesos/mesos.proto (lines 1570 - 1571)


The visibility setting for a Port overrides the general visibility setting 
in the DiscoveryInfo.



include/mesos/mesos.proto (line 1573)


s/tags that could be/metadata to be/ since you shouldn't decorate the 
message unless you have a consumer in mind, plus 'metadata' is more "meta".
Also, you probably don't need to quote "decorate".


- Adam B


On Dec. 16, 2015, 7:07 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41187/
> ---
> 
> (Updated Dec. 16, 2015, 7:07 p.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adding labels field to Port information, for service discovery to associate 
> arbitrary tags by applications to ports for a given task.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8ca213062c480f0266ffc51a621eb4a118140c77 
>   include/mesos/v1/mesos.proto 8f357b0fb778098ec66ac85d174bdd7e387954b5 
> 
> Diff: https://reviews.apache.org/r/41187/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 41380: Added repeated vip field to DiscoveryInfo and an instance_port field to Port

[Kevin Klues]

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


This review depends on 41189, but 41189 is somehow marked as "Invite-only", so 
it is not viewable by the public.  This is causing the jenkins review-bot to 
fail with:

Verifying review 41381
Dependent review: https://reviews.apache.org/api/review-requests/41380/
Dependent review: https://reviews.apache.org/api/review-requests/41189/
Error handling URL https://reviews.apache.org/api/review-requests/41189/: 
FORBIDDEN

>From my reading of 
>https://www.reviewboard.org/docs/manual/2.5/admin/configuration/access-control/
My guess is all you need to do is make sure that 41189 has the 'mesos' group 
added to it. If no group is there, it will likely be treated as "Invite-only" 
since no public groups are accessible.

- Kevin Klues


On Dec. 17, 2015, 3:31 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41380/
> ---
> 
> (Updated Dec. 17, 2015, 3:31 a.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-4114
> https://issues.apache.org/jira/browse/MESOS-4114
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added repeated vip field to DiscoveryInfo and an instance_port field to Port
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8ca213062c480f0266ffc51a621eb4a118140c77 
>   include/mesos/v1/mesos.proto 8f357b0fb778098ec66ac85d174bdd7e387954b5 
> 
> Diff: https://reviews.apache.org/r/41380/diff/
> 
> 
> Testing
> ---
> 
> make check, and make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 40431: Move RoleInfo message out of allocator.proto

[Adam B]

> On Dec. 10, 2015, 2:40 a.m., Adam B wrote:
> > include/mesos/master/allocator.proto, line 19
> > 
> >
> > Shouldn't this file have `java_package` and `java_outer_classname` just 
> > like the other protos?
> > Looks like isolator.proto and oversubscription.proto are missing it 
> > too. Would you mind creating a separate patch to fix that?
> 
> Yongqiao Wang wrote:
> I am not sure if we need to add java_package and java_outer_classname in 
> those proto files, can you please clarify a little more about why we need to 
> do this?
> 
> Adam B wrote:
> Nevermind. That's only necessary for the scheduler/executor API 
> protobufs, since they may need to be consumed by Java processes.
> https://developers.google.com/protocol-buffers/docs/javatutorial
> Dropping the issue.
> 
> Yongqiao Wang wrote:
> So can I log another JIRA ticket to fix this issue?

Not necessary. RoleInfo, isolator.proto, and oversubscription.proto are not a 
part of the scheduler/executor APIs, which are the only Mesos APIs that have 
Java bindings and would want/need to generate Java classes from the protos. So, 
since these protos are only used by C++, they don't need the java options. 
Doesn't hurt to add them, but they won't be used.


- Adam


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


On Dec. 7, 2015, 9:20 p.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40431/
> ---
> 
> (Updated Dec. 7, 2015, 9:20 p.m.)
> 
> 
> Review request for mesos, Adam B, Guangya Liu, Qian Zhang, and Jian Qiu.
> 
> 
> Bugs: MESOS-3944
> https://issues.apache.org/jira/browse/MESOS-3944
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently role protobuf is defined in allocator.proto due to only the 
> traditional DRF allocator uses roles as it’s first level of hierarchy, I 
> think we should move it out and define it in a separated file as quota had in 
> dynamic roles project, because role protobuf will also be used by master to 
> persist.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   include/mesos/role/role.hpp PRE-CREATION 
>   include/mesos/role/role.proto PRE-CREATION 
>   src/CMakeLists.txt c0d77c745eb5b12dd6d9d7afaba7e820f8d848ef 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/40431/diff/
> 
> 
> Testing
> ---
> 
> 1. Make Check successfully;
> 
> 2. $ curl http://9.110.48.168:5050/roles
> {"roles":[{"frameworks":[],"name":"*","resources":{"cpus":0,"disk":0,"mem":0},"weight":1.0}]}
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 41487: Provisioner: Changed docker v2 manifest naming.

[Gilbert Song]

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

(Updated Dec. 17, 2015, 1:18 a.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.


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


Repository: mesos


Description
---

Provisioner: Changed docker v2 manifest naming.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/message.proto 
5c032701671b275d86c6d9276791a46df814396c 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
9b02b6ff6dc5c6e8aabdc4ac0aa4df337764ef30 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
e69bab43d5f5359cfd8eb4cb7c5ad4a1d22c4e05 
  src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
89f61c20e52e5eff8d8e92748f03b3b461516cd2 
  src/slave/containerizer/mesos/provisioner/docker/spec.hpp 
e674b6f2e705f8b3e49770560eeab4c127473f94 
  src/slave/containerizer/mesos/provisioner/docker/spec.cpp 
1f05c75dc9473bd5e4c0d3f74fa0ef996b96a84e 
  src/tests/containerizer/provisioner_docker_tests.cpp 
3f1717b770e139c3759aab0aeda9dbcf5029b0c2 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 41490: Provisioner: Added test case for docker v1 manifest serialization.

[Gilbert Song]

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

(Updated Dec. 17, 2015, 1:18 a.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.


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


Repository: mesos


Description
---

Provisioner: Added test case for docker v1 manifest serialization.


Diffs (updated)
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
3f1717b770e139c3759aab0aeda9dbcf5029b0c2 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

[Jan Schlicht]

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

(Updated Dec. 17, 2015, 10:26 a.m.)


Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
Remoortere, and Till Toenshoff.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

Quota: Implemented quota request authorization.


Diffs (updated)
-

  src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
  src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 41380: Added repeated vip field to DiscoveryInfo and an instance_port field to Port

[Avinash sridharan]

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

(Updated Dec. 17, 2015, 9:33 a.m.)


Review request for mesos, Adam B and Anand Mazumdar.


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


Repository: mesos


Description
---

Added repeated vip field to DiscoveryInfo and an instance_port field to Port


Diffs
-

  include/mesos/mesos.proto 8ca213062c480f0266ffc51a621eb4a118140c77 
  include/mesos/v1/mesos.proto 8f357b0fb778098ec66ac85d174bdd7e387954b5 

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


Testing
---

make check, and make


Thanks,

Avinash sridharan

Review Request 41503: [WIP] evict executor in slaves

[Klaus Ma]

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

Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Joris Van 
Remoortere, and Joseph Wu.


Repository: mesos


Description
---

This is an overall ticket to handle the action on evicting executor in slave. 
It's going to seperate into several RRs:
- checkAvailableResources
- evictExecutor
- shutdownExecutor enhancement


Diffs
-

  src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
  src/slave/slave.cpp 9bd86e122c070cd072a54d4de8097f844bd95bb0 

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


Testing
---


Thanks,

Klaus Ma

Review Request 41502: [WIP] offer allocation slack in allocator

[Klaus Ma]

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

Review request for mesos and Guangya Liu.


Repository: mesos


Description
---

This the overall tickets about allocation slack resources, there'll several RR 
to follow up:
- offer allocation slack in allocator
- update allocation slack for dynamic reservation
- return resources to allocator
- add allocation slack resource when slave register/re-register

The final design is discussing in working group.


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
99c742906874c30c39c159e58a65277ade3c07fd 
  src/master/allocator/mesos/hierarchical.cpp 
5da825a1d578a9ee40b4985378fddb3c5fb3b416 

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


Testing
---


Thanks,

Klaus Ma

Re: Review Request 41407: Unified Container: Save all docker image information on disk instead of rootfs only.

[Gilbert Song]

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

(Updated Dec. 17, 2015, 1:16 a.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.


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


Repository: mesos


Description
---

Unified Container: Save all docker image information on disk instead of rootfs 
only.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
4aa4a9c4074d96c30c3bceea59d071feeecae2ea 
  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
1ad7b67a94b1d9367afcb7c30a6d01fdf6b8ab6c 
  src/tests/containerizer/provisioner_docker_tests.cpp 
3f1717b770e139c3759aab0aeda9dbcf5029b0c2 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 41333: Added helper functions to filter usage slack resources.

[Guangya Liu]

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

(Updated Dec. 17, 2015, 9:37 a.m.)


Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, 
Joseph Wu, and Klaus Ma.


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


Repository: mesos


Description
---

Added helper functions to filter usage slack resources.


Diffs (updated)
-

  include/mesos/resources.hpp 9c8bfb0e7e1e40ca99b4cc75079e5b950630243d 
  include/mesos/v1/resources.hpp db87c992e2ec466cb1d4d17775bf6423a2fb15aa 
  src/common/resources.cpp 5a7981744726a0544435cbbd5007487a43a01211 
  src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320 
  src/v1/resources.cpp d300842576b2fb8fe75f5b18699f23c1fd324717 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 41334: Added helper functions to filter allocation slack resources.

[Guangya Liu]

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

(Updated Dec. 17, 2015, 9:43 a.m.)


Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, 
Joseph Wu, and Klaus Ma.


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


Repository: mesos


Description
---

Added helper functions to filter allocation slack resources.


Diffs (updated)
-

  include/mesos/resources.hpp 9c8bfb0e7e1e40ca99b4cc75079e5b950630243d 
  include/mesos/v1/resources.hpp db87c992e2ec466cb1d4d17775bf6423a2fb15aa 
  src/common/resources.cpp 5a7981744726a0544435cbbd5007487a43a01211 
  src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320 
  src/v1/resources.cpp d300842576b2fb8fe75f5b18699f23c1fd324717 

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


Testing (updated)
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 41337: WIP: Set task as TASK_LOST if not enough allocation slack resources.

[Guangya Liu]

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

(Updated Dec. 17, 2015, 9:22 a.m.)


Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, 
Joseph Wu, Klaus Ma, and Jian Qiu.


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


Repository: mesos


Description
---

Set task as TASK_LOST if not enough allocation slack resources.


Diffs (updated)
-

  src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
  src/slave/slave.cpp ef869695ffeb2e6d9ef0a78ddb676b1b7cd19afe 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 41380: Added repeated vip field to DiscoveryInfo and an instance_port field to Port

[Avinash sridharan]

> On Dec. 17, 2015, 8:37 a.m., Kevin Klues wrote:
> > This review depends on 41189, but 41189 is somehow marked as "Invite-only", 
> > so it is not viewable by the public.  This is causing the jenkins 
> > review-bot to fail with:
> > 
> > Verifying review 41381
> > Dependent review: https://reviews.apache.org/api/review-requests/41380/
> > Dependent review: https://reviews.apache.org/api/review-requests/41189/
> > Error handling URL https://reviews.apache.org/api/review-requests/41189/: 
> > FORBIDDEN
> > 
> > From my reading of 
> > https://www.reviewboard.org/docs/manual/2.5/admin/configuration/access-control/
> > My guess is all you need to do is make sure that 41189 has the 'mesos' 
> > group added to it. If no group is there, it will likely be treated as 
> > "Invite-only" since no public groups are accessible.

Thanks Kevin,
 Had discarded 41189. Since the changes were on the same branch the dependency 
got created but persisted after discarding 41189. Have updated the dependency 
to 41187


- Avinash


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


On Dec. 17, 2015, 9:33 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41380/
> ---
> 
> (Updated Dec. 17, 2015, 9:33 a.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-4114
> https://issues.apache.org/jira/browse/MESOS-4114
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added repeated vip field to DiscoveryInfo and an instance_port field to Port
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8ca213062c480f0266ffc51a621eb4a118140c77 
>   include/mesos/v1/mesos.proto 8f357b0fb778098ec66ac85d174bdd7e387954b5 
> 
> Diff: https://reviews.apache.org/r/41380/diff/
> 
> 
> Testing
> ---
> 
> make check, and make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 40345: [1/4] Quota Authorization: Added "SetQuota" message to ACL protobuf.

[Joerg Schad]

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

Ship it!



include/mesos/authorizer/authorizer.proto (line 127)


Not yours, but would it make sense to group the ACLs (and add some spacing)?


- Joerg Schad


On Dec. 4, 2015, 1:53 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40345/
> ---
> 
> (Updated Dec. 4, 2015, 1:53 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added "SetQuota" message to ACL protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 74fcc86d3c92cb3aa27e45b647b1653705b3201c 
> 
> Diff: https://reviews.apache.org/r/40345/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Review Request 41501: [WIP] Add flatten & allocationSlack into resources

[Klaus Ma]

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

Review request for mesos, Ben Mahler, Guangya Liu, Artem Harutyunyan, Joris Van 
Remoortere, and Joseph Wu.


Repository: mesos


Description
---

Add flatten & allocationSlack into resources; it's util for allocator to 
calculate allocation slack resources.


Diffs
-

  include/mesos/resources.hpp 9c8bfb0e7e1e40ca99b4cc75079e5b950630243d 
  src/common/resources.cpp 5a7981744726a0544435cbbd5007487a43a01211 

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


Testing
---


Thanks,

Klaus Ma

Re: Review Request 40346: [2/4] Quota Authorization: Implemented authorization of quota requests in the authorizer.

[Joerg Schad]

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

Ship it!



include/mesos/authorizer/authorizer.hpp (lines 177 - 180)


, otherwise ...


- Joerg Schad


On Dec. 15, 2015, 11:06 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40346/
> ---
> 
> (Updated Dec. 15, 2015, 11:06 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented authorization of quota requests in the authorizer.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> ec3d3e9c9bae461c8cd136f17cfda58e0cbd4e1b 
>   src/authorizer/local/authorizer.hpp 
> 965658b53d21734a2dbf3713d02d44d728fd6cca 
>   src/authorizer/local/authorizer.cpp 
> 1ac6a41d20454515a3ef2b84e296494a4592b3ea 
>   src/tests/mesos.hpp 689efb40dc22337766b624218d2504d5de3e5cd8 
>   src/tests/mesos.cpp a501f04dfbf9c93c2196e83b512519e925a67c97 
> 
> Diff: https://reviews.apache.org/r/40346/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40346: [2/4] Quota Authorization: Implemented authorization of quota requests in the authorizer.

[Joerg Schad]

> On Dec. 17, 2015, 10:20 a.m., Joerg Schad wrote:
> > include/mesos/authorizer/authorizer.hpp, lines 177-180
> > 
> >
> > , otherwise ...

(Consistent with the other comments)


- Joerg


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


On Dec. 15, 2015, 11:06 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40346/
> ---
> 
> (Updated Dec. 15, 2015, 11:06 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented authorization of quota requests in the authorizer.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> ec3d3e9c9bae461c8cd136f17cfda58e0cbd4e1b 
>   src/authorizer/local/authorizer.hpp 
> 965658b53d21734a2dbf3713d02d44d728fd6cca 
>   src/authorizer/local/authorizer.cpp 
> 1ac6a41d20454515a3ef2b84e296494a4592b3ea 
>   src/tests/mesos.hpp 689efb40dc22337766b624218d2504d5de3e5cd8 
>   src/tests/mesos.cpp a501f04dfbf9c93c2196e83b512519e925a67c97 
> 
> Diff: https://reviews.apache.org/r/40346/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 41334: Added helper functions to filter allocation slack resources.

[Klaus Ma]

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


@Guangya, there's some my comments:
- add a role parameter for `allocationSlack`, so we can remove role's reserved 
resources
- add `flatten(RevocableInfo::Type)` to avoid send `RevocableInfo::Type` by 
protobuf's func
- not sure how to define `usageSlack`, I'd suggest to remove it; and add it 
back when necessary, just as what we have done to `noRevoable`

- Klaus Ma


On Dec. 17, 2015, 5:43 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41334/
> ---
> 
> (Updated Dec. 17, 2015, 5:43 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, and Klaus Ma.
> 
> 
> Bugs: MESOS-4146
> https://issues.apache.org/jira/browse/MESOS-4146
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper functions to filter allocation slack resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 9c8bfb0e7e1e40ca99b4cc75079e5b950630243d 
>   include/mesos/v1/resources.hpp db87c992e2ec466cb1d4d17775bf6423a2fb15aa 
>   src/common/resources.cpp 5a7981744726a0544435cbbd5007487a43a01211 
>   src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320 
>   src/v1/resources.cpp d300842576b2fb8fe75f5b18699f23c1fd324717 
> 
> Diff: https://reviews.apache.org/r/41334/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 41475: Updated docker build script for centos 7.

[Greg Mann]

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

Ship it!


I confirmed that the original script failed as described with `OS=centos:7`, 
then tested the updated script with `OS=centos:7` and `OS=ubuntu:14.04`, and it 
looks good!

- Greg Mann


On Dec. 16, 2015, 11:39 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41475/
> ---
> 
> (Updated Dec. 16, 2015, 11:39 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Joris Van Remoortere, and Michael Park.
> 
> 
> Bugs: MESOS-4184
> https://issues.apache.org/jira/browse/MESOS-4184
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jenkins builds are consistently failing for centos 7, with the
> failure:
> 
> checking value of Java system property 'java.home'...
> /usr/lib/jvm/java-1.8.0-openjdk-1.8.0.65-3.b17.el7.x86_64/jre
> configure: error: could not guess JAVA_HOME
> 
> They also fail early on during 'bootstrap' with a missing 'which'
> command.
> 
> The solution is to update support/docker_build.sh to install 'which' as
> well as make sure the proper versions of java are installed during the
> installation process.
> 
> The problem with java is that we install maven BEFORE installing
> java-1.7.0-openjdk-devel, causing maven to pull in a dependency on
> java-1.8.0-openjdk. This causes problems with finding the proper
> java.home in our mesos/configure script because of the mismatch between
> the most up to date jre (1.8.0) and the most up to date development
> tools (1.7.0). We can either update the script to pull in the 1.8 devel
> tools or move our dependence on maven until AFTER our installation of
> java-1.7.0-openjdk-devel.
> 
> This commit takes the approach of updating to explicitly install the
> java-1.8.0 tools.
> 
> 
> Diffs
> -
> 
>   support/docker_build.sh c14370dcf5c0ccb883d798d9771b5503fa81c9a5 
> 
> Diff: https://reviews.apache.org/r/41475/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Review Request 41514: Accepted a single JSON object for quota set request.

[Alexander Rukletsov]

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

Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and 
Joris Van Remoortere.


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


Repository: mesos


Description
---

POST request to "/quota" requires a single JSON object as opposed to key-value 
pairs encoded in a string.


Diffs
-

  src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 
  src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74 

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


Testing
---

make check on Mac OS 10.10.4


Thanks,

Alexander Rukletsov

Review Request 41515: Refactored error messages to reduce jaggedness.

[Alexander Rukletsov]

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

Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
and Neil Conway.


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


Repository: mesos


Description
---

Includes minor updates to wording and punctuation.


Diffs
-

  src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 

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


Testing
---

make check on Mac OS 10.10.4


Thanks,

Alexander Rukletsov

Review Request 41509: [libprocess] Cleaned up STL I/O includes.

[Alexander Rukletsov]

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

Review request for mesos, Benjamin Bannier, Joris Van Remoortere, and Michael 
Park.


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


Repository: mesos


Description
---

Where possible move `operator<<` definitions and functions using streams to 
".cpp" files and include `` in ".hpp"s. Also remove unused I/O includes 
and clean up `std::` prefixes.


Diffs
-

  3rdparty/libprocess/include/process/address.hpp 
79429e904546ebeb993663efb9bd9e212c1dbd63 
  3rdparty/libprocess/include/process/future.hpp 
817fca2ba5352a2c1cea1cb6cf6ecc49821215e4 
  3rdparty/libprocess/include/process/http.hpp 
f0666f0fa48c4f3a98332d12066561a02a715236 
  3rdparty/libprocess/include/process/pid.hpp 
b22c160ad0051ea1dac733a39a9f833719dbcb58 
  3rdparty/libprocess/src/http.cpp e937df6875c8024ea9c178833b9faceede990969 
  3rdparty/libprocess/src/pid.cpp 1a9cbd1eec6aefbd9a40113ae0f4475a90011b85 

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


Testing
---

make check on Mac OS 10.10.4.


Thanks,

Alexander Rukletsov

Review Request 41511: Cleaned up STL I/O includes in public header (including v1).

[Alexander Rukletsov]

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

Review request for mesos, Benjamin Bannier, Joris Van Remoortere, and Michael 
Park.


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


Repository: mesos


Description
---

Where possible move `operator<<` definitions and functions using streams to 
".cpp" files and include `` in ".hpp"s. Also remove unused I/O 
includes, add necessary includes, clean up `std::` prefixes and blank lines.


Diffs
-

  include/mesos/resources.hpp 9c8bfb0e7e1e40ca99b4cc75079e5b950630243d 
  include/mesos/type_utils.hpp b370b5180e71ed246fa09bf0fe119dfa946d0b08 
  include/mesos/v1/mesos.hpp 80e76040de77e65b20f10b0465124ab86c1feab6 
  include/mesos/v1/resources.hpp db87c992e2ec466cb1d4d17775bf6423a2fb15aa 
  include/mesos/v1/values.hpp a160bd4d49d53fd2f794f36fea69ea99a455af09 
  include/mesos/values.hpp 58af972186d7156660eac742b41501ce420cefe9 
  src/common/resources.cpp 5a7981744726a0544435cbbd5007487a43a01211 
  src/common/type_utils.cpp c6c9ba7d1bfc6e6202b9e3ff94a74711e3602d6f 
  src/v1/mesos.cpp 13a58baf086ef3aaf46abcf7f58b71b7e2639728 
  src/v1/resources.cpp d300842576b2fb8fe75f5b18699f23c1fd324717 

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


Testing
---

make check on Mac OS 10.10.4.


Thanks,

Alexander Rukletsov

Review Request 41512: Cleaned up STL I/O includes in public headers which are not part of v1.

[Alexander Rukletsov]

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

Review request for mesos, Benjamin Bannier, Joris Van Remoortere, and Michael 
Park.


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


Repository: mesos


Description
---

Where possible move `operator<<` definitions and functions using streams to 
".cpp" files and include `` in ".hpp"s. Also remove unused I/O 
includes, add necessary includes, clean up `std::` prefixes.


Diffs
-

  include/mesos/authorizer/authorizer.hpp 
ec3d3e9c9bae461c8cd136f17cfda58e0cbd4e1b 
  include/mesos/http.hpp 8529ed0bf5b0954a223af5f7025fcd0d54c7348d 
  include/mesos/uri/uri.hpp aa3ab5d24bb8c501dd19e93d7563cb0afd889f23 
  src/authorizer/authorizer.cpp 31712e511cdaac3f6d137f425d5cd430434544aa 
  src/common/http.cpp 5198650ba6dc1ea3dab7912e5ef6e375bf9acc96 

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


Testing
---

make check on Mac OS 10.10.4.


Thanks,

Alexander Rukletsov

Review Request 41510: Cleaned up STL I/O includes in external containerizer.

[Alexander Rukletsov]

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

Review request for mesos, Michael Park and Till Toenshoff.


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


Repository: mesos


Description
---

Remove unused I/O includes, clean up blank lines and include order.


Diffs
-

  src/slave/containerizer/external_containerizer.hpp 
a5b4079a8f89c0c144468b42787c6cec78692d63 
  src/slave/containerizer/external_containerizer.cpp 
04cf31e005c2be8af0f80dbf856bb94b2ab5351a 

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


Testing
---

make check on Mac OS 10.10.4.


Thanks,

Alexander Rukletsov

Review Request 41513: Cleaned up STL I/O includes.

[Alexander Rukletsov]

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

Review request for mesos.


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


Repository: mesos


Description
---

Move `operator<<` definitions to ".cpp" files and include `` in ".hpp"s.


Diffs
-

  src/linux/perf.cpp 8ee9f381c3587bb2fa0f309a477035340bc44e15 
  src/linux/routing/filter/ip.hpp 1a63dcca2badc041e73fe454a9994b82695117de 
  src/linux/routing/filter/ip.cpp 158bab6cb95fe10b8dc41b22934a946b21ff88ed 
  src/linux/routing/handle.hpp 5b03be5e70ad3910487a1867fa4fa0d8de25d11e 
  src/linux/routing/handle.cpp e88c6eb7336ac31683a6ff394a139f6777576a3e 
  src/messages/messages.hpp 350118399927a8d7185d87a6a8e3370b7158fe14 
  src/messages/messages.cpp 30f55bb8ee5a57ff9517144ffc43d45df6b3de5b 

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


Testing
---

make check on Mac OS 10.10.4.


Thanks,

Alexander Rukletsov

Re: Review Request 41381: Added unit test cases to test the new vip and instance_port fields

[Avinash sridharan]

> On Dec. 17, 2015, 11:21 a.m., Adam B wrote:
> > src/tests/slave_tests.cpp, line 2240
> > 
> >
> > Do you want to verify that the vips are set as expected too?

The vips are part of the DiscoveryInfo and we are already comparing the JSON 
retrieved and the DiscoveryInfo set:
EXPECT_EQ(JSON::Object(JSON::protobuf(discovery)), discoveryObject);

Were you suggesting doing an explicit comparison?


- Avinash


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


On Dec. 17, 2015, 3:32 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41381/
> ---
> 
> (Updated Dec. 17, 2015, 3:32 a.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit test cases to test the new vip and instance_port fields
> 
> 
> Diffs
> -
> 
>   src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
> 
> Diff: https://reviews.apache.org/r/41381/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 41420: Added ContainerInfo to internal Task protobuf.

[Artem Harutyunyan]

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

(Updated Dec. 17, 2015, 9:39 a.m.)


Review request for mesos, Artem Harutyunyan and Jie Yu.


Repository: mesos


Description
---

Added ContainerInfo to internal Task protobuf.


Diffs
-

  src/messages/messages.proto b66bac14056407140bb3cf5aa3e67ac94df1a2f8 

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


Testing
---


Thanks,

Artem Harutyunyan

Re: Review Request 41421: Modified ptotobuf::createTask to use ContainerInfo from internal Task protobuf.

[Artem Harutyunyan]

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

(Updated Dec. 17, 2015, 9:39 a.m.)


Review request for mesos, Artem Harutyunyan and Jie Yu.


Repository: mesos


Description
---

Modified ptotobuf::createTask to use ContainerInfo from internal Task protobuf.


Diffs
-

  src/common/protobuf_utils.cpp 6e1eb0b8465809d1da5dac1cd29b692b9fa6ff66 

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


Testing
---


Thanks,

Artem Harutyunyan

Re: Review Request 41510: Cleaned up STL I/O includes in external containerizer.

[Gilbert Song]

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

Ship it!


Ship It!

- Gilbert Song


On Dec. 17, 2015, 6:37 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41510/
> ---
> 
> (Updated Dec. 17, 2015, 6:37 a.m.)
> 
> 
> Review request for mesos, Michael Park and Till Toenshoff.
> 
> 
> Bugs: MESOS-4183
> https://issues.apache.org/jira/browse/MESOS-4183
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove unused I/O includes, clean up blank lines and include order.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/external_containerizer.hpp 
> a5b4079a8f89c0c144468b42787c6cec78692d63 
>   src/slave/containerizer/external_containerizer.cpp 
> 04cf31e005c2be8af0f80dbf856bb94b2ab5351a 
> 
> Diff: https://reviews.apache.org/r/41510/diff/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.10.4.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41422: Removed manual construction of Task in master.

[Artem Harutyunyan]

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

(Updated Dec. 17, 2015, 9:40 a.m.)


Review request for mesos, Artem Harutyunyan and Jie Yu.


Repository: mesos


Description
---

Removed manual construction of Task in master.


Diffs
-

  src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4 

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


Testing
---


Thanks,

Artem Harutyunyan

Re: Review Request 41509: [libprocess] Cleaned up STL I/O includes.

[Gilbert Song]

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

Ship it!


Ship It!

- Gilbert Song


On Dec. 17, 2015, 6:37 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41509/
> ---
> 
> (Updated Dec. 17, 2015, 6:37 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Joris Van Remoortere, and Michael 
> Park.
> 
> 
> Bugs: MESOS-4183
> https://issues.apache.org/jira/browse/MESOS-4183
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Where possible move `operator<<` definitions and functions using streams to 
> ".cpp" files and include `` in ".hpp"s. Also remove unused I/O 
> includes and clean up `std::` prefixes.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/address.hpp 
> 79429e904546ebeb993663efb9bd9e212c1dbd63 
>   3rdparty/libprocess/include/process/future.hpp 
> 817fca2ba5352a2c1cea1cb6cf6ecc49821215e4 
>   3rdparty/libprocess/include/process/http.hpp 
> f0666f0fa48c4f3a98332d12066561a02a715236 
>   3rdparty/libprocess/include/process/pid.hpp 
> b22c160ad0051ea1dac733a39a9f833719dbcb58 
>   3rdparty/libprocess/src/http.cpp e937df6875c8024ea9c178833b9faceede990969 
>   3rdparty/libprocess/src/pid.cpp 1a9cbd1eec6aefbd9a40113ae0f4475a90011b85 
> 
> Diff: https://reviews.apache.org/r/41509/diff/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.10.4.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41421: Modified ptotobuf::createTask to use ContainerInfo from internal Task protobuf.

[Artem Harutyunyan]

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

(Updated Dec. 17, 2015, 10:26 a.m.)


Review request for mesos, Artem Harutyunyan and Jie Yu.


Repository: mesos


Description
---

Modified ptotobuf::createTask to use ContainerInfo from internal Task protobuf.


Diffs (updated)
-

  src/common/protobuf_utils.cpp 6e1eb0b8465809d1da5dac1cd29b692b9fa6ff66 

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


Testing
---


Thanks,

Artem Harutyunyan

Re: Review Request 40553: Enable mesos tests installation

[James Peach]

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

(Updated Dec. 17, 2015, 5:43 p.m.)


Review request for mesos, Benjamin Bannier and Till Toenshoff.


Changes
---

Rebased onto master.


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


Repository: mesos


Description
---

This patch enables the installation mesos-tests and its dependencies
and helper tool. The goal is to allow operators to build a separate
test package that can be run at deployment time to verify that Mesos
works in the deployment environment.

Since the build directory is searched first, to run it on a host
that has a build tree, you need to specify a non-existent tree:

~ $ $PREFIX/libexec/mesos/tests/mesos-tests --build_dir=/none

- Add --enable-tests-install
- Fix mesos-tests gmock dependencies
- Optionally install tests, helpers and test modules
- Add utility helpers to find various test resources

Current test status:

  [==] 784 tests from 106 test cases ran. (135304 ms total)
  [  PASSED  ] 780 tests.
  [  FAILED  ] 4 tests, listed below:
  [  FAILED  ] ExamplesTest.TestFramework
  [  FAILED  ] ExamplesTest.NoExecutorFramework
  [  FAILED  ] ExamplesTest.EventCallFramework
  [  FAILED  ] ExamplesTest.PersistentVolumeFramework


Diffs (updated)
-

  configure.ac 40d60a63cdba41d06305f09141f4d14d6e229d95 
  src/Makefile.am e6d48dc16135b5d147d036e851422686eff7d5ef 
  src/tests/containerizer/launch_tests.cpp 
c7ebe2606e4ff99ced90342dd16e0b4bf02bc504 
  src/tests/containerizer/memory_test_helper.cpp 
4a3de2e3c887aa6afc604588850e1386f92d8c11 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
fe679354d04d68b68e168cd8c4eab23898f6532f 
  src/tests/containerizer/ns_tests.cpp 603e54b7303c5aa15e2c5715dc7a2f7e7d39541b 
  src/tests/containerizer/port_mapping_tests.cpp 
9bcf05ec071b44156b57d8515f47ee6a8bbfdfa0 
  src/tests/environment.cpp 36f0ad0b739559e5c883d72585731944da4283cf 
  src/tests/fetcher_tests.cpp 1831d896ca8a52bec4adf87a67b6af845079796c 
  src/tests/health_check_tests.cpp 0fbccc373204d3b9431c614bdd6d046cc07e4566 
  src/tests/mesos.cpp a501f04dfbf9c93c2196e83b512519e925a67c97 
  src/tests/module.cpp e272bf0eccb61ae54440ec79adac8efad804c828 
  src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 
  src/tests/oversubscription_tests.cpp 7a75fb38e0177e33cf0e7cb82b4b9ebf8f05fe0a 
  src/tests/script.cpp ee44fef29fb40e414d7507168091ee5cd0d15736 
  src/tests/slave_tests.cpp f19199344b8b542ebad1e59f4b97d8ca661aeb06 
  src/tests/utils.hpp a6cca472f4dfab12cd6eccab6972206d842177aa 
  src/tests/utils.cpp 877139e97249761658dce3b1058cdc2e2a52367b 

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


Testing
---


Thanks,

James Peach

Re: Review Request 41487: Provisioner: Changed docker v2 manifest naming.

[Jie Yu]

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


Since you're on it. Can you rename docker::DockerImageManifest to 
docker::v2::ImageManifest?

- Jie Yu


On Dec. 17, 2015, 9:18 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41487/
> ---
> 
> (Updated Dec. 17, 2015, 9:18 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4186
> https://issues.apache.org/jira/browse/MESOS-4186
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Provisioner: Changed docker v2 manifest naming.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/message.proto 
> 5c032701671b275d86c6d9276791a46df814396c 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> 9b02b6ff6dc5c6e8aabdc4ac0aa4df337764ef30 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e69bab43d5f5359cfd8eb4cb7c5ad4a1d22c4e05 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> 89f61c20e52e5eff8d8e92748f03b3b461516cd2 
>   src/slave/containerizer/mesos/provisioner/docker/spec.hpp 
> e674b6f2e705f8b3e49770560eeab4c127473f94 
>   src/slave/containerizer/mesos/provisioner/docker/spec.cpp 
> 1f05c75dc9473bd5e4c0d3f74fa0ef996b96a84e 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 3f1717b770e139c3759aab0aeda9dbcf5029b0c2 
> 
> Diff: https://reviews.apache.org/r/41487/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 41423: Exposed partial contents of ContainerInfo through the state endpoint.

[Artem Harutyunyan]

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

(Updated Dec. 17, 2015, 11:57 a.m.)


Review request for mesos, Artem Harutyunyan and Jie Yu.


Repository: mesos


Description
---

Exposed partial contents of ContainerInfo through the state endpoint.


Diffs (updated)
-

  src/common/http.cpp 5198650ba6dc1ea3dab7912e5ef6e375bf9acc96 
  src/tests/containerizer/docker_containerizer_tests.cpp 
3f199e622dd337cdbf32d4368f4ead424c39823c 

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


Testing
---

make check


Thanks,

Artem Harutyunyan

Re: Review Request 41421: Modified ptotobuf::createTask to use ContainerInfo from internal Task protobuf.

[Artem Harutyunyan]

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

(Updated Dec. 17, 2015, 11:59 a.m.)


Review request for mesos, Artem Harutyunyan and Jie Yu.


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


Repository: mesos


Description
---

Modified ptotobuf::createTask to use ContainerInfo from internal Task protobuf.


Diffs
-

  src/common/protobuf_utils.cpp 6e1eb0b8465809d1da5dac1cd29b692b9fa6ff66 

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


Testing
---


Thanks,

Artem Harutyunyan

Re: Review Request 41422: Removed manual construction of Task in master.

[Artem Harutyunyan]

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

(Updated Dec. 17, 2015, 11:58 a.m.)


Review request for mesos, Artem Harutyunyan and Jie Yu.


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


Repository: mesos


Description
---

Removed manual construction of Task in master.


Diffs
-

  src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4 

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


Testing
---


Thanks,

Artem Harutyunyan

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Benjamin Hindman]

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

Ship it!


Only high-level comment is that the _container_ logger talks a lot about 
executor's and tasks, I need to look into some of the upcoming reviews, but why 
isn't it sufficient to only talk about containers?


include/mesos/slave/container_logger.hpp (line 40)


s/executors/containers/

s/of tasks launched by default executors/of tasks launched without an 
executor (that implicitly use the command executor)/



include/mesos/slave/container_logger.hpp (lines 43 - 44)


Make this part a TODO?



include/mesos/slave/container_logger.hpp (lines 73 - 75)


Can we document what the failure case of using an IO::PIPE means? As in, 
what if someone specifies an IO::PIPE? What breaks down?



include/mesos/slave/container_logger.hpp (line 125)


s/Error/Failure/
s/error/failure/



include/mesos/slave/container_logger.hpp (line 128)


What's the long term plan for this? Will we have multiple container loggers 
running simultaneously at some point?

I'd like you to capture a TODO here to follow up with the semantics on 
executors that can't be recovered. And let's roll this into the next phase so 
that we have consistent semantics here.



include/mesos/slave/container_logger.hpp (line 135)


s/launches an executor or a default executor launches a task/creates a 
container/



include/mesos/slave/container_logger.hpp (line 139)


s/an executor or task/container/



include/mesos/slave/container_logger.hpp (line 155)


Kill newline.


- Benjamin Hindman


On Dec. 15, 2015, 8:38 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> ---
> 
> (Updated Dec. 15, 2015, 8:38 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, 
> and Thomas Rampelberg.
> 
> 
> Bugs: MESOS-4087
> https://issues.apache.org/jira/browse/MESOS-4087
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The container logger is an agent module whose job is to take an executor or 
> task's stdout/stderr, and pipe it to some sink.
> 
> Existing executor and task stdout/stderr are piped into files ("stdout" and 
> "stderr") located in the executor's sandbox directory.
> This module aims to encompass this default behavior as well as allow more 
> sophisticated logging solutions.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/container_logger.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41002/diff/
> 
> 
> Testing
> ---
> 
> This is added to the Makefile later in the review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 41188: Providing JSON bindings to expose DiscoveryInfo protobuf messages to HTTP endpoints

[Avinash sridharan]

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

(Updated Dec. 17, 2015, 7:51 p.m.)


Review request for mesos, Adam B and Anand Mazumdar.


Summary (updated)
-

Providing JSON bindings to expose DiscoveryInfo protobuf messages to HTTP 
endpoints


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


Repository: mesos


Description
---

Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
protobuf messages to HTTP endpoints


Diffs
-

  src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
  src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
  src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 

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


Testing
---

make check.
Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
Also added Unit test to test that DiscoveryInfo gets exposed in master when 
TaskInfo protobuf is converted to JSON objects.


Thanks,

Avinash sridharan

Re: Review Request 41489: Provisioner: Implemented docker v1 parse serialization method in spec.

[Jie Yu]

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

Ship it!



src/slave/containerizer/mesos/provisioner/docker/spec.hpp (line 38)


s/validateManifest/validate/

Since ImageManifest is already presented in the function arguments.



src/slave/containerizer/mesos/provisioner/docker/spec.hpp (line 50)


s/validateManifest/validate/



src/slave/containerizer/mesos/provisioner/docker/spec.cpp (line 48)


2 lines apart


- Jie Yu


On Dec. 17, 2015, 2:39 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41489/
> ---
> 
> (Updated Dec. 17, 2015, 2:39 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4186
> https://issues.apache.org/jira/browse/MESOS-4186
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Provisioner: Implemented docker v1 parse serialization method in spec.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> e69bab43d5f5359cfd8eb4cb7c5ad4a1d22c4e05 
>   src/slave/containerizer/mesos/provisioner/docker/spec.hpp 
> e674b6f2e705f8b3e49770560eeab4c127473f94 
>   src/slave/containerizer/mesos/provisioner/docker/spec.cpp 
> 1f05c75dc9473bd5e4c0d3f74fa0ef996b96a84e 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 3f1717b770e139c3759aab0aeda9dbcf5029b0c2 
> 
> Diff: https://reviews.apache.org/r/41489/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 41361: Wrote cmd to create WANdiscoSVN file for centos.

[Joseph Wu]

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

Ship it!


Ship It!

- Joseph Wu


On Dec. 16, 2015, 3:09 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41361/
> ---
> 
> (Updated Dec. 16, 2015, 3:09 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joseph Wu, and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously we just told users to create this file and then add the
> contents to it. Now they can just copy and paste a single command to
> create the file and fill the contents automatically.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md 0225c9db985261b9c3057b2b376f29b1c80784fb 
> 
> Diff: https://reviews.apache.org/r/41361/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 41488: Provisioner: Added docker v1 manifest protobuf message.

[Jie Yu]

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



src/slave/containerizer/mesos/provisioner/docker/message.proto (line 51)


Ditto here. s/docker::V1DockerImageManifest/docker::v1::ImageManifest/



src/slave/containerizer/mesos/provisioner/docker/message.proto (line 52)


What's this field. I couldn't find it in the v1 spec? Also, why 
'architecture', 'authors', etc are not in this protobuf?


- Jie Yu


On Dec. 17, 2015, 2:39 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41488/
> ---
> 
> (Updated Dec. 17, 2015, 2:39 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4186
> https://issues.apache.org/jira/browse/MESOS-4186
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Provisioner: Added docker v1 manifest protobuf message.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/message.proto 
> 5c032701671b275d86c6d9276791a46df814396c 
> 
> Diff: https://reviews.apache.org/r/41488/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 41515: Refactored error messages to reduce jaggedness.

[Neil Conway]

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

Ship it!


Ship It!

- Neil Conway


On Dec. 17, 2015, 2:52 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41515/
> ---
> 
> (Updated Dec. 17, 2015, 2:52 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Neil Conway.
> 
> 
> Bugs: MESOS-3960
> https://issues.apache.org/jira/browse/MESOS-3960
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Includes minor updates to wording and punctuation.
> 
> 
> Diffs
> -
> 
>   src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 
> 
> Diff: https://reviews.apache.org/r/41515/diff/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41475: Updated docker build script for centos 7.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [41475]

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

- Mesos ReviewBot


On Dec. 16, 2015, 11:39 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41475/
> ---
> 
> (Updated Dec. 16, 2015, 11:39 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Joris Van Remoortere, and Michael Park.
> 
> 
> Bugs: MESOS-4184
> https://issues.apache.org/jira/browse/MESOS-4184
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jenkins builds are consistently failing for centos 7, with the
> failure:
> 
> checking value of Java system property 'java.home'...
> /usr/lib/jvm/java-1.8.0-openjdk-1.8.0.65-3.b17.el7.x86_64/jre
> configure: error: could not guess JAVA_HOME
> 
> They also fail early on during 'bootstrap' with a missing 'which'
> command.
> 
> The solution is to update support/docker_build.sh to install 'which' as
> well as make sure the proper versions of java are installed during the
> installation process.
> 
> The problem with java is that we install maven BEFORE installing
> java-1.7.0-openjdk-devel, causing maven to pull in a dependency on
> java-1.8.0-openjdk. This causes problems with finding the proper
> java.home in our mesos/configure script because of the mismatch between
> the most up to date jre (1.8.0) and the most up to date development
> tools (1.7.0). We can either update the script to pull in the 1.8 devel
> tools or move our dependence on maven until AFTER our installation of
> java-1.7.0-openjdk-devel.
> 
> This commit takes the approach of updating to explicitly install the
> java-1.8.0 tools.
> 
> 
> Diffs
> -
> 
>   support/docker_build.sh c14370dcf5c0ccb883d798d9771b5503fa81c9a5 
> 
> Diff: https://reviews.apache.org/r/41475/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 41532: Fixed formatting of C++ style guide.

[Neil Conway]

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

(Updated Dec. 17, 2015, 9:16 p.m.)


Review request for mesos and Jan Schlicht.


Repository: mesos


Description (updated)
---

Fixed formatting of C++ style guide.

Apparently Github's markdown is a bit more liberal than the tool we use to 
generate the docs HTML.


Diffs
-

  docs/c++-style-guide.md ff18c05b9777f1ee62233482059c765d1ea7afd4 

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


Testing
---

Previewed with mesos-site docker container.


Thanks,

Neil Conway

Re: Review Request 41187: Adding labels field to Port information, for service discovery to associate arbitrary tags by applications to ports for a given task.

[Avinash sridharan]

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

(Updated Dec. 17, 2015, 10:35 p.m.)


Review request for mesos, Adam B and Anand Mazumdar.


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


Repository: mesos


Description
---

Adding labels field to Port information, for service discovery to associate 
arbitrary tags by applications to ports for a given task.


Diffs (updated)
-

  include/mesos/mesos.proto 8ca213062c480f0266ffc51a621eb4a118140c77 
  include/mesos/v1/mesos.proto 8f357b0fb778098ec66ac85d174bdd7e387954b5 

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


Testing
---

make check


Thanks,

Avinash sridharan

Review Request 41532: Fixed formatting of C++ style guide.

[Neil Conway]

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

Review request for mesos.


Repository: mesos


Description
---

Fixed formatting of C++ style guide.


Diffs
-

  docs/c++-style-guide.md ff18c05b9777f1ee62233482059c765d1ea7afd4 

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


Testing
---

Previewed with mesos-site docker container.


Thanks,

Neil Conway

Re: Review Request 39888: Windows: Added compatibility code for `grp.h` and `pwd.h`.

[Joris Van Remoortere]

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

Ship it!



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/grp.hpp (line 
36)


this can go on the previous line as per existing pattern in the code base.



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/grp.hpp (line 
49)


let's mark the closing of the extern



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/pwd.hpp (line 
37)


ditto



3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/pwd.hpp (line 
50)


let's mark the closing of the extern


- Joris Van Remoortere


On Dec. 15, 2015, 11:36 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39888/
> ---
> 
> (Updated Dec. 15, 2015, 11:36 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This code will be particularly useful when we expand Windows support for
> `files/files.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> d1ef6f0df82e83d8e0d38f7b8986403702519a7d 
>   3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/grp.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/pwd.hpp 
> PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
> 43c85f50958dd9a7ee2ad7c32565585560486c69 
> 
> Diff: https://reviews.apache.org/r/39888/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39852: Windows: Replaced global `GetMessage` macro with inline function.

[Joris Van Remoortere]

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

Ship it!



3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (lines 68 - 71)


indentation.



3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp (lines 77 - 80)


indentation.


- Joris Van Remoortere


On Dec. 15, 2015, 10:10 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39852/
> ---
> 
> (Updated Dec. 15, 2015, 10:10 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Replaced global `GetMessage` macro with inline function.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows.hpp 
> 43c85f50958dd9a7ee2ad7c32565585560486c69 
> 
> Diff: https://reviews.apache.org/r/39852/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 40553: Enable mesos tests installation

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39780, 39781, 39782, 40553]

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

- Mesos ReviewBot


On Dec. 17, 2015, 5:43 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40553/
> ---
> 
> (Updated Dec. 17, 2015, 5:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Till Toenshoff.
> 
> 
> Bugs: MESOS-3608
> https://issues.apache.org/jira/browse/MESOS-3608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch enables the installation mesos-tests and its dependencies
> and helper tool. The goal is to allow operators to build a separate
> test package that can be run at deployment time to verify that Mesos
> works in the deployment environment.
> 
> Since the build directory is searched first, to run it on a host
> that has a build tree, you need to specify a non-existent tree:
> 
> ~ $ $PREFIX/libexec/mesos/tests/mesos-tests --build_dir=/none
> 
> - Add --enable-tests-install
> - Fix mesos-tests gmock dependencies
> - Optionally install tests, helpers and test modules
> - Add utility helpers to find various test resources
> 
> Current test status:
> 
>   [==] 784 tests from 106 test cases ran. (135304 ms total)
>   [  PASSED  ] 780 tests.
>   [  FAILED  ] 4 tests, listed below:
>   [  FAILED  ] ExamplesTest.TestFramework
>   [  FAILED  ] ExamplesTest.NoExecutorFramework
>   [  FAILED  ] ExamplesTest.EventCallFramework
>   [  FAILED  ] ExamplesTest.PersistentVolumeFramework
> 
> 
> Diffs
> -
> 
>   configure.ac 40d60a63cdba41d06305f09141f4d14d6e229d95 
>   src/Makefile.am e6d48dc16135b5d147d036e851422686eff7d5ef 
>   src/tests/containerizer/launch_tests.cpp 
> c7ebe2606e4ff99ced90342dd16e0b4bf02bc504 
>   src/tests/containerizer/memory_test_helper.cpp 
> 4a3de2e3c887aa6afc604588850e1386f92d8c11 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> fe679354d04d68b68e168cd8c4eab23898f6532f 
>   src/tests/containerizer/ns_tests.cpp 
> 603e54b7303c5aa15e2c5715dc7a2f7e7d39541b 
>   src/tests/containerizer/port_mapping_tests.cpp 
> 9bcf05ec071b44156b57d8515f47ee6a8bbfdfa0 
>   src/tests/environment.cpp 36f0ad0b739559e5c883d72585731944da4283cf 
>   src/tests/fetcher_tests.cpp 1831d896ca8a52bec4adf87a67b6af845079796c 
>   src/tests/health_check_tests.cpp 0fbccc373204d3b9431c614bdd6d046cc07e4566 
>   src/tests/mesos.cpp a501f04dfbf9c93c2196e83b512519e925a67c97 
>   src/tests/module.cpp e272bf0eccb61ae54440ec79adac8efad804c828 
>   src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 
>   src/tests/oversubscription_tests.cpp 
> 7a75fb38e0177e33cf0e7cb82b4b9ebf8f05fe0a 
>   src/tests/script.cpp ee44fef29fb40e414d7507168091ee5cd0d15736 
>   src/tests/slave_tests.cpp f19199344b8b542ebad1e59f4b97d8ca661aeb06 
>   src/tests/utils.hpp a6cca472f4dfab12cd6eccab6972206d842177aa 
>   src/tests/utils.cpp 877139e97249761658dce3b1058cdc2e2a52367b 
> 
> Diff: https://reviews.apache.org/r/40553/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 41188: Providing JSON bindings to expose DiscoveryInfo protobuf messages to HTTP endpoints

[Avinash sridharan]

> On Dec. 17, 2015, 12:12 a.m., Anand Mazumdar wrote:
> > src/tests/common/http_tests.cpp, lines 130-140
> > 
> >
> > Can you confirm that other objects that have labels that are exposed 
> > via `/state` endpoint also have a nested `labels` mapping ?
> > 
> > AFAICT, this is an artifact of us using the Protobuf to JSON converters 
> > when we should have just used the `model(...)` functions.
> 
> Avinash sridharan wrote:
> I am trying to understand this comment. Is it that applications don't 
> expect nested labels (for ports) when they read state.json? 
> 
> In terms of exposing labels in state.json I can see from 
> TEST_F(SlaveTest, TaskLabels) (slave_tests.cpp) the labels are nested within 
> tasks. Since, DiscoveryInfo also has labels and Port has labels unless we 
> nest them, the relationship would not be explicit if we do not replicate the 
> protobuf right?
> 
> Anand Mazumdar wrote:
> Let me give you an example:
> 
> Here is how the Task Labels JSON returned from this test looks like:
> 
> ```
> "tasks": [
> {
>   "executor_id": "default",
>   "framework_id": "5452180f-fa83-4d25-802f-8a0c974dfd21-",
>   "id": "1",
>   "labels": [
> {
>   "key": "foo",
>   "value": "bar"
> },
> {
>   "key": "bar",
>   "value": "baz"
> },
> {
>   "key": "bar",
>   "value": "qux"
> }
>   ],
>   "name": "",
>   "resources": {
> "cpus": 2,
> "disk": 1024,
> "mem": 1024,
> "ports": "[31000-32000]"
>   },
> ```
> 
> If you notice, it does not have a "labels" field nested inside a "labels" 
> field as we have when we do automated protobuf to JSON conversions. Do we 
> want to preserve this behavior ?
> 
> Avinash sridharan wrote:
> I get your point . You are right this is an artifact of using the 
> JSON::protobuf compred to explicit model functions . 
> 
> Is this going to be an issue? Since the dependency is reflected in the 
> protobuf definition.
> One reason I can of think of keeping it as is, is that the protobuf is a 
> contract between the framework and service discovery, so they might actually 
> expect this.
> 
> Adam B wrote:
> Note that your proposed changes also have "ports" nested under "ports".
> Arguments for the compact (no nesting) format:
> + App: Shorter find/get query strings, more compact json output to read.
> + Perf: Less json to generate, fewer bytes over the wire.
> + Not matching exact protobuf format means we can break the format easier 
> later.
> Arguments for nested format:
> + Direct json translation to/from protobuf; otherwise, all 
> clients/readers will have to manually update their parsing (in a potentially 
> undocumented manner) anytime the data is modeled differently.
> 
> Also note that this nested protobuf pattern is one of the less fortunate 
> side effects of having an `optional Labels labels` field instead of directly 
> using a `repeated Label labels` field. Maybe we should switch to the 
> `repeated` pattern for all labels/ports so we can use the direct 
> json/protobuf translation functions for everything?

Based on discussion with Adam we are going to keep it simple (though verbose) 
for now. With Mpark's changes in the near future all this would change/improve 
so tabling this discussion and sticking to the simple by keeping everything as 
is (in this code).


- Avinash


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


On Dec. 17, 2015, 7:51 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 17, 2015, 7:51 p.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
>   src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
>   src/tests/slave_tests.cpp 

Re: Review Request 40167: [2/7] Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Dec. 17, 2015, 6:58 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40167/
> ---
> 
> (Updated Dec. 17, 2015, 6:58 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-4178
> https://issues.apache.org/jira/browse/MESOS-4178
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 74fcc86d3c92cb3aa27e45b647b1653705b3201c 
> 
> Diff: https://reviews.apache.org/r/40167/diff/
> 
> 
> Testing
> ---
> 
> This is the second in a chain of 7 patches. `make check` was used to test 
> after all patches were applied.
> 
> Note that this chain of patches touches many of the same files as another 
> chain beginning with Review #39985 and ending with Review #39989, which is 
> currently in review as well. To avoid conflicts, the beginning of this chain 
> begins on top of Review #39989.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40168: [3/7] Added 'CreateVolume' and 'DestroyVolume' ACL support to the authorizer.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Dec. 17, 2015, 12:20 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40168/
> ---
> 
> (Updated Dec. 17, 2015, 12:20 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-4178
> https://issues.apache.org/jira/browse/MESOS-4178
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'CreateVolume' and 'DestroyVolume' ACL support to the authorizer.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> ec3d3e9c9bae461c8cd136f17cfda58e0cbd4e1b 
>   src/authorizer/local/authorizer.hpp 
> 965658b53d21734a2dbf3713d02d44d728fd6cca 
>   src/authorizer/local/authorizer.cpp 
> 1ac6a41d20454515a3ef2b84e296494a4592b3ea 
>   src/tests/authorization_tests.cpp b5fb3c30e4c53bf3bf081c2a9a30a327f2c7c880 
>   src/tests/mesos.hpp 689efb40dc22337766b624218d2504d5de3e5cd8 
>   src/tests/mesos.cpp a501f04dfbf9c93c2196e83b512519e925a67c97 
> 
> Diff: https://reviews.apache.org/r/40168/diff/
> 
> 
> Testing
> ---
> 
> This is the third in a chain of 7 patches. New tests were added to 
> `authorization_tests.cpp` in order to test the authorizer's handling of both 
> successful and failed authorization attempts using `ACL::CreateVolume` and 
> `ACL::DestroyVolume`. `make check` was used to test after all patches were 
> applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40169: [4/7] Added 'Master::authorize{Destroy, Create}Volume' to create/destroy persistent volumes.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Dec. 17, 2015, 12:29 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40169/
> ---
> 
> (Updated Dec. 17, 2015, 12:29 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-4179
> https://issues.apache.org/jira/browse/MESOS-4179
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'Master::authorize{Destroy,Create}Volume' to create/destroy persistent 
> volumes.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
>   src/master/master.cpp 0d1482279c68f2a4a27dabaf28774769a5d515c4 
> 
> Diff: https://reviews.apache.org/r/40169/diff/
> 
> 
> Testing
> ---
> 
> This is the fourth in a chain of 7 patches. `make check` was used to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39889: Windows: Added support for `files/files.hpp`.

[Joris Van Remoortere]

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

Ship it!


- Joris Van Remoortere


On Nov. 16, 2015, 9:15 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39889/
> ---
> 
> (Updated Nov. 16, 2015, 9:15 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added support for `files/files.hpp`.
> 
> 
> Diffs
> -
> 
>   src/files/files.hpp 818087b13cc787d0bd3186bb3e8a069751629bf9 
> 
> Diff: https://reviews.apache.org/r/39889/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 39889: Windows: Added support for `files/files.hpp`.

[Alex Clemmer]

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

(Updated Dec. 17, 2015, 10:11 p.m.)


Review request for mesos, Artem Harutyunyan, Michael Hopcroft, Joris Van 
Remoortere, and Joseph Wu.


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


Repository: mesos


Description
---

Windows: Added support for `files/files.hpp`.


Diffs
-

  src/files/files.hpp 818087b13cc787d0bd3186bb3e8a069751629bf9 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 41188: Providing JSON bindings to expose DiscoveryInfo protobuf messages to HTTP endpoints

[Avinash sridharan]

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

(Updated Dec. 17, 2015, 10:40 p.m.)


Review request for mesos, Adam B and Anand Mazumdar.


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


Repository: mesos


Description
---

Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
protobuf messages to HTTP endpoints


Diffs (updated)
-

  src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
  src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
  src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 

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


Testing
---

make check.
Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
Also added Unit test to test that DiscoveryInfo gets exposed in master when 
TaskInfo protobuf is converted to JSON objects.


Thanks,

Avinash sridharan

Re: Review Request 41380: Added repeated vip field to DiscoveryInfo and an instance_port field to Port

[Avinash sridharan]

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

(Updated Dec. 17, 2015, 10:41 p.m.)


Review request for mesos, Adam B and Anand Mazumdar.


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


Repository: mesos


Description
---

Added repeated vip field to DiscoveryInfo and an instance_port field to Port


Diffs (updated)
-

  include/mesos/mesos.proto 8ca213062c480f0266ffc51a621eb4a118140c77 
  include/mesos/v1/mesos.proto 8f357b0fb778098ec66ac85d174bdd7e387954b5 

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


Testing
---

make check, and make


Thanks,

Avinash sridharan

Re: Review Request 41532: Fixed formatting of C++ style guide.

[Adam B]

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

Ship it!


Ship It!

- Adam B


On Dec. 17, 2015, 1:16 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41532/
> ---
> 
> (Updated Dec. 17, 2015, 1:16 p.m.)
> 
> 
> Review request for mesos and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed formatting of C++ style guide.
> 
> Apparently Github's markdown is a bit more liberal than the tool we use to 
> generate the docs HTML.
> 
> 
> Diffs
> -
> 
>   docs/c++-style-guide.md ff18c05b9777f1ee62233482059c765d1ea7afd4 
> 
> Diff: https://reviews.apache.org/r/41532/diff/
> 
> 
> Testing
> ---
> 
> Previewed with mesos-site docker container.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41539: Enabled agent do not kill exeucutor for some error cases.

[Klaus Ma]

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

Ship it!


Ship It!

- Klaus Ma


On Dec. 18, 2015, 1:38 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41539/
> ---
> 
> (Updated Dec. 18, 2015, 1:38 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled agent do not kill exeucutor for some error cases.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp ef869695ffeb2e6d9ef0a78ddb676b1b7cd19afe 
> 
> Diff: https://reviews.apache.org/r/41539/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 41472: Disallowed dynamic reservations for roles not on the role whitelist.

[Adam B]

> On Dec. 16, 2015, 5:17 p.m., Michael Park wrote:
> > src/master/http.cpp, lines 1000-1006
> > 
> >
> > Hm, should this be added to `validation::operation::validate` instead? 
> > That way if/when we have frameworks reserving for specific roles, it'll 
> > just work?
> 
> Neil Conway wrote:
> I thought about that, but it would require passing either the role 
> whitelist or a pointer to the master itself into the 
> `validation::operation::validate()` -- both of which seemed like they would 
> be regrettable changes to make. What do you think?

I'm fine with it as is, but I'll wait for @mcypark to make the call.


- Adam


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


On Dec. 16, 2015, 2:48 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41472/
> ---
> 
> (Updated Dec. 16, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Bugs: MESOS-4143
> https://issues.apache.org/jira/browse/MESOS-4143
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added a test that dynamic reservations via the "/reserve" endpoint are
> allowed when using implicit roles.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
> 
> Diff: https://reviews.apache.org/r/41472/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41075: Added support for implicit roles.

[Yongqiao Wang]

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



src/master/master.cpp (line 588)


If the value of the specified weight is default value 1.0, do we still need 
to put it in weights hashmap and pass to allocator?


- Yongqiao Wang


On Dec. 18, 2015, 1:54 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 18, 2015, 1:54 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yongqiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd 
>   src/master/master.hpp 7cb0e1692644e51271588abffa832e08c536b838 
>   src/master/master.cpp 470b542729b01f41fc6a2e601a7a6c3d0c5353d5 
>   src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> e239b4746494fcc2b362a83afb634a2ce5e25f9b 
>   src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74 
> 
> Diff: https://reviews.apache.org/r/41075/diff/
> 
> 
> Testing
> ---
> 
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the 
> more likely role-related tests.
> 
> TODOs:
> 
> * Add tests for allocation behavior for weights + implicit roles
> * More tests for quota + implicit roles?
> 
> Notes:
> 
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` 
> would probably be nicer. I'm inclined to leave this as-is for now though 
> (making use of unique_ptr is a broader issue).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41225: Added test cases for implicit roles.

[Adam B]

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


Looks good. Just some minor changes and it'll be good to go.


src/tests/role_tests.cpp (line 47)


Why is this an instance member method of RoleTest? Seems idempotent and 
stateless enough that it could be a static method, or even a freestanding 
helper function. If this is only called once, why is it even a function at all?

Oh.. it's copied from `MasterQuotaTest::createRequestBody()`.

Either find a way to actually merge the two implementations, or just inline 
this into the one test that uses it.



src/tests/role_tests.cpp (line 107)


Is this necessary? The defaultAgentResourcesString is 
"cpus:2;mem:1024;disk:1024;ports:[31000-32000]"
Overriding that means that Mesos will auto-detect cpus/mem



src/tests/role_tests.cpp (line 120)


s/by default/the default/



src/tests/role_tests.cpp (line 145)


And expect that it doesn't already contain dynamicallyReserved?



src/tests/role_tests.cpp (line 160)


Could you also 
`EXPECT_FALSE(Resources(offer.resources()).contains(unreserved));`



src/tests/role_tests.cpp (line 162)


Why 'volume1' if there's no other volume? 'volume' should be fine.



src/tests/role_tests.cpp (lines 165 - 166)


Can you make these "volumeId1" and "/container/path" so it's clearer what 
they're supposed to be?



src/tests/role_tests.cpp (line 181)


And expect that it doesn't contain `unreserved` or `dynamicallyReserved` 
anymore.



src/tests/role_tests.cpp (line 216)


s/by/the/



src/tests/role_tests.cpp (lines 251 - 259)


Why bother to send back the ack? Or even send the status back at all? As 
soon as you have a MockExecutor that receives a launchTask, you've verified 
that "when using implicit roles, a static reservation can be used to launch a 
task"



src/tests/role_tests.cpp (line 379)


Seems we don't have any tests with weights yet. Congrats on writing the 
first!



src/tests/role_tests.cpp (line 397)


You could push this AWAIT down next to the AWAIT for fwkId2. No reason to 
block starting driver2 because you're waiting on driver1 to get its fwkId. 
Really you just want to make sure both fwks are registered before you hit 
`/roles`.



src/tests/role_tests.cpp (lines 483 - 484)


Can you clarify what "the expected information" is? Looks like this tests 
that `/roles` shows a role that has quota configured, even if that role has no 
frameworks registered, no reserved resources, and default weight.



src/tests/role_tests.cpp (line 495)


(because there are no slaves registered with any resources)


- Adam B


On Dec. 17, 2015, 5:55 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41225/
> ---
> 
> (Updated Dec. 17, 2015, 5:55 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Yongqiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for implicit roles.
> 
> 
> Diffs
> -
> 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41225/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 41539: Enabled agent do not kill exeucutor for some error cases.

[Guangya Liu]

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

Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, 
Joseph Wu, Klaus Ma, and Jian Qiu.


Repository: mesos


Description
---

Enabled agent do not kill exeucutor for some error cases.


Diffs
-

  src/slave/slave.cpp ef869695ffeb2e6d9ef0a78ddb676b1b7cd19afe 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 41408: Updated documentation for implicit roles.

[Adam B]

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


Looks good, but please also update the CHANGELOG and configuration.md


docs/upgrades.md (line 11)


Let's call out this JIRA(s) in the API changes section in the 0.27 
CHANGELOG as well.



docs/upgrades.md (lines 12 - 13)


"must be specified" - if you want to use roles. You can leave it out, but 
everything goes to "*"



docs/upgrades.md (line 18)


Link to a JIRA, diff, or other doc that explains the API change in more 
detail.



src/master/flags.cpp (lines 186 - 188)


Don't forget to update configuration.md to match your change to the flags.


- Adam B


On Dec. 15, 2015, 12:59 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41408/
> ---
> 
> (Updated Dec. 15, 2015, 12:59 p.m.)
> 
> 
> Review request for mesos, Adam B and Yongqiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated documentation for implicit roles.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md 3c5f2776d44a050ee3ac9967dd0ba0253b9c4558 
>   docs/configuration.md c75f56ce849f6960b3b6246bfa6949156a82eabb 
>   docs/roles.md 459d70717f4a0bdd921bb18b6ddc2505b6fc596e 
>   docs/upgrades.md 7c1f1814680078380ca33bbc27421675ffe61d60 
>   src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6 
> 
> Diff: https://reviews.apache.org/r/41408/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41539: Enabled agent do not kill exeucutor for some error cases.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [41539]

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

- Mesos ReviewBot


On Dec. 18, 2015, 5:38 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41539/
> ---
> 
> (Updated Dec. 18, 2015, 5:38 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled agent do not kill exeucutor for some error cases.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp ef869695ffeb2e6d9ef0a78ddb676b1b7cd19afe 
> 
> Diff: https://reviews.apache.org/r/41539/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 41472: Disallowed dynamic reservations for roles not on the role whitelist.

[Adam B]

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

Ship it!


Looks good to me. Address MPark's concern and let's commit it!

- Adam B


On Dec. 16, 2015, 2:48 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41472/
> ---
> 
> (Updated Dec. 16, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Bugs: MESOS-4143
> https://issues.apache.org/jira/browse/MESOS-4143
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added a test that dynamic reservations via the "/reserve" endpoint are
> allowed when using implicit roles.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 8dfd67f7d51b8395953d6beb77b2d71bc538eacd 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
> 
> Diff: https://reviews.apache.org/r/41472/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41475: Updated docker build script for centos 7.

[Joris Van Remoortere]

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

Ship it!


Ship It!

- Joris Van Remoortere


On Dec. 16, 2015, 11:39 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41475/
> ---
> 
> (Updated Dec. 16, 2015, 11:39 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Joris Van Remoortere, and Michael Park.
> 
> 
> Bugs: MESOS-4184
> https://issues.apache.org/jira/browse/MESOS-4184
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jenkins builds are consistently failing for centos 7, with the
> failure:
> 
> checking value of Java system property 'java.home'...
> /usr/lib/jvm/java-1.8.0-openjdk-1.8.0.65-3.b17.el7.x86_64/jre
> configure: error: could not guess JAVA_HOME
> 
> They also fail early on during 'bootstrap' with a missing 'which'
> command.
> 
> The solution is to update support/docker_build.sh to install 'which' as
> well as make sure the proper versions of java are installed during the
> installation process.
> 
> The problem with java is that we install maven BEFORE installing
> java-1.7.0-openjdk-devel, causing maven to pull in a dependency on
> java-1.8.0-openjdk. This causes problems with finding the proper
> java.home in our mesos/configure script because of the mismatch between
> the most up to date jre (1.8.0) and the most up to date development
> tools (1.7.0). We can either update the script to pull in the 1.8 devel
> tools or move our dependence on maven until AFTER our installation of
> java-1.7.0-openjdk-devel.
> 
> This commit takes the approach of updating to explicitly install the
> java-1.8.0 tools.
> 
> 
> Diffs
> -
> 
>   support/docker_build.sh c14370dcf5c0ccb883d798d9771b5503fa81c9a5 
> 
> Diff: https://reviews.apache.org/r/41475/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 41188: Providing JSON bindings to expose DiscoveryInfo protobuf messages to HTTP endpoints

[Avinash sridharan]

> On Dec. 17, 2015, 10:47 a.m., Adam B wrote:
> > src/tests/slave_tests.cpp, lines -2224
> > 
> >
> > Doesn't this all fit on one 80-char line?
> > 
> > Also, it looks like we have a AWAIT_EXPECT_RESPONSE_HEADER_EQ, but it 
> > doesn't really save any typing (1char?), and you don't need to AWAIT again.

Sticking with EXPECT_SOME_EQ for the time being.


- Avinash


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


On Dec. 17, 2015, 10:40 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 17, 2015, 10:40 p.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
>   src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
> 
> Diff: https://reviews.apache.org/r/41188/diff/
> 
> 
> Testing
> ---
> 
> make check.
> Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
> Also added Unit test to test that DiscoveryInfo gets exposed in master when 
> TaskInfo protobuf is converted to JSON objects.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Joseph Wu]

> On Dec. 17, 2015, 12:56 p.m., Benjamin Hindman wrote:
> > Only high-level comment is that the _container_ logger talks a lot about 
> > executor's and tasks, I need to look into some of the upcoming reviews, but 
> > why isn't it sufficient to only talk about containers?

I wanted to be explicit about the scope of the module.  i.e. All 
executors/tasks run in containers, but not all containers are executors/tasks.

Changed to use "container" in the comments.


> On Dec. 17, 2015, 12:56 p.m., Benjamin Hindman wrote:
> > include/mesos/slave/container_logger.hpp, lines 43-44
> > 
> >
> > Make this part a TODO?

I'll add a TODO to expose some sort of message or URL in the Mesos web UI.  

The module still needs to provide a log-reading interface (because write-only 
logs aren't very useful).  But Mesos won't know how to read said logs.


> On Dec. 17, 2015, 12:56 p.m., Benjamin Hindman wrote:
> > include/mesos/slave/container_logger.hpp, lines 73-75
> > 
> >
> > Can we document what the failure case of using an IO::PIPE means? As 
> > in, what if someone specifies an IO::PIPE? What breaks down?

Would it be reasonable to `ABORT` the agent if the module specifies a `PIPE`?  
Or perhaps make this a compile-time restriction (i.e. by introducing a subclass 
of `Subprocess::IO`).

(Note: The check for `PIPE` isn't implemented yet.)


> On Dec. 17, 2015, 12:56 p.m., Benjamin Hindman wrote:
> > include/mesos/slave/container_logger.hpp, line 128
> > 
> >
> > What's the long term plan for this? Will we have multiple container 
> > loggers running simultaneously at some point?
> > 
> > I'd like you to capture a TODO here to follow up with the semantics on 
> > executors that can't be recovered. And let's roll this into the next phase 
> > so that we have consistent semantics here.

For Docker command executors, there will be a container logger running 
alongside the executor.  But it will only ever spawn one task (because the 
executor will refuse >1 task).

In general, this means we could:

- Launch an executor that logs to the sandbox,
- Restart the agent with a special container logger (say, to syslog),
- Have the recovered executor continue logging to the sandbox, while new 
executors log to syslog.


- Joseph


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


On Dec. 17, 2015, 3:46 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41002/
> ---
> 
> (Updated Dec. 17, 2015, 3:46 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, 
> and Thomas Rampelberg.
> 
> 
> Bugs: MESOS-4087
> https://issues.apache.org/jira/browse/MESOS-4087
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The container logger is an agent module whose job is to take an executor or 
> task's stdout/stderr, and pipe it to some sink.
> 
> Existing executor and task stdout/stderr are piped into files ("stdout" and 
> "stderr") located in the executor's sandbox directory.
> This module aims to encompass this default behavior as well as allow more 
> sophisticated logging solutions.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/container_logger.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41002/diff/
> 
> 
> Testing
> ---
> 
> This is added to the Makefile later in the review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 41514: Accepted a single JSON object for quota set request.

[Anand Mazumdar]

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


LGTM.. Just some nits around:

- using `const` for test strings.
- reducing jaggedness for some blocks.

Also, a query regarding just accepting `true/false` as `force` field values.


src/master/quota_handler.cpp (line 250)


nit: use backticks (`force`)
s/flag/field



src/master/quota_handler.cpp (line 274)


hmm,  why don't we do a `using google::protobuf::RepeatedPtrField` and get 
rid of all this jaggedness ?



src/master/quota_handler.cpp (line 313)


I like the less jagged version more here. It's also more consistent with 
other similar strings in the function. What do you think ?



src/master/quota_handler.cpp (line 328)


hmmm .. Should we return a `BadRequest` for all other non-allowed values of 
`force` other then `true` or `false` ?



src/tests/master_quota_tests.cpp (line 116)


nit: `const string`



src/tests/master_quota_tests.cpp (line 122)


nit: `const string`



src/tests/master_quota_tests.cpp (line 199)


Not related to this review : Why don't we make all these `badRequest` 
expected strings `const` ?


- Anand Mazumdar


On Dec. 17, 2015, 2:52 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41514/
> ---
> 
> (Updated Dec. 17, 2015, 2:52 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Bernd Mathiske, Joerg Schad, and 
> Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3960
> https://issues.apache.org/jira/browse/MESOS-3960
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> POST request to "/quota" requires a single JSON object as opposed to 
> key-value pairs encoded in a string.
> 
> 
> Diffs
> -
> 
>   src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 
>   src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74 
> 
> Diff: https://reviews.apache.org/r/41514/diff/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 41381: Added unit test cases to test the new vip and instance_port fields

[Avinash sridharan]

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

(Updated Dec. 17, 2015, 10:43 p.m.)


Review request for mesos, Adam B and Anand Mazumdar.


Repository: mesos


Description
---

Added unit test cases to test the new vip and instance_port fields


Diffs (updated)
-

  src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
  src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 

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


Testing
---

make check


Thanks,

Avinash sridharan

Re: Review Request 41381: Added unit test cases to test the new vip and instance_port fields

[Avinash sridharan]

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

(Updated Dec. 17, 2015, 11:15 p.m.)


Review request for mesos, Adam B and Anand Mazumdar.


Repository: mesos


Description
---

Added unit test cases to test the new vip and instance_port fields


Diffs (updated)
-

  src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
  src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 

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


Testing
---

make check


Thanks,

Avinash sridharan

Re: Review Request 41002: Logger Module: Introduce the ContainerLogger interface for logging the stdout/stderr of executors and tasks.

[Joseph Wu]

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

(Updated Dec. 17, 2015, 3:46 p.m.)


Review request for mesos, Benjamin Hindman, Cody Maloney, Artem Harutyunyan, 
and Thomas Rampelberg.


Changes
---

Comment changes, per BenH's comments.


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


Repository: mesos


Description
---

The container logger is an agent module whose job is to take an executor or 
task's stdout/stderr, and pipe it to some sink.

Existing executor and task stdout/stderr are piped into files ("stdout" and 
"stderr") located in the executor's sandbox directory.
This module aims to encompass this default behavior as well as allow more 
sophisticated logging solutions.


Diffs (updated)
-

  include/mesos/slave/container_logger.hpp PRE-CREATION 

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


Testing
---

This is added to the Makefile later in the review chain.


Thanks,

Joseph Wu

Re: Review Request 41188: Providing JSON bindings to expose DiscoveryInfo protobuf messages to HTTP endpoints

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [41187, 41188]

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

- Mesos ReviewBot


On Dec. 18, 2015, 2:45 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 18, 2015, 2:45 a.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
>   src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
> 
> Diff: https://reviews.apache.org/r/41188/diff/
> 
> 
> Testing
> ---
> 
> make check.
> Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
> Also added Unit test to test that DiscoveryInfo gets exposed in master when 
> TaskInfo protobuf is converted to JSON objects.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 41076: Added tests for implicit roles.

[Neil Conway]

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

(Updated Dec. 17, 2015, 7:59 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
Mann, and Yongqiao Wang.


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


Repository: mesos


Description
---

Also added tests for the "/role" HTTP endpoint.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
fb214a829a57529d3f5c49730ae9733f53e622ca 
  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---


Thanks,

Neil Conway

Re: Review Request 41119: Cleaned up DRF allocator tests.

[Neil Conway]

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

(Updated Dec. 17, 2015, 8:01 p.m.)


Review request for mesos.


Repository: mesos


Description
---

Cleaned up DRF allocator tests.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
fb214a829a57529d3f5c49730ae9733f53e622ca 

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


Testing
---


Thanks,

Neil Conway

Re: Review Request 41250: Enabled slave get ALLOCATION_SLACK metrics.

[Klaus Ma]

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



src/slave/slave.cpp (line 5194)


So we are going to only cover `SCALAR` resources? Is that necessary?


- Klaus Ma


On Dec. 14, 2015, 3:13 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41250/
> ---
> 
> (Updated Dec. 14, 2015, 3:13 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, 
> and Klaus Ma.
> 
> 
> Bugs: MESOS-4124
> https://issues.apache.org/jira/browse/MESOS-4124
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled slave get ALLOCATION_SLACK metrics.
> 
> 
> Diffs
> -
> 
>   src/slave/metrics.hpp 2c22de521309806d70f9bd124359756b5e4c75b4 
>   src/slave/metrics.cpp 1b73121264de3ff6eabc8f620e5b4d5930ba5d43 
>   src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
>   src/slave/slave.cpp 9bd86e122c070cd072a54d4de8097f844bd95bb0 
> 
> Diff: https://reviews.apache.org/r/41250/diff/
> 
> 
> Testing
> ---
> 
> $ curl "http://192.168.0.107:5051/metrics/snapshot; 2>/dev/null| jq . | grep 
> alloc_slack
>   "slave/cpus_alloc_slack_revocable_percent": 0,
>   "slave/cpus_alloc_slack_revocable_total": 3,
>   "slave/cpus_alloc_slack_revocable_used": 0,
>   "slave/disk_alloc_slack_revocable_percent": 0,
>   "slave/disk_alloc_slack_revocable_total": 0,
>   "slave/disk_alloc_slack_revocable_used": 0,
>   "slave/mem_alloc_slack_revocable_percent": 0,
>   "slave/mem_alloc_slack_revocable_total": 1000,
>   "slave/mem_alloc_slack_revocable_used": 0,
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 41244: Using 'git rev-parse --git-dir' in post-reviews.py.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [41243, 41244]

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

- Mesos ReviewBot


On Dec. 17, 2015, 3:44 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41244/
> ---
> 
> (Updated Dec. 17, 2015, 3:44 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-4125
> https://issues.apache.org/jira/browse/MESOS-4125
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The post-reviews.py script has similar problems to the bootstrap script
> in assuming that .git is a folder in the top-level directory of the git
> repo.  This may not be true when mesos is included as a submodule in
> another project. Use 'git rev-parse --git-dir' instead.
> 
> 
> Diffs
> -
> 
>   support/post-reviews.py 170be83aa6dca6e8175292169d78e8f7915f7e6e 
> 
> Diff: https://reviews.apache.org/r/41244/diff/
> 
> 
> Testing
> ---
> 
> I have been posting all of my reviews with this script since first creating 
> this commit. Tested both in an environment with and without mesos embedded as 
> a submodule inside another git repository.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Adam B]

> On Dec. 16, 2015, 4:12 p.m., Anand Mazumdar wrote:
> > src/tests/common/http_tests.cpp, lines 130-140
> > 
> >
> > Can you confirm that other objects that have labels that are exposed 
> > via `/state` endpoint also have a nested `labels` mapping ?
> > 
> > AFAICT, this is an artifact of us using the Protobuf to JSON converters 
> > when we should have just used the `model(...)` functions.
> 
> Avinash sridharan wrote:
> I am trying to understand this comment. Is it that applications don't 
> expect nested labels (for ports) when they read state.json? 
> 
> In terms of exposing labels in state.json I can see from 
> TEST_F(SlaveTest, TaskLabels) (slave_tests.cpp) the labels are nested within 
> tasks. Since, DiscoveryInfo also has labels and Port has labels unless we 
> nest them, the relationship would not be explicit if we do not replicate the 
> protobuf right?
> 
> Anand Mazumdar wrote:
> Let me give you an example:
> 
> Here is how the Task Labels JSON returned from this test looks like:
> 
> ```
> "tasks": [
> {
>   "executor_id": "default",
>   "framework_id": "5452180f-fa83-4d25-802f-8a0c974dfd21-",
>   "id": "1",
>   "labels": [
> {
>   "key": "foo",
>   "value": "bar"
> },
> {
>   "key": "bar",
>   "value": "baz"
> },
> {
>   "key": "bar",
>   "value": "qux"
> }
>   ],
>   "name": "",
>   "resources": {
> "cpus": 2,
> "disk": 1024,
> "mem": 1024,
> "ports": "[31000-32000]"
>   },
> ```
> 
> If you notice, it does not have a "labels" field nested inside a "labels" 
> field as we have when we do automated protobuf to JSON conversions. Do we 
> want to preserve this behavior ?
> 
> Avinash sridharan wrote:
> I get your point . You are right this is an artifact of using the 
> JSON::protobuf compred to explicit model functions . 
> 
> Is this going to be an issue? Since the dependency is reflected in the 
> protobuf definition.
> One reason I can of think of keeping it as is, is that the protobuf is a 
> contract between the framework and service discovery, so they might actually 
> expect this.

Note that your proposed changes also have "ports" nested under "ports".
Arguments for the compact (no nesting) format:
+ App: Shorter find/get query strings, more compact json output to read.
+ Perf: Less json to generate, fewer bytes over the wire.
+ Not matching exact protobuf format means we can break the format easier later.
Arguments for nested format:
+ Direct json translation to/from protobuf; otherwise, all clients/readers will 
have to manually update their parsing (in a potentially undocumented manner) 
anytime the data is modeled differently.

Also note that this nested protobuf pattern is one of the less fortunate side 
effects of having an `optional Labels labels` field instead of directly using a 
`repeated Label labels` field. Maybe we should switch to the `repeated` pattern 
for all labels/ports so we can use the direct json/protobuf translation 
functions for everything?


- Adam


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


On Dec. 16, 2015, 7:10 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 16, 2015, 7:10 p.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
>   src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
> 
> Diff: https://reviews.apache.org/r/41188/diff/
> 
> 
> Testing
> ---
> 
> make check.
> Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
> Also added Unit test to test that DiscoveryInfo gets exposed in master when 
> TaskInfo protobuf is converted to JSON objects.
> 
> 
> 

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

[Joerg Schad]

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

Ship it!



src/master/quota_handler.cpp (line 348)


Shouldn't this comment be above line 327?


- Joerg Schad


On Dec. 17, 2015, 9:26 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40347/
> ---
> 
> (Updated Dec. 17, 2015, 9:26 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented quota request authorization.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
>   src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 
> 
> Diff: https://reviews.apache.org/r/40347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

[Jan Schlicht]

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

(Updated Dec. 17, 2015, 12:16 p.m.)


Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
Remoortere, and Till Toenshoff.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

Quota: Implemented quota request authorization.


Diffs (updated)
-

  src/master/master.hpp 9aa548aa6e159046c94e4ec96f631ea8b3bfd5d8 
  src/master/quota_handler.cpp 11167879b2480d9c8dd6398ca39c479089ec2272 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 40348: [4/4] Quota Authorization: Documented quota authorization.

[Jan Schlicht]

> On Dec. 14, 2015, 4:57 p.m., Alexander Rukletsov wrote:
> > docs/authorization.md, line 7
> > 
> >
> > I think this can be a bit misleading: these features are not added in 
> > 0.20.0.
> > 
> > Moreover, it feels a bit weird to maintain a "changelog" kind of thing 
> > here. Let's add stuff here as it comes and put it into the official 
> > changelog as needed.

Yes, given the context, this sentence doesn't make sense anymore. I will remove 
it. The section can then serve as an overview of possible authorization 
capabilities.


- Jan


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


On Dec. 15, 2015, 12:08 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40348/
> ---
> 
> (Updated Dec. 15, 2015, 12:08 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Documented quota authorization.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md 1fc0e3f9a686480ffc4deb25e49f867318d1321a 
>   src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6 
> 
> Diff: https://reviews.apache.org/r/40348/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 41337: WIP: Set task as TASK_LOST if not enough allocation slack resources.

[Klaus Ma]

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



src/slave/slave.cpp (line 4860)


Honestly, I'd suggest to update `shutdownExecutor` to return future; so 
slave'll wait all future finished before `runTask`. LaunchTask periodic does 
not make sense :).


- Klaus Ma


On Dec. 17, 2015, 5:22 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41337/
> ---
> 
> (Updated Dec. 17, 2015, 5:22 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van 
> Remoortere, Joseph Wu, Klaus Ma, and Jian Qiu.
> 
> 
> Bugs: MESOS-4148
> https://issues.apache.org/jira/browse/MESOS-4148
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set task as TASK_LOST if not enough allocation slack resources.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp b7586ce42bfac9d9885a3eb8d82deb94680c236c 
>   src/slave/slave.cpp ef869695ffeb2e6d9ef0a78ddb676b1b7cd19afe 
> 
> Diff: https://reviews.apache.org/r/41337/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Adam B]

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


Looks great! Barring the nested labels discussion, I just had a couple of nits 
and it's otherwise shippable.


src/tests/common/http_tests.cpp (lines 38 - 39)


Alphabetize, please



src/tests/common/http_tests.cpp (lines 81 - 82)


Need to set DiscoveryInfo.visibility, since it's `required`



src/tests/slave_tests.cpp (line 2169)


`Times(1)` is the default, unnecessary. Please delete.



src/tests/slave_tests.cpp (line 2184)


May want to use a different example than "vip", if we're about to add 
`vips` as a first-class field.



src/tests/slave_tests.cpp (lines 2190 - 2194)


s/`Port *port`/`Port* port`/



src/tests/slave_tests.cpp (line 2219)


Don't think you have to AWAIT on `response` here, since you AWAIT_EXPECT on 
it just below.



src/tests/slave_tests.cpp (lines  - 2224)


Doesn't this all fit on one 80-char line?

Also, it looks like we have a AWAIT_EXPECT_RESPONSE_HEADER_EQ, but it 
doesn't really save any typing (1char?), and you don't need to AWAIT again.


- Adam B


On Dec. 16, 2015, 7:10 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 16, 2015, 7:10 p.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
>   src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
> 
> Diff: https://reviews.apache.org/r/41188/diff/
> 
> 
> Testing
> ---
> 
> make check.
> Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
> Also added Unit test to test that DiscoveryInfo gets exposed in master when 
> TaskInfo protobuf is converted to JSON objects.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 40348: [4/4] Quota Authorization: Documented quota authorization.

[Joerg Schad]

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



docs/authorization.md (line 7)


s/Authorization allows/Authorization currently allows



docs/authorization.md (line 17)


s/For each of the 4 cases /For each of the above cases?


- Joerg Schad


On Dec. 15, 2015, 11:08 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40348/
> ---
> 
> (Updated Dec. 15, 2015, 11:08 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Documented quota authorization.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md 1fc0e3f9a686480ffc4deb25e49f867318d1321a 
>   src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6 
> 
> Diff: https://reviews.apache.org/r/40348/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 41380: Added repeated vip field to DiscoveryInfo and an instance_port field to Port

[Adam B]

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


Please explain (the intended use of) instance_port better. Otherwise shippable.


include/mesos/mesos.proto (line 1577)


When/why/how would it be different from the port number? Please give a 
clearer example.

If it would help the explanation to have `number` and `instance_port` next 
to each other, you can put `instance_port` just below `number`. The ordering in 
the protobuf definition doesn't matter, since it's the `= 6` fieldId that 
determines order over the wire.



include/mesos/mesos.proto (lines 1615 - 1616)


Please add this comment above the vips field, rather than bloating the 
message-level comment further. Comments that only talk about a single field 
don't have a reason to be at the top-level.



include/mesos/mesos.proto (lines 1617 - 1618)


Fix wrapping


- Adam B


On Dec. 17, 2015, 1:33 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41380/
> ---
> 
> (Updated Dec. 17, 2015, 1:33 a.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-4114
> https://issues.apache.org/jira/browse/MESOS-4114
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added repeated vip field to DiscoveryInfo and an instance_port field to Port
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8ca213062c480f0266ffc51a621eb4a118140c77 
>   include/mesos/v1/mesos.proto 8f357b0fb778098ec66ac85d174bdd7e387954b5 
> 
> Diff: https://reviews.apache.org/r/41380/diff/
> 
> 
> Testing
> ---
> 
> make check, and make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 41438: Added documentation on using network proxy for mesos fetcher

[Bernd Mathiske]

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



docs/fetcher.md (line 224)


s/a/an

How about this title?

"HTTP and SOCKS proxy settings"



docs/fetcher.md (lines 225 - 226)


s/it's/it is



docs/fetcher.md (line 229)


Since multiple protocols may be addressed, would plural read better here? 
"when certain environment variables are set."

(Otherwise, an "a" is missing before "certain".)



docs/fetcher.md (line 231)


s/The/The respective

This may make it even more clear that this is a variable per scheme



docs/fetcher.md (line 232)


s/uri/URI

BTW: You can end the sentence after this word and start a new one.



docs/fetcher.md (line 239)


We should point out that FTP/FTPS also go over HTTP/HTTPS proxies. Even 
though in general this  constrains the available FTP protocol operations, 
everything the fetcher uses is supported.



docs/fetcher.md (line 241)


Suggestion: "Your proxy settings can be placed in `...`. Here is an 
example:"



docs/fetcher.md (line 244)


Did you not have to write "export" in front?
I know of a case where nothing worked unless you did.



docs/fetcher.md (line 248)


Suggestion (less uncertain than "would" and less colloquial than "get"): 

The fetcher will pick up these environment variable settings since the 
utility program mesos-fetcher which it employs is a child of mesos-slave.


- Bernd Mathiske


On Dec. 16, 2015, 12:27 a.m., Shuai Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41438/
> ---
> 
> (Updated Dec. 16, 2015, 12:27 a.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Bugs: MESOS-4130
> https://issues.apache.org/jira/browse/MESOS-4130
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation on using network proxy for mesos fetcher
> 
> 
> Diffs
> -
> 
>   docs/fetcher.md ec7372293491ed9ee4e20dda3c2def72a77a3f84 
> 
> Diff: https://reviews.apache.org/r/41438/diff/
> 
> 
> Testing
> ---
> 
> - Set `https_proxy=http://localhost:3128` (a local squid server) in 
> `/etc/default/mesos-slave`, restart mesos-slave, launch a new marathon task 
> with a `https://` uri, and through the following sysdig command I can see 
> there is data exchanging between mesos-fetcher process and port 3128.
> 
> ```
> sudo sysdig -A -c echo_fds proc.name=mesos-fetcher and fd.port=3128
> ```
> 
> - Stop the local squid server, try to restart the marathon task, the task 
> would fail repeatly, from slave logs there are error messages that fetcher 
> failed to fetch the uri.
> ```
> Dec 16 15:16:35 lin-E400 mesos-slave[24247]: E1216 15:16:35.678032 24283 
> slave.cpp:3342] Container '45c14132-c56a-4cff-a6b5-f57ba2670643' for executor 
> 'testapp_web.f0ef72d2-a3c4-11e5-af60-56847afe9799' of framework 
> 'b52179fd-8968-4
> bf8-baf0-dddc8a38c903-' failed to start: Failed to fetch all URIs for 
> container '45c14132-c56a-4cff-a6b5-f57ba2670643' with exit status: 2
> ```
> 
> - Restart the squid server, the task would start without a problem.
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>

Re: Review Request 40346: [2/4] Quota Authorization: Implemented authorization of quota requests in the authorizer.

[Jan Schlicht]

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

(Updated Dec. 17, 2015, 12:15 p.m.)


Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
Remoortere, and Till Toenshoff.


Changes
---

Addressed issues.


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


Repository: mesos


Description
---

Quota: Implemented authorization of quota requests in the authorizer.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp 
ec3d3e9c9bae461c8cd136f17cfda58e0cbd4e1b 
  src/authorizer/local/authorizer.hpp 965658b53d21734a2dbf3713d02d44d728fd6cca 
  src/authorizer/local/authorizer.cpp 1ac6a41d20454515a3ef2b84e296494a4592b3ea 
  src/tests/mesos.hpp 689efb40dc22337766b624218d2504d5de3e5cd8 
  src/tests/mesos.cpp a501f04dfbf9c93c2196e83b512519e925a67c97 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 41381: Added unit test cases to test the new vip and instance_port fields

[Adam B]

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


Errors in one of the tests, but only minor suggestions otherwise.


src/tests/common/http_tests.cpp (lines 78 - 79)


Can you make these different? Just in case we run into a bug where we 
report each one as the other, or something equally tricksy



src/tests/common/http_tests.cpp (lines 148 - 150)


There's that nesting again..



src/tests/slave_tests.cpp (lines 2197 - 2199)


s/port1/port2/?
And you call `port2->set_number()` twice. I assume the second one should 
have been the `port2->set_instance_port()`?



src/tests/slave_tests.cpp (line 2240)


Do you want to verify that the vips are set as expected too?


- Adam B


On Dec. 16, 2015, 7:32 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41381/
> ---
> 
> (Updated Dec. 16, 2015, 7:32 p.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit test cases to test the new vip and instance_port fields
> 
> 
> Diffs
> -
> 
>   src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
> 
> Diff: https://reviews.apache.org/r/41381/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 41438: Added documentation on using network proxy for mesos fetcher

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [41438]

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

- Mesos ReviewBot


On Dec. 16, 2015, 8:27 a.m., Shuai Lin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41438/
> ---
> 
> (Updated Dec. 16, 2015, 8:27 a.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Bugs: MESOS-4130
> https://issues.apache.org/jira/browse/MESOS-4130
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation on using network proxy for mesos fetcher
> 
> 
> Diffs
> -
> 
>   docs/fetcher.md ec7372293491ed9ee4e20dda3c2def72a77a3f84 
> 
> Diff: https://reviews.apache.org/r/41438/diff/
> 
> 
> Testing
> ---
> 
> - Set `https_proxy=http://localhost:3128` (a local squid server) in 
> `/etc/default/mesos-slave`, restart mesos-slave, launch a new marathon task 
> with a `https://` uri, and through the following sysdig command I can see 
> there is data exchanging between mesos-fetcher process and port 3128.
> 
> ```
> sudo sysdig -A -c echo_fds proc.name=mesos-fetcher and fd.port=3128
> ```
> 
> - Stop the local squid server, try to restart the marathon task, the task 
> would fail repeatly, from slave logs there are error messages that fetcher 
> failed to fetch the uri.
> ```
> Dec 16 15:16:35 lin-E400 mesos-slave[24247]: E1216 15:16:35.678032 24283 
> slave.cpp:3342] Container '45c14132-c56a-4cff-a6b5-f57ba2670643' for executor 
> 'testapp_web.f0ef72d2-a3c4-11e5-af60-56847afe9799' of framework 
> 'b52179fd-8968-4
> bf8-baf0-dddc8a38c903-' failed to start: Failed to fetch all URIs for 
> container '45c14132-c56a-4cff-a6b5-f57ba2670643' with exit status: 2
> ```
> 
> - Restart the squid server, the task would start without a problem.
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>

Re: Review Request 41061: Logger Module: Add container_logger flag to the agent.

[Joseph Wu]

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

(Updated Dec. 17, 2015, 11:32 a.m.)


Review request for mesos, Benjamin Hindman and Artem Harutyunyan.


Changes
---

Update description.


Summary (updated)
-

Logger Module: Add container_logger flag to the agent.


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


Repository: mesos


Description (updated)
---

`--container_logger` is the container logger module the agent should use.  If 
unspecified, a `SandboxContainerLogger` is used.


Diffs
-

  src/slave/flags.hpp 1250786d1bdc8b312a1912a37ac8aac373dd5ec9 
  src/slave/flags.cpp c4343ab7ff7b7b4d2c335119d41319b0779d2806 

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


Testing
---

make


Thanks,

Joseph Wu

Re: Review Request 41423: Exposed partial contents of ContainerInfo through the state endpoint.

[Artem Harutyunyan]

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

(Updated Dec. 17, 2015, 11:58 a.m.)


Review request for mesos, Artem Harutyunyan and Jie Yu.


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


Repository: mesos


Description
---

Exposed partial contents of ContainerInfo through the state endpoint.


Diffs
-

  src/common/http.cpp 5198650ba6dc1ea3dab7912e5ef6e375bf9acc96 
  src/tests/containerizer/docker_containerizer_tests.cpp 
3f199e622dd337cdbf32d4368f4ead424c39823c 

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


Testing
---

make check


Thanks,

Artem Harutyunyan

Re: Review Request 41111: Logger Module: Add test for default executor/task stdout/stderr logging behavior (to sandbox).

[Joseph Wu]

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

(Updated Dec. 17, 2015, 12:14 p.m.)


Review request for mesos, Benjamin Hindman and Artem Harutyunyan.


Changes
---

Renamed some test variables.


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


Repository: mesos


Description
---

`ContainerLoggerTest.DefaultToSandbox` is a regression test which checks the 
existing logging behavior, prior to introducing/using the `ContainerLogger` 
module.  As of this patch, the `ContainerLogger` is not actually being tested.


Diffs (updated)
-

  src/Makefile.am e6d48dc16135b5d147d036e851422686eff7d5ef 
  src/tests/container_logger_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Joseph Wu

Re: Review Request 41003: Logger Module: Add the SandboxContainerLogger, the default ContainerLogger implementation.

[Joseph Wu]

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

(Updated Dec. 17, 2015, 12:14 p.m.)


Review request for mesos, Benjamin Hindman, Cody Maloney, and Artem Harutyunyan.


Changes
---

Comments + tweaked one line of code.


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


Repository: mesos


Description
---

This implementation mirrors how executor/task stdout/stderr is currently saved 
to plain files.


Diffs (updated)
-

  src/slave/container_loggers/sandbox.hpp PRE-CREATION 
  src/slave/container_loggers/sandbox.cpp PRE-CREATION 

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


Testing
---

This is added to the Makefile later in the review chain.


Thanks,

Joseph Wu

Re: Review Request 41004: Logger Module: Introduce the ContainerLogger module.

[Joseph Wu]

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

(Updated Dec. 17, 2015, 12:14 p.m.)


Review request for mesos, Benjamin Hindman, Cody Maloney, and Artem Harutyunyan.


Changes
---

Lambda-ified a test module initializer.  Renamed a local variable.


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


Repository: mesos


Description
---

Modularizes the `ContainerLogger` interface and adds the 
`SandboxContainerLogger` as the default.


Diffs (updated)
-

  include/mesos/module/container_logger.hpp PRE-CREATION 
  src/Makefile.am e6d48dc16135b5d147d036e851422686eff7d5ef 
  src/examples/test_container_logger_module.cpp PRE-CREATION 
  src/module/manager.cpp 1f04790510a2ab9ccd6907fd01be192f52ee90c6 
  src/slave/container_logger.cpp PRE-CREATION 
  src/tests/module.hpp e46ed12c80707bf44ceef3ed1a4eb2f321ce10f6 
  src/tests/module.cpp e272bf0eccb61ae54440ec79adac8efad804c828 

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


Testing
---

make

Tests are modified and run later in the review chain.


Thanks,

Joseph Wu

Re: Review Request 41003: Logger Module: Add the SandboxContainerLogger, the default ContainerLogger implementation.

[Joseph Wu]

> On Dec. 16, 2015, 5:23 p.m., Benjamin Hindman wrote:
> > src/slave/container_loggers/sandbox.cpp, line 55
> > 
> >
> > Is this comment useful? Is there something else here you want to 
> > capture?

Removed this comment in favor of a better header-file comment.


> On Dec. 16, 2015, 5:23 p.m., Benjamin Hindman wrote:
> > src/slave/container_loggers/sandbox.cpp, line 72
> > 
> >
> > Can we put a newline before this? Or:
> > 
> > info.environment = environment.get({});

Changed to `getOrElse`.


> On Dec. 16, 2015, 5:23 p.m., Benjamin Hindman wrote:
> > src/slave/container_loggers/sandbox.hpp, line 44
> > 
> >
> > Can we leave a comment of what this provides? And that it's the default?

Done!


- Joseph


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


On Dec. 17, 2015, 12:14 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41003/
> ---
> 
> (Updated Dec. 17, 2015, 12:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Cody Maloney, and Artem 
> Harutyunyan.
> 
> 
> Bugs: MESOS-4087
> https://issues.apache.org/jira/browse/MESOS-4087
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This implementation mirrors how executor/task stdout/stderr is currently 
> saved to plain files.
> 
> 
> Diffs
> -
> 
>   src/slave/container_loggers/sandbox.hpp PRE-CREATION 
>   src/slave/container_loggers/sandbox.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41003/diff/
> 
> 
> Testing
> ---
> 
> This is added to the Makefile later in the review chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

> On Dec. 18, 2015, 12:57 a.m., Adam B wrote:
> > src/tests/role_tests.cpp, lines 79-83
> > 
> >
> > `AWAIT_EXPECT_RESPONSE_HEADER_EQ(APPLICATION_JSON, "Content-Type", 
> > response);` all fits on one line, and does the AWAITY_READY(response) as 
> > well as the header EQ check.
> > You'll have to `include ` to get `APPLICATION_JSON`.

Fixed. To avoid the need to resolve some merge conflicts on rebase, I actually 
made this fix in https://reviews.apache.org/r/41225/ -- let me know if you'd 
rather I do the rebase + cleanup.


- Neil


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


On Dec. 16, 2015, 9:05 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 16, 2015, 9:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and 
> Yongqiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8f6b98b5b0ddbfe6b97346704cb96937e0eca02e 
>   src/tests/reservation_tests.cpp f429472e6b93a5d6d8fe6a5f7d5b94fc331f7295 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>