Re: Review Request 37669: Ignore overflow components in version parsing.

[Timothy Chen]

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

Ship it!


Ship It!

- Timothy Chen


On Aug. 22, 2015, 5:04 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37669/
> ---
> 
> (Updated Aug. 22, 2015, 5:04 p.m.)
> 
> 
> Review request for mesos, Isabel Jimenez and Timothy Chen.
> 
> 
> Bugs: MESOS-2986
> https://issues.apache.org/jira/browse/MESOS-2986
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignore overflow components in version parsing.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 
> 51010845e8885ac52ce88ec0deb6b65a81122cba 
>   3rdparty/libprocess/3rdparty/stout/tests/version_tests.cpp 
> e8f8358f3c113b4e21e30cb5e9d6a3d229191484 
> 
> Diff: https://reviews.apache.org/r/37669/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37669: Ignore overflow components in version parsing.

[haosdent huang]

> On Aug. 28, 2015, 7:32 p.m., Bernd Mathiske wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 59
> > 
> >
> > "allowed" and "would" are not quite right.
> > 
> > Suggested phrasing:
> > 
> > << "only " << maxComponents << " components will "
> > << "be recognized, the rest will be ignored."

Thank you. Let me update.


- haosdent


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


On Aug. 22, 2015, 5:04 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37669/
> ---
> 
> (Updated Aug. 22, 2015, 5:04 p.m.)
> 
> 
> Review request for mesos, Isabel Jimenez and Timothy Chen.
> 
> 
> Bugs: MESOS-2986
> https://issues.apache.org/jira/browse/MESOS-2986
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignore overflow components in version parsing.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 
> 51010845e8885ac52ce88ec0deb6b65a81122cba 
>   3rdparty/libprocess/3rdparty/stout/tests/version_tests.cpp 
> e8f8358f3c113b4e21e30cb5e9d6a3d229191484 
> 
> Diff: https://reviews.apache.org/r/37669/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37914: Updated the ReviewBot to flag reviews that do not contain reviewers.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37914]

All tests passed.

- Mesos ReviewBot


On Aug. 29, 2015, 12:08 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37914/
> ---
> 
> (Updated Aug. 29, 2015, 12:08 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> I'm seeing more and more reviews being sent without tagging reviewers. This 
> should flag such reviews to guide new contributors.
> 
> 
> Diffs
> -
> 
>   support/verify_reviews.py b85a3245b2f248dbdb8e2783f2384092e3d53031 
> 
> Diff: https://reviews.apache.org/r/37914/diff/
> 
> 
> Testing
> ---
> 
> Tested locally
> 
> ./support/verify_reviews.py vinod kone 10 "?from-user=neilc"
> git rev-parse HEAD
> Checking if review: 37445 needs verification
> Latest diff timestamp: 2015-08-13 21:01:08
> Verifying review 37445
> Posting review: Bad patch!
> 
> Reviews applied: []
> 
> Error:
>  No reviewers specified. Please find a reviewer by asking on JIRA or the 
> mailing list.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 37913: Improve allocator filtering by keeping per-slave filter sets.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37913]

All tests passed.

- Mesos ReviewBot


On Aug. 28, 2015, 11:46 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37913/
> ---
> 
> (Updated Aug. 28, 2015, 11:46 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3052
> https://issues.apache.org/jira/browse/MESOS-3052
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When frameworks refuse a lot of resources, the list of filters gets
> long. Since the filters are per-slave,
> HierarchicalAllocatorProcess::isFiltered spends a lot of time just
> comparing SlaveID (which tend to be long strings). Eliminate this
> whole problem by organizing the filters by SlaveID in the first
> place.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
> 
> Diff: https://reviews.apache.org/r/37913/diff/
> 
> 
> Testing
> ---
> 
> make check on CentOS 6 w/ devtoolset-3.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 37908: Silence oversubscription logging

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37908]

All tests passed.

- Mesos ReviewBot


On Aug. 28, 2015, 10:19 p.m., Niklas Nielsen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37908/
> ---
> 
> (Updated Aug. 28, 2015, 10:19 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Resource estimates are continuously written to the logs even though they are 
> empty (as with the majority of Mesos clusters at the moment). To reduce the 
> noise, this patch moves log lines for empty resources to VLOG(2).
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 8cfad7b86675b10c98524174975828bbcf47aed2 
> 
> Diff: https://reviews.apache.org/r/37908/diff/
> 
> 
> Testing
> ---
> 
> make check - with and without the patch, verifying the log statements.
> 
> 
> Thanks,
> 
> Niklas Nielsen
> 
>

Re: Review Request 37531: MESOS-3070

[Klaus Ma]

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

(Updated Aug. 29, 2015, 1:13 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Update the code diff after rebase


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


Repository: mesos


Description
---

MESOS-3070 (Master CHECK failure if a framework uses duplicated task id.)


Diffs (updated)
-

  src/master/http.cpp 37d76ee 
  src/master/master.hpp 36c6759 
  src/master/master.cpp 95207d2 
  src/tests/master_tests.cpp 8a6b98b 

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


Testing
---

make
make check


Thanks,

Klaus Ma

Re: Review Request 37114: MESOS-3187, support docker host command line option

[Vaibhav Khanduja]

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

(Updated Aug. 29, 2015, 1:11 a.m.)


Review request for mesos, Benjamin Hindman, Ben Mahler, Timothy Chen, and Vinod 
Kone.


Changes
---

Addresses comments and feedback


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


Repository: mesos


Description
---

MESOS-3187, support docker host command line option.

Docker daemon supports starting on a non-default port. Such scenarios would 
needed when starting Docker daemon on TCP or non-default unix port. Mesos slave 
does not work if Docker daemon is started on any of such non-default port. The 
code change is needed in Mesos slave to accept this parameter so as connect for 
its operations to the right Docker daemon. The change is made in Mesos slave, 
so as it is available to any framework making using Docker executor. 

The code is added to start slave binary with --docker_host, instructing it to 
connect on port as specified in the parameter. The default value of 
--default_host is "unix:///var/run/docker.sock, which is default port for 
Docker daemon.

The main class src/docker.cpp/.hpp is kept backward compartible to make Docker 
cli execute on default Docker port.


Diffs (updated)
-

  src/docker/docker.hpp 38e5299ad38b9e20501387f2193b0fa448e49e3e 
  src/docker/docker.cpp 1367de8a7bbbda6348a30e4ef4c616378e450250 
  src/docker/executor.hpp fa13b6e9905051eef27d3a51b75a5c86fdad0dd7 
  src/docker/executor.cpp 256d53d59d5cda63bbeb8c987ce0019e24b9fb77 
  src/slave/containerizer/docker.cpp 8f5d302477b216df9ac2f59156304bbc4a96f24b 
  src/tests/containerizer/docker_containerizer_tests.cpp 
80ed60e2b0fa39e8302867a7cb6a7388c25f9a40 
  src/tests/containerizer/docker_tests.cpp 
a4a2725c05ae0cb88426c587f7ded0da77154edc 
  src/tests/environment.cpp 525347090f38b61f2085a2b2a6002d28d11b222f 
  src/tests/flags.hpp 364495695c5915e54257014aeebb1e212d3da6fc 

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


Testing
---

Following scenarios were executed to test the code changes. Kindly suggest if 
more test-cases are required:


a) Mesos slave with unix port : unix:///var/run/docker_myport.sock
 
   i) Start slave with --docker_host parameter 
"unix:///var/run/docker_myport.sock
   ii) Using a framework, in my case Marathon, post a Docker job
   iii) The docker job does get started on the slave, confirmed with docker ps 
command output 
   
docker -H unix:///var/run/docker_myport.sock ps

CONTAINER IDIMAGE   COMMANDCREATED  
   STATUS  PORTS   NAMES
07fc4ec86bacmygoserver  "/bin/sh -c /mygoser   19 minutes ago   
   Up 19 minutes   */tcp, */udp
mesos-20150731-104052-1051068938-5050-7913-S33.17b355cd-2754-4fb2-a558-66820dff033c

iv) Stop or destroy the job from Marathon GUI

b) Two mesos slave with non-default docker port
i) On two different hosts, start slave, with one running on default port 
and other non-default. The start slaves with attributes - default and or 
non-default.
ii) Give jobs to these slaves, using Marathon UNIQUE attribute, selecting 
slave - non-default & default
iii) Stop/destroy the jobs

d) Modified unit test-case taking docker port value - make check


Thanks,

Vaibhav Khanduja

Re: Review Request 37541: Add TraceEvent API

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37540, 37541]

All tests passed.

- Mesos ReviewBot


On Aug. 28, 2015, 11:07 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37541/
> ---
> 
> (Updated Aug. 28, 2015, 11:07 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, and Jie Yu.
> 
> 
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Based on the PerfEvent API's, add API for Linux kernel trace events, 
> especially the schedule trace events.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.hpp b77b61d7048b12cea4586bcf802cbc2ff634331b 
>   src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 
>   src/tests/containerizer/cgroups_tests.cpp 
> 0b171eeb53037f26b7e952830e88e59f1278e7c6 
>   src/tests/containerizer/perf_tests.cpp 
> 6b3d70f3e7ea8f59f94e6961491d4e9a730e3334 
> 
> Diff: https://reviews.apache.org/r/37541/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Review Request 37914: Updated the ReviewBot to flag reviews that do not contain reviewers.

[Vinod Kone]

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

Review request for mesos, Benjamin Hindman and Michael Park.


Repository: mesos


Description
---

I'm seeing more and more reviews being sent without tagging reviewers. This 
should flag such reviews to guide new contributors.


Diffs
-

  support/verify_reviews.py b85a3245b2f248dbdb8e2783f2384092e3d53031 

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


Testing
---

Tested locally

./support/verify_reviews.py vinod kone 10 "?from-user=neilc"
git rev-parse HEAD
Checking if review: 37445 needs verification
Latest diff timestamp: 2015-08-13 21:01:08
Verifying review 37445
Posting review: Bad patch!

Reviews applied: []

Error:
 No reviewers specified. Please find a reviewer by asking on JIRA or the 
mailing list.


Thanks,

Vinod Kone

Re: Review Request 37364: Maintenance Primitives: Adds an endpoint for retrieving the maintenance status for machines.

[Joseph Wu]

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

(Updated Aug. 28, 2015, 5:04 p.m.)


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


Changes
---

Rebased and updated to match changes in previous reviews.


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


Repository: mesos


Description (updated)
---

Endpoint: /maintenance/status

Also adds a protobuf for the purpose of serialization.


Diffs (updated)
-

  include/mesos/maintenance/maintenance.proto PRE-CREATION 
  src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44 
  src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc 
  src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
  src/tests/master_maintenance_tests.cpp PRE-CREATION 

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


Testing
---

`make check`

New Tests:
  MasterMaintenanceTest.FailToUnscheduleDeactivatedMachines
Extra test case for the /maintenance.schedule endpoint, which requires all 
three endpoints to work.
  MasterMaintenanceTest.MachineStatus
Schedules, starts, and stops maintenance.  Checks machine statuses after 
each step.


Thanks,

Joseph Wu

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

[Jojy Varghese]

> On Aug. 28, 2015, 11:03 p.m., Lily Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 465
> > 
> >
> > Same issue with path naming as with getManifest

Please see https://docs.docker.com/registry/spec/api/. Its a path.


- Jojy


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


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

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

[Jojy Varghese]

> On Aug. 28, 2015, 6:46 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.hpp, line 150
> > 
> >
> > Do we need to define static here if it's not being used anywhere else 
> > but the cpp files?

Its a class constant and hence declared here.


> On Aug. 28, 2015, 6:46 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 87
> > 
> >
> > What's the reasoning behind prefixing underscores for these? You've 
> > already added trailing underscores for your class variables right?

The function body uses similar variables. Hence to disambiguate.


> On Aug. 28, 2015, 6:46 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 153
> > 
> >
> > What residue?

The new directory created.


- Jojy


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


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

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

[Jojy Varghese]

> On Aug. 28, 2015, 11:03 p.m., Lily Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 409
> > 
> >
> > I think path may be misleading, can we name this repo?

https://docs.docker.com/registry/spec/api/. If you look at the description of 
the field, its a path.


- Jojy


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


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

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

[Jojy Varghese]

> On Aug. 28, 2015, 11:03 p.m., Lily Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.cpp, line 415
> > 
> >
> > can we do a path::join here? It would be hard to get the "/"s correct 
> > just passing path through

This is not a file path. So os::path wont work.


- Jojy


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


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

Re: Review Request 37814: Added documentation for libprocess environment variables

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37814]

All tests passed.

- Mesos ReviewBot


On Aug. 28, 2015, 10:25 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37814/
> ---
> 
> (Updated Aug. 28, 2015, 10:25 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and haosdent huang.
> 
> 
> Bugs: MESOS-2466
> https://issues.apache.org/jira/browse/MESOS-2466
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for libprocess environment variables
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2b23d48c8841e43e6e2776dfe6bfa7c022a941a7 
> 
> Diff: https://reviews.apache.org/r/37814/diff/
> 
> 
> Testing
> ---
> 
> Built the updated website using the docker site builder 
> (https://github.com/mesosphere/mesos-website-container).
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

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

[Jojy Varghese]

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



src/slave/containerizer/provisioners/docker/registry_client.cpp (lines 468 - 
469)


The default has already been taken care of in the caller.


- Jojy Varghese


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

Re: Review Request 37907: Libevent: Used `evutil_gettimeofday` to implement `time()`.

[Vinod Kone]

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

Ship it!


Are there any libevent benchmark tests that we can validate this against?

- Vinod Kone


On Aug. 28, 2015, 9:54 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37907/
> ---
> 
> (Updated Aug. 28, 2015, 9:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Vinod Kone.
> 
> 
> Bugs: MESOS-3189
> https://issues.apache.org/jira/browse/MESOS-3189
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This used to be implemented by `event_base_gettimeofday_cached()`. We
> want to avoid cached results in case they introduce any issues.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent.cpp 
> d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5 
> 
> Diff: https://reviews.apache.org/r/37907/diff/
> 
> 
> Testing
> ---
> 
> `./tests --gtest_repeat=1 --gtest_filter="TimeTest.Now" 
> --gtest_throw_on_failure`
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 37913: Improve allocator filtering by keeping per-slave filter sets.

[James Peach]

> On Aug. 28, 2015, 11:44 p.m., Neil Conway wrote:
> > Diff seems weird -- maybe needs a rebase.

Thanks ... rebased again and it looks better now :)


- James


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


On Aug. 28, 2015, 11:46 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37913/
> ---
> 
> (Updated Aug. 28, 2015, 11:46 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3052
> https://issues.apache.org/jira/browse/MESOS-3052
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When frameworks refuse a lot of resources, the list of filters gets
> long. Since the filters are per-slave,
> HierarchicalAllocatorProcess::isFiltered spends a lot of time just
> comparing SlaveID (which tend to be long strings). Eliminate this
> whole problem by organizing the filters by SlaveID in the first
> place.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
> 
> Diff: https://reviews.apache.org/r/37913/diff/
> 
> 
> Testing
> ---
> 
> make check on CentOS 6 w/ devtoolset-3.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 37913: Improve allocator filtering by keeping per-slave filter sets.

[James Peach]

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

(Updated Aug. 28, 2015, 11:46 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Rebased onto master.


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


Repository: mesos


Description
---

When frameworks refuse a lot of resources, the list of filters gets
long. Since the filters are per-slave,
HierarchicalAllocatorProcess::isFiltered spends a lot of time just
comparing SlaveID (which tend to be long strings). Eliminate this
whole problem by organizing the filters by SlaveID in the first
place.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
38f8fd2c84314bb3731684d0e9795cb4f50a227e 

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


Testing
---

make check on CentOS 6 w/ devtoolset-3.


Thanks,

James Peach

Re: Review Request 37913: Improve allocator filtering by keeping per-slave filter sets.

[Neil Conway]

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


Diff seems weird -- maybe needs a rebase.

- Neil Conway


On Aug. 28, 2015, 11:38 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37913/
> ---
> 
> (Updated Aug. 28, 2015, 11:38 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3052
> https://issues.apache.org/jira/browse/MESOS-3052
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When frameworks refuse a lot of resources, the list of filters gets
> long. Since the filters are per-slave,
> HierarchicalAllocatorProcess::isFiltered spends a lot of time just
> comparing SlaveID (which tend to be long strings). Eliminate this
> whole problem by organizing the filters by SlaveID in the first
> place.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/CMakeLists.txt 
> 997cc0d0e316e316136d4746e50e9e292a82b36b 
>   3rdparty/libprocess/3rdparty/Makefile.am 
> eb34251d24b1e5d1540151b59cf1062ca85aeb03 
>   3rdparty/libprocess/3rdparty/gmock-1.6.0.tar.gz PRE-CREATION 
>   3rdparty/libprocess/3rdparty/gmock-1.7.0.tar.gz 
> 09f5ea3ce95fab34c505367ae04965e01c8bb30d 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> f95ed03004d4e5382874d75969bc9285a0f44918 
>   3rdparty/libprocess/3rdparty/stout/README.md 
> d7596e58e67699c1bc9c28da841b89ccd26f3a34 
>   3rdparty/libprocess/3rdparty/stout/cmake/StoutTestsConfigure.cmake 
> 08325297ceb79b80c305ba4f2164ffd37591a0e8 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 8853f92fcfcff81d0a3197bade02110685fa0325 
>   3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 
> 2b003d26d6b6b65b1d7b1dd6396f808c35b53177 
>   3rdparty/libprocess/3rdparty/stout/include/stout/attributes.hpp 
> 27087041d8255b96159bb10c184f00cf5bc9c34e 
>   3rdparty/libprocess/3rdparty/stout/include/stout/base64.hpp 
> 4893e7ba0b7d83fd3ba36bf18aa541c60293cc23 
>   3rdparty/libprocess/3rdparty/stout/include/stout/bytes.hpp 
> e9cf85637157f98a0eac166096bb18fa5652c669 
>   3rdparty/libprocess/3rdparty/stout/include/stout/cache.hpp 
> 0d51c8d418acb49b52cebfb644ee0476d6ec4502 
>   3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp 
> 90551541f59889e96b21dbe1b65d3904850464c2 
>   3rdparty/libprocess/3rdparty/stout/include/stout/exit.hpp 
> 38dabd45aa1d7f02e9991ce4ae28b44cd39db87c 
>   3rdparty/libprocess/3rdparty/stout/include/stout/fatal.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> 9da213f802aec6a7768ce6f5aea7b437d980356a 
>   3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 
> 1cf6dd18aa163688d6c8f3a6d33eacad3918015d 
>   3rdparty/libprocess/3rdparty/stout/include/stout/gzip.hpp 
> 85c773ac675c88b313dffda7a9c32bac42ebe62d 
>   3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp 
> f09bea125035aa3621402b83608b233e42877559 
>   3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp 
> 1839d28638cd82dae10ba9b0f99c1a97cf34f9c9 
>   3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp 
> d207dc5ad7558818da7fd0d04c6ef8df217b78c1 
>   3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
> 1ad119d54820e97497b1773518875be25ddbf98a 
>   3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 
> f28138c4682c41e94ab6c7641a78d66b2f9daa5f 
>   3rdparty/libprocess/3rdparty/stout/include/stout/mac.hpp 
> 9428717fac4655898d7768957f02937592e1a398 
>   3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp 
> d9e4031cee64e48ad50541c04ca11e7861d0a17c 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp 
> 3f829bafe96526bc2263c9f228f85de38c435f60 
>   3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 
> db5e33220844d20ef08a7324f641eeb1ff6d2052 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 5141c1369af60afd6cd5c70c6295d575ea960a83 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/close.hpp 
> 972f833835633ec343f97b3cde504772537c5272 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/exists.hpp 
> 6eb7f8f2be208e591d43088b2541d030a272f328 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/fcntl.hpp 
> 3728bc49477dff6203f255f87bc852beea86 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/fork.hpp 
> 8ea971fe72237423164adb0a4a10ddf1603d49cf 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp 
> de11bdadf3222fc955fd4d1870d1c406535dc1b9 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/linux.hpp 
> b994b13941628947c9d12b8baae155d5da1ec7bd 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/ls.hpp 
> e80d885725b3f51c6640e24abba1f37d556fb476 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/open.hpp 
> 

Review Request 37913: Improve allocator filtering by keeping per-slave filter sets.

[James Peach]

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

Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

When frameworks refuse a lot of resources, the list of filters gets
long. Since the filters are per-slave,
HierarchicalAllocatorProcess::isFiltered spends a lot of time just
comparing SlaveID (which tend to be long strings). Eliminate this
whole problem by organizing the filters by SlaveID in the first
place.


Diffs
-

  3rdparty/libprocess/3rdparty/CMakeLists.txt 
997cc0d0e316e316136d4746e50e9e292a82b36b 
  3rdparty/libprocess/3rdparty/Makefile.am 
eb34251d24b1e5d1540151b59cf1062ca85aeb03 
  3rdparty/libprocess/3rdparty/gmock-1.6.0.tar.gz PRE-CREATION 
  3rdparty/libprocess/3rdparty/gmock-1.7.0.tar.gz 
09f5ea3ce95fab34c505367ae04965e01c8bb30d 
  3rdparty/libprocess/3rdparty/stout/Makefile.am 
f95ed03004d4e5382874d75969bc9285a0f44918 
  3rdparty/libprocess/3rdparty/stout/README.md 
d7596e58e67699c1bc9c28da841b89ccd26f3a34 
  3rdparty/libprocess/3rdparty/stout/cmake/StoutTestsConfigure.cmake 
08325297ceb79b80c305ba4f2164ffd37591a0e8 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
8853f92fcfcff81d0a3197bade02110685fa0325 
  3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 
2b003d26d6b6b65b1d7b1dd6396f808c35b53177 
  3rdparty/libprocess/3rdparty/stout/include/stout/attributes.hpp 
27087041d8255b96159bb10c184f00cf5bc9c34e 
  3rdparty/libprocess/3rdparty/stout/include/stout/base64.hpp 
4893e7ba0b7d83fd3ba36bf18aa541c60293cc23 
  3rdparty/libprocess/3rdparty/stout/include/stout/bytes.hpp 
e9cf85637157f98a0eac166096bb18fa5652c669 
  3rdparty/libprocess/3rdparty/stout/include/stout/cache.hpp 
0d51c8d418acb49b52cebfb644ee0476d6ec4502 
  3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp 
90551541f59889e96b21dbe1b65d3904850464c2 
  3rdparty/libprocess/3rdparty/stout/include/stout/exit.hpp 
38dabd45aa1d7f02e9991ce4ae28b44cd39db87c 
  3rdparty/libprocess/3rdparty/stout/include/stout/fatal.hpp PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
9da213f802aec6a7768ce6f5aea7b437d980356a 
  3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 
1cf6dd18aa163688d6c8f3a6d33eacad3918015d 
  3rdparty/libprocess/3rdparty/stout/include/stout/gzip.hpp 
85c773ac675c88b313dffda7a9c32bac42ebe62d 
  3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp 
f09bea125035aa3621402b83608b233e42877559 
  3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp 
1839d28638cd82dae10ba9b0f99c1a97cf34f9c9 
  3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp 
d207dc5ad7558818da7fd0d04c6ef8df217b78c1 
  3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
1ad119d54820e97497b1773518875be25ddbf98a 
  3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 
f28138c4682c41e94ab6c7641a78d66b2f9daa5f 
  3rdparty/libprocess/3rdparty/stout/include/stout/mac.hpp 
9428717fac4655898d7768957f02937592e1a398 
  3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp 
d9e4031cee64e48ad50541c04ca11e7861d0a17c 
  3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp 
3f829bafe96526bc2263c9f228f85de38c435f60 
  3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 
db5e33220844d20ef08a7324f641eeb1ff6d2052 
  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
5141c1369af60afd6cd5c70c6295d575ea960a83 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/close.hpp 
972f833835633ec343f97b3cde504772537c5272 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/exists.hpp 
6eb7f8f2be208e591d43088b2541d030a272f328 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/fcntl.hpp 
3728bc49477dff6203f255f87bc852beea86 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/fork.hpp 
8ea971fe72237423164adb0a4a10ddf1603d49cf 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp 
de11bdadf3222fc955fd4d1870d1c406535dc1b9 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/linux.hpp 
b994b13941628947c9d12b8baae155d5da1ec7bd 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/ls.hpp 
e80d885725b3f51c6640e24abba1f37d556fb476 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/open.hpp 
43f261fd7a60b534f642f826ebf6ab18d72180c2 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/os.hpp 
82ccd766bd22393f48be4554c7da46338dd92d1f 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/permissions.hpp 
196c3f5fac7c3526924f2bea03c06d1fbce25c61 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/close.hpp 
d8679ca0720382795ca1617b777553f218ef1687 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/exists.hpp 
835466b78e1ea1bd81bcf732eaff861e858edd1c 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/fcntl.hpp 
a9cb07ecd16f363cf2f77bc867277c

Re: Review Request 37908: Silence oversubscription logging

[Niklas Nielsen]

> On Aug. 28, 2015, 3:42 p.m., Jie Yu wrote:
> > src/slave/slave.cpp, lines 4240-4248
> > 
> >
> > Hum, why zero is a special case here? What if the RE initially returns 
> > 10cpus, and due to some reaons, it returns nothing later. Do we still need 
> > to get notified about this change?
> > 
> > I would suggest changing LOG(INFO) to VLOG(1) without other changes.

Alrighty - will do; don't have any strong opinions other than getting it out of 
non-oversubscribed cluster logs :)


- Niklas


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


On Aug. 28, 2015, 3:19 p.m., Niklas Nielsen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37908/
> ---
> 
> (Updated Aug. 28, 2015, 3:19 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Resource estimates are continuously written to the logs even though they are 
> empty (as with the majority of Mesos clusters at the moment). To reduce the 
> noise, this patch moves log lines for empty resources to VLOG(2).
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 8cfad7b86675b10c98524174975828bbcf47aed2 
> 
> Diff: https://reviews.apache.org/r/37908/diff/
> 
> 
> Testing
> ---
> 
> make check - with and without the patch, verifying the log statements.
> 
> 
> Thanks,
> 
> Niklas Nielsen
> 
>

Re: Review Request 37362: Maintenance Primitives: Adds an endpoint for transitioning agents back into UP mode.

[Joseph Wu]

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

(Updated Aug. 28, 2015, 4:20 p.m.)


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


Changes
---

Rebased + changes from previous reviews.


Summary (updated)
-

Maintenance Primitives: Adds an endpoint for transitioning agents back into UP 
mode.


Bugs: MESOS-2067 and MESOS-3069
https://issues.apache.org/jira/browse/MESOS-2067
https://issues.apache.org/jira/browse/MESOS-3069


Repository: mesos


Description (updated)
---

Endpoint: /machine/up

Registry operation = maintenance::StopMaintenance
  Sets the list of machines back to UP mode.  Removes those machines from the 
maintenance schedule.


Diffs (updated)
-

  src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44 
  src/master/maintenance.hpp PRE-CREATION 
  src/master/maintenance.cpp PRE-CREATION 
  src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc 
  src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
  src/tests/master_maintenance_tests.cpp PRE-CREATION 
  src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166 

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


Testing (updated)
---

`make check`

New Tests:
  RegistrarTest.StopMaintenance
Schedules some machines.  Deactivates some.  Reactivates some.
  MasterMaintenanceTest.BringUpMachines
Tests some invalid lists.
Schedules some machines.  Deactivates some.  Tests some valid and invalid 
lists.
Checks that the schedule is modified.


Thanks,

Joseph Wu

Re: Review Request 37541: Add TraceEvent API

[Cong Wang]

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

(Updated Aug. 28, 2015, 11:07 p.m.)


Review request for mesos, Chi Zhang, Ian Downes, and Jie Yu.


Changes
---

Clean up


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


Repository: mesos


Description
---

Based on the PerfEvent API's, add API for Linux kernel trace events, especially 
the schedule trace events.


Diffs (updated)
-

  src/linux/perf.hpp b77b61d7048b12cea4586bcf802cbc2ff634331b 
  src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 
  src/tests/containerizer/cgroups_tests.cpp 
0b171eeb53037f26b7e952830e88e59f1278e7c6 
  src/tests/containerizer/perf_tests.cpp 
6b3d70f3e7ea8f59f94e6961491d4e9a730e3334 

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


Testing
---

make check


Thanks,

Cong Wang

Re: Review Request 37881: Implemented AppcProvisioner.

[Jiang Yan Xu]

> On Aug. 28, 2015, 12:59 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/appc.cpp, lines 79-97
> > 
> >
> > Some high level comments.
> > 
> > I think it will be beneficial if we can move the fetch and discovery 
> > logic to the Store. For the MVP, we don't have to impl. that in the Store.
> > 
> > The Store only has one interface, which is the 'get' method. 'get' 
> > takes an Image::Appc and returns a vector of paths to layers. THis 
> > semantics is more natural and easy to understand: get layers for my image, 
> > if some layers are already cached, return them directly, otherwise, 
> > discover it and fetch it.
> > 
> > The job of the provisioner is to get layers from the Store and stack 
> > them into a rootfs using the backend. It also manages the rootfs 
> > directories.
> > 
> > To me, this is a more natural split of functionalities among 
> > components. In the future, we can also potentially unify the impl. of the 
> > provisioner (i.e., one single provisioner for both Docker and Appc images). 
> > All the image specific details are hiden in the Store interface.
> > 
> > Thoughts?
> 
> Jiang Yan Xu wrote:
> I am OK the this direction but a few points:
> 
> 1) I think a "dumb" store that only returns what it has and stores what 
> is given to it is natual too. (So how about stripping the fetching logic from 
> the store and put it in the provisioner? This way the provisioner is the 
> "manager" that controls all the dumb single-purpose helpers.)
> 
> 2) Unifying the provisioner would be nice but one of the remaining 
> obstacles is that we need to make sure we have a common "provisioner 
> direcotry layout", otherwise they need to be further extracted out from the 
> provisioner and the provisioner becomes so thin that there is no point in 
> having a unified provisioner anymore. --> So I am not totally sure if this 
> will happen.
> 
> 3) In the future there may be a lot of more logic required for the new 
> Store if it is responsible for doing all the heavy-lifting: cache eviction, 
> P2P fetching, registering new provision requests with ongoing fetching 
> processes, etc. But I guess this is not an argument for keeping these in the 
> provisioner, we should create other single purpose class to handle them, we 
> just need to design a Store API that's takes these into consideration.
> 
> For now since none of these aformentioned functionality is necessary for 
> our "read-only, no dependency" provisioner let's find a way to check this as 
> long as it doesn't make it hard to refactor in the future.
> 
> Jie Yu wrote:
> Those are valid points! Let me express my rationale in some depth:
> 
> Store manages store_dir and privisioner manages rootfs_dir. A natural 
> split is that: any thing involved in updating 'store_dir' should be handled 
> by Store and anything involved in updating 'rootfs_dir' should be handled by 
> rootfs_dir.
> 
> In that sense, fetching and caching should be handled by the Store. As 
> you said, you can introduce sub-component in 'Store' to dealing with 
> Discovery, fetching, caching, etc.
> 
> Another reason I want fetching and caching to be done in Store, 
> independent of provisioner, is that: maybe, in the future, we can support 
> 'pre-fetching' which is not associated with any provisioning at all. Putting 
> all such logics in the provisioner will complicate it a lot. A component with 
> less dependency is definitely more managable.

OK I am going to make the recommended change beause I realized that this is 
basically the two patterns for caching:

1) Cache-aside: the cache returns none if it's not in it and the application 
reads the data and put it in the cache.
2) Read-through: the cache fetches the data transparently if it's not cached.

We cannot do 1) cleanly because given the size of the data on the disk we need 
the data to be downloaded directly into the cache and the application shouldn't 
need to know how the cache manages its data.

So we can just do 2).


- Jiang Yan


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


On Aug. 28, 2015, 2:04 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> ---
> 
> (Updated Aug. 28, 2015, 2:04 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-2796
> https://issues.apache.org/jira/browse/MESOS-2796
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented AppcProvisioner.
> 
> 
> Diffs
> -
> 
>   

Re: Review Request 37540: Add PerfEvent API

[Cong Wang]

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

(Updated Aug. 28, 2015, 11:06 p.m.)


Review request for mesos, Chi Zhang, Ian Downes, and Jie Yu.


Changes
---

Cleanup


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


Repository: mesos


Description
---

Abstract Linux kernel perf event API and provide API to collect schedule events.


Diffs (updated)
-

  src/linux/perf.hpp b77b61d7048b12cea4586bcf802cbc2ff634331b 
  src/linux/perf.cpp cdc5f8314a875ababf2b17a32873775d808e1c78 
  src/tests/containerizer/perf_tests.cpp 
6b3d70f3e7ea8f59f94e6961491d4e9a730e3334 

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


Testing
---

make check


Thanks,

Cong Wang

Re: Review Request 37114: MESOS-3187, support docker host command line option

[Timothy Chen]

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



src/docker/docker.hpp (line 45)


This doesn't need to be a public static method since it's only used in 
Docker::create.

Matter of fact just inline the prefixing in the create method, no need for 
a small prefix method.



src/docker/docker.hpp (line 161)


Alignment is off:

 Docker(const std::string& _path,
const std::string& _socket)
  : path(_path), socket(_socket) {};



src/docker/docker.hpp (line 242)


Get rid of these if you follow my comment.



src/docker/docker.cpp (line 57)


You can get rid of all these if you follow my comment below.



src/docker/docker.cpp (line 102)


So if you look at the definition of docker_socket, it's explicitly saying 
we need the file path to the docker_socket (e.g: /var/run/docker.sock)

I think we shouldn't even expect users passing in tcp:// or 
unix:///var/run/docker.sock as well.

What I would do instead here, is actually check the socket path is valid by 
just making sure it's a absolute path: 

if (!strings::startsWith(socket, "/")) {
  return Error("Invalid Docker socket path: " + socket);
}

And then just prefix unix:// before passing it to Docker constructor.



src/docker/docker.cpp (line 129)


Fix the style:

Try Docker::create(
const string& path,
const string& socket,
bool validate)


- Timothy Chen


On Aug. 26, 2015, 2:15 a.m., Vaibhav Khanduja wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37114/
> ---
> 
> (Updated Aug. 26, 2015, 2:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Timothy Chen, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-3187
> https://issues.apache.org/jira/browse/MESOS-3187
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3187, support docker host command line option.
> 
> Docker daemon supports starting on a non-default port. Such scenarios would 
> needed when starting Docker daemon on TCP or non-default unix port. Mesos 
> slave does not work if Docker daemon is started on any of such non-default 
> port. The code change is needed in Mesos slave to accept this parameter so as 
> connect for its operations to the right Docker daemon. The change is made in 
> Mesos slave, so as it is available to any framework making using Docker 
> executor. 
> 
> The code is added to start slave binary with --docker_host, instructing it to 
> connect on port as specified in the parameter. The default value of 
> --default_host is "unix:///var/run/docker.sock, which is default port for 
> Docker daemon.
> 
> The main class src/docker.cpp/.hpp is kept backward compartible to make 
> Docker cli execute on default Docker port.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 38e5299ad38b9e20501387f2193b0fa448e49e3e 
>   src/docker/docker.cpp 1367de8a7bbbda6348a30e4ef4c616378e450250 
>   src/docker/executor.hpp fa13b6e9905051eef27d3a51b75a5c86fdad0dd7 
>   src/docker/executor.cpp 256d53d59d5cda63bbeb8c987ce0019e24b9fb77 
>   src/slave/containerizer/docker.cpp 8f5d302477b216df9ac2f59156304bbc4a96f24b 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 80ed60e2b0fa39e8302867a7cb6a7388c25f9a40 
>   src/tests/containerizer/docker_tests.cpp 
> a4a2725c05ae0cb88426c587f7ded0da77154edc 
>   src/tests/environment.cpp 525347090f38b61f2085a2b2a6002d28d11b222f 
>   src/tests/flags.hpp 364495695c5915e54257014aeebb1e212d3da6fc 
> 
> Diff: https://reviews.apache.org/r/37114/diff/
> 
> 
> Testing
> ---
> 
> Following scenarios were executed to test the code changes. Kindly suggest if 
> more test-cases are required:
> 
> 
> a) Mesos slave with unix port : unix:///var/run/docker_myport.sock
>  
>i) Start slave with --docker_host parameter 
> "unix:///var/run/docker_myport.sock
>ii) Using a framework, in my case Marathon, post a Docker job
>iii) The docker job does get started on the slave, confirmed with docker 
> ps command output 
>
> docker -H unix:///var/run/docker_myport.sock ps
> 
> CONTAINER IDIMAGE   COMMANDCREATED
>  STATUS  PORTS   NAMES
> 07fc4ec86bacmygoserver  "/bin/sh -c /mygoser   19 minutes ago 
>  Up 19 minutes   */tcp, */udp
> mesos-20150731

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

[Lily Chen]

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



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 409)


I think path may be misleading, can we name this repo?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 415)


can we do a path::join here? It would be hard to get the "/"s correct just 
passing path through



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 465)


Same issue with path naming as with getManifest



src/slave/containerizer/provisioners/docker/registry_client.cpp (lines 468 - 
469)


Add defaults here as with as with image manifest.


- Lily Chen


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

Re: Review Request 37881: Implemented AppcProvisioner.

[Jie Yu]

> On Aug. 28, 2015, 7:59 p.m., Jie Yu wrote:
> > src/slave/flags.cpp, lines 72-76
> > 
> >
> > Why do you still need this flag?
> 
> Jiang Yan Xu wrote:
> We instantiate all supported backends but this flag specifies the user's 
> choice for new images. I don't think we can make a perfect guess based soley 
> on system compatibility: what if the user only has small images and don't 
> have the mount point created?
> 
> Jie Yu wrote:
> User does not care which backend we use, right? We just pick a backend 
> that works. If there are multiple supported backend, just pick the one that's 
> the best.
> 
> The flag is a little wiered to me because what if bind backend is not 
> supported but copy backend is supported, the user specifes the backend to be 
> bind, should we fail, or fall back to the copy backend? I think we can punt 
> on this flag for now since we only have one backend right now.
> 
> Jiang Yan Xu wrote:
> Sorry I meant the operator and the image in the default container info: 
> If the operator just downloads an Appc image from the interenet which doesn't 
> have the mountpoint for BindBackend but he does't have the choice to choose 
> CopyBackend instead (which we should add soon)..

OK, I think that's fine. Not a big deal.


- Jie


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


On Aug. 28, 2015, 9:04 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> ---
> 
> (Updated Aug. 28, 2015, 9:04 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-2796
> https://issues.apache.org/jira/browse/MESOS-2796
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented AppcProvisioner.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioner.cpp 
> efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/paths.hpp 
> 41e3bf79da0854406c488855f953111e67353829 
>   src/slave/containerizer/provisioners/appc/paths.cpp 
> 3113c84b9526dd9e9e89fb9aa4ec75ed66a996c7 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/appc_provisioner_tests.cpp 
> 47b66b9c30cefe8f9a8e2c1c1341776c2d235020 
> 
> Diff: https://reviews.apache.org/r/37881/diff/
> 
> 
> Testing
> ---
> 
> sudo make check.
> 
> More test cases coming.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 37358: Maintenance Primitives: Adds an endpoint for transitioning agents into the DOWN mode.

[Joseph Wu]

> On Aug. 27, 2015, 10:02 p.m., Benjamin Hindman wrote:
> > src/master/maintenance.cpp, lines 237-239
> > 
> >
> > It seems weird to make this an error ... why is it a prerequisite that 
> > validation of a MachineInfos requires more than one machine in the list? Is 
> > this somehow tied to your expectation in the code?

We have this check because a no-op doesn't make much sense, with regards to 
maintenance; and also to match how we don't allow empty Maintenance Windows 
(i.e. no machines, but with an unavailability).

It's easy to change this behavior though, so I'll leave this issue open for now.


- Joseph


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


On Aug. 28, 2015, 3:32 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37358/
> ---
> 
> (Updated Aug. 28, 2015, 3:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-2067 and MESOS-3069
> https://issues.apache.org/jira/browse/MESOS-2067
> https://issues.apache.org/jira/browse/MESOS-3069
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Endpoint: /machine/down
> 
> Registry operation = maintenance::StartMaintenance
>   Sets the list of machines as Down.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44 
>   src/master/maintenance.hpp PRE-CREATION 
>   src/master/maintenance.cpp PRE-CREATION 
>   src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc 
>   src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
>   src/tests/master_maintenance_tests.cpp PRE-CREATION 
>   src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166 
> 
> Diff: https://reviews.apache.org/r/37358/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> New Tests:
>   RegistrarTest.StartMaintenance
> Schedules some machines.  Downs some.
>   MasterMaintenanceTest.BringDownMachines
> Tests some invalid lists.
> Schedules some machines.  Tests some valid and invalid lists.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 37907: Libevent: Used `evutil_gettimeofday` to implement `time()`.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37907]

All tests passed.

- Mesos ReviewBot


On Aug. 28, 2015, 9:54 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37907/
> ---
> 
> (Updated Aug. 28, 2015, 9:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Vinod Kone.
> 
> 
> Bugs: MESOS-3189
> https://issues.apache.org/jira/browse/MESOS-3189
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This used to be implemented by `event_base_gettimeofday_cached()`. We
> want to avoid cached results in case they introduce any issues.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent.cpp 
> d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5 
> 
> Diff: https://reviews.apache.org/r/37907/diff/
> 
> 
> Testing
> ---
> 
> `./tests --gtest_repeat=1 --gtest_filter="TimeTest.Now" 
> --gtest_throw_on_failure`
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 37908: Silence oversubscription logging

[Jie Yu]

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



src/slave/slave.cpp (line 4226)


I think VLOG(1) is more appropriate.



src/slave/slave.cpp (lines 4240 - 4248)


Hum, why zero is a special case here? What if the RE initially returns 
10cpus, and due to some reaons, it returns nothing later. Do we still need to 
get notified about this change?

I would suggest changing LOG(INFO) to VLOG(1) without other changes.


- Jie Yu


On Aug. 28, 2015, 10:19 p.m., Niklas Nielsen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37908/
> ---
> 
> (Updated Aug. 28, 2015, 10:19 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Resource estimates are continuously written to the logs even though they are 
> empty (as with the majority of Mesos clusters at the moment). To reduce the 
> noise, this patch moves log lines for empty resources to VLOG(2).
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 8cfad7b86675b10c98524174975828bbcf47aed2 
> 
> Diff: https://reviews.apache.org/r/37908/diff/
> 
> 
> Testing
> ---
> 
> make check - with and without the patch, verifying the log statements.
> 
> 
> Thanks,
> 
> Niklas Nielsen
> 
>

Re: Review Request 37881: Implemented AppcProvisioner.

[Jiang Yan Xu]

> On Aug. 28, 2015, 12:59 p.m., Jie Yu wrote:
> > src/slave/flags.cpp, lines 72-76
> > 
> >
> > Why do you still need this flag?
> 
> Jiang Yan Xu wrote:
> We instantiate all supported backends but this flag specifies the user's 
> choice for new images. I don't think we can make a perfect guess based soley 
> on system compatibility: what if the user only has small images and don't 
> have the mount point created?
> 
> Jie Yu wrote:
> User does not care which backend we use, right? We just pick a backend 
> that works. If there are multiple supported backend, just pick the one that's 
> the best.
> 
> The flag is a little wiered to me because what if bind backend is not 
> supported but copy backend is supported, the user specifes the backend to be 
> bind, should we fail, or fall back to the copy backend? I think we can punt 
> on this flag for now since we only have one backend right now.

Sorry I meant the operator and the image in the default container info: If the 
operator just downloads an Appc image from the interenet which doesn't have the 
mountpoint for BindBackend but he does't have the choice to choose CopyBackend 
instead (which we should add soon)..


- Jiang Yan


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


On Aug. 28, 2015, 2:04 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> ---
> 
> (Updated Aug. 28, 2015, 2:04 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-2796
> https://issues.apache.org/jira/browse/MESOS-2796
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented AppcProvisioner.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioner.cpp 
> efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/paths.hpp 
> 41e3bf79da0854406c488855f953111e67353829 
>   src/slave/containerizer/provisioners/appc/paths.cpp 
> 3113c84b9526dd9e9e89fb9aa4ec75ed66a996c7 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/appc_provisioner_tests.cpp 
> 47b66b9c30cefe8f9a8e2c1c1341776c2d235020 
> 
> Diff: https://reviews.apache.org/r/37881/diff/
> 
> 
> Testing
> ---
> 
> sudo make check.
> 
> More test cases coming.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 37358: Maintenance Primitives: Adds an endpoint for transitioning agents into the DOWN mode.

[Joseph Wu]

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

(Updated Aug. 28, 2015, 3:32 p.m.)


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


Changes
---

Rebase + changes to match previous reviews in chain.
Renamed endpoint to match the renamed `DOWN` mode.


Summary (updated)
-

Maintenance Primitives: Adds an endpoint for transitioning agents into the DOWN 
mode.


Bugs: MESOS-2067 and MESOS-3069
https://issues.apache.org/jira/browse/MESOS-2067
https://issues.apache.org/jira/browse/MESOS-3069


Repository: mesos


Description (updated)
---

Endpoint: /machine/down

Registry operation = maintenance::StartMaintenance
  Sets the list of machines as Down.


Diffs (updated)
-

  src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44 
  src/master/maintenance.hpp PRE-CREATION 
  src/master/maintenance.cpp PRE-CREATION 
  src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc 
  src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
  src/tests/master_maintenance_tests.cpp PRE-CREATION 
  src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166 

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


Testing (updated)
---

`make check`

New Tests:
  RegistrarTest.StartMaintenance
Schedules some machines.  Downs some.
  MasterMaintenanceTest.BringDownMachines
Tests some invalid lists.
Schedules some machines.  Tests some valid and invalid lists.


Thanks,

Joseph Wu

Re: Review Request 37814: Added documentation for libprocess environment variables

[Greg Mann]

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

(Updated Aug. 28, 2015, 10:25 p.m.)


Review request for mesos, Alexander Rojas and haosdent huang.


Changes
---

Added missing  tag.


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


Repository: mesos


Description
---

Added documentation for libprocess environment variables


Diffs (updated)
-

  docs/configuration.md 2b23d48c8841e43e6e2776dfe6bfa7c022a941a7 

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


Testing (updated)
---

Built the updated website using the docker site builder 
(https://github.com/mesosphere/mesos-website-container).


Thanks,

Greg Mann

Re: Review Request 37881: Implemented AppcProvisioner.

[Jie Yu]

> On Aug. 28, 2015, 7:59 p.m., Jie Yu wrote:
> > src/slave/flags.cpp, lines 72-76
> > 
> >
> > Why do you still need this flag?
> 
> Jiang Yan Xu wrote:
> We instantiate all supported backends but this flag specifies the user's 
> choice for new images. I don't think we can make a perfect guess based soley 
> on system compatibility: what if the user only has small images and don't 
> have the mount point created?

User does not care which backend we use, right? We just pick a backend that 
works. If there are multiple supported backend, just pick the one that's the 
best.

The flag is a little wiered to me because what if bind backend is not supported 
but copy backend is supported, the user specifes the backend to be bind, should 
we fail, or fall back to the copy backend? I think we can punt on this flag for 
now since we only have one backend right now.


- Jie


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


On Aug. 28, 2015, 9:04 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> ---
> 
> (Updated Aug. 28, 2015, 9:04 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-2796
> https://issues.apache.org/jira/browse/MESOS-2796
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented AppcProvisioner.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioner.cpp 
> efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/paths.hpp 
> 41e3bf79da0854406c488855f953111e67353829 
>   src/slave/containerizer/provisioners/appc/paths.cpp 
> 3113c84b9526dd9e9e89fb9aa4ec75ed66a996c7 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/appc_provisioner_tests.cpp 
> 47b66b9c30cefe8f9a8e2c1c1341776c2d235020 
> 
> Diff: https://reviews.apache.org/r/37881/diff/
> 
> 
> Testing
> ---
> 
> sudo make check.
> 
> More test cases coming.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Review Request 37908: Silence oversubscription logging

[Niklas Nielsen]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Resource estimates are continuously written to the logs even though they are 
empty (as with the majority of Mesos clusters at the moment). To reduce the 
noise, this patch moves log lines for empty resources to VLOG(2).


Diffs
-

  src/slave/slave.cpp 8cfad7b86675b10c98524174975828bbcf47aed2 

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


Testing
---

make check - with and without the patch, verifying the log statements.


Thanks,

Niklas Nielsen

Re: Review Request 37881: Implemented AppcProvisioner.

[Jie Yu]

> On Aug. 28, 2015, 7:59 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/appc.cpp, lines 79-97
> > 
> >
> > Some high level comments.
> > 
> > I think it will be beneficial if we can move the fetch and discovery 
> > logic to the Store. For the MVP, we don't have to impl. that in the Store.
> > 
> > The Store only has one interface, which is the 'get' method. 'get' 
> > takes an Image::Appc and returns a vector of paths to layers. THis 
> > semantics is more natural and easy to understand: get layers for my image, 
> > if some layers are already cached, return them directly, otherwise, 
> > discover it and fetch it.
> > 
> > The job of the provisioner is to get layers from the Store and stack 
> > them into a rootfs using the backend. It also manages the rootfs 
> > directories.
> > 
> > To me, this is a more natural split of functionalities among 
> > components. In the future, we can also potentially unify the impl. of the 
> > provisioner (i.e., one single provisioner for both Docker and Appc images). 
> > All the image specific details are hiden in the Store interface.
> > 
> > Thoughts?
> 
> Jiang Yan Xu wrote:
> I am OK the this direction but a few points:
> 
> 1) I think a "dumb" store that only returns what it has and stores what 
> is given to it is natual too. (So how about stripping the fetching logic from 
> the store and put it in the provisioner? This way the provisioner is the 
> "manager" that controls all the dumb single-purpose helpers.)
> 
> 2) Unifying the provisioner would be nice but one of the remaining 
> obstacles is that we need to make sure we have a common "provisioner 
> direcotry layout", otherwise they need to be further extracted out from the 
> provisioner and the provisioner becomes so thin that there is no point in 
> having a unified provisioner anymore. --> So I am not totally sure if this 
> will happen.
> 
> 3) In the future there may be a lot of more logic required for the new 
> Store if it is responsible for doing all the heavy-lifting: cache eviction, 
> P2P fetching, registering new provision requests with ongoing fetching 
> processes, etc. But I guess this is not an argument for keeping these in the 
> provisioner, we should create other single purpose class to handle them, we 
> just need to design a Store API that's takes these into consideration.
> 
> For now since none of these aformentioned functionality is necessary for 
> our "read-only, no dependency" provisioner let's find a way to check this as 
> long as it doesn't make it hard to refactor in the future.

Those are valid points! Let me express my rationale in some depth:

Store manages store_dir and privisioner manages rootfs_dir. A natural split is 
that: any thing involved in updating 'store_dir' should be handled by Store and 
anything involved in updating 'rootfs_dir' should be handled by rootfs_dir.

In that sense, fetching and caching should be handled by the Store. As you 
said, you can introduce sub-component in 'Store' to dealing with Discovery, 
fetching, caching, etc.

Another reason I want fetching and caching to be done in Store, independent of 
provisioner, is that: maybe, in the future, we can support 'pre-fetching' which 
is not associated with any provisioning at all. Putting all such logics in the 
provisioner will complicate it a lot. A component with less dependency is 
definitely more managable.


- Jie


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


On Aug. 28, 2015, 9:04 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> ---
> 
> (Updated Aug. 28, 2015, 9:04 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-2796
> https://issues.apache.org/jira/browse/MESOS-2796
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented AppcProvisioner.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioner.cpp 
> efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/paths.hpp 
> 41e3bf79da0854406c488855f953111e67353829 
>   src/slave/containerizer/provisioners/appc/paths.cpp 
> 3113c84b9526dd9e9e89fb9aa4ec75ed66a996c7 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containe

Review Request 37907: Libevent: Used `evutil_gettimeofday` to implement `time()`.

[Joris Van Remoortere]

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

Review request for mesos, Benjamin Hindman and Vinod Kone.


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


Repository: mesos


Description
---

This used to be implemented by `event_base_gettimeofday_cached()`. We
want to avoid cached results in case they introduce any issues.


Diffs
-

  3rdparty/libprocess/src/libevent.cpp d7c47fbd1dbdec1fc974840e6f3a1428a8f189d5 

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


Testing
---

`./tests --gtest_repeat=1 --gtest_filter="TimeTest.Now" 
--gtest_throw_on_failure`


Thanks,

Joris Van Remoortere

Re: Review Request 37881: Implemented AppcProvisioner.

[Jiang Yan Xu]

> On Aug. 28, 2015, 12:59 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioner.cpp, lines 43-46
> > 
> >
> > I would love to get this TODO solved in this patch. It should be pretty 
> > straightfoward, right? Just hard code them. And right now, only Appc is 
> > supported.

Fine with me.


> On Aug. 28, 2015, 12:59 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/appc.cpp, lines 79-97
> > 
> >
> > Some high level comments.
> > 
> > I think it will be beneficial if we can move the fetch and discovery 
> > logic to the Store. For the MVP, we don't have to impl. that in the Store.
> > 
> > The Store only has one interface, which is the 'get' method. 'get' 
> > takes an Image::Appc and returns a vector of paths to layers. THis 
> > semantics is more natural and easy to understand: get layers for my image, 
> > if some layers are already cached, return them directly, otherwise, 
> > discover it and fetch it.
> > 
> > The job of the provisioner is to get layers from the Store and stack 
> > them into a rootfs using the backend. It also manages the rootfs 
> > directories.
> > 
> > To me, this is a more natural split of functionalities among 
> > components. In the future, we can also potentially unify the impl. of the 
> > provisioner (i.e., one single provisioner for both Docker and Appc images). 
> > All the image specific details are hiden in the Store interface.
> > 
> > Thoughts?

I am OK the this direction but a few points:

1) I think a "dumb" store that only returns what it has and stores what is 
given to it is natual too. (So how about stripping the fetching logic from the 
store and put it in the provisioner? This way the provisioner is the "manager" 
that controls all the dumb single-purpose helpers.)

2) Unifying the provisioner would be nice but one of the remaining obstacles is 
that we need to make sure we have a common "provisioner direcotry layout", 
otherwise they need to be further extracted out from the provisioner and the 
provisioner becomes so thin that there is no point in having a unified 
provisioner anymore. --> So I am not totally sure if this will happen.

3) In the future there may be a lot of more logic required for the new Store if 
it is responsible for doing all the heavy-lifting: cache eviction, P2P 
fetching, registering new provision requests with ongoing fetching processes, 
etc. But I guess this is not an argument for keeping these in the provisioner, 
we should create other single purpose class to handle them, we just need to 
design a Store API that's takes these into consideration.

For now since none of these aformentioned functionality is necessary for our 
"read-only, no dependency" provisioner let's find a way to check this as long 
as it doesn't make it hard to refactor in the future.


> On Aug. 28, 2015, 12:59 p.m., Jie Yu wrote:
> > src/slave/flags.cpp, lines 72-76
> > 
> >
> > Why do you still need this flag?

We instantiate all supported backends but this flag specifies the user's choice 
for new images. I don't think we can make a perfect guess based soley on system 
compatibility: what if the user only has small images and don't have the mount 
point created?


> On Aug. 28, 2015, 12:59 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioner.cpp, line 27
> > 
> >
> > See my comments below. This is no longer needed.

OK.


- Jiang Yan


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


On Aug. 28, 2015, 2:04 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> ---
> 
> (Updated Aug. 28, 2015, 2:04 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-2796
> https://issues.apache.org/jira/browse/MESOS-2796
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented AppcProvisioner.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioner.cpp 
> efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/paths.hpp 
> 41e3bf79da0854406c488855f953111e67353829 
>   src/slave/containerizer/provisioners/appc/paths.cpp 
> 3113c84b9526dd9e9e89fb9aa4ec75ed66

Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.

[Joseph Wu]

> On Aug. 26, 2015, 11:05 p.m., Alexander Rukletsov wrote:
> > Not a full review, mostly nit-picks. Will do a second pass.

Awesome.  Thanks!


- Joseph


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


On Aug. 28, 2015, 2:37 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37325/
> ---
> 
> (Updated Aug. 28, 2015, 2:37 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-2067 and MESOS-3069
> https://issues.apache.org/jira/browse/MESOS-2067
> https://issues.apache.org/jira/browse/MESOS-3069
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Endpoint: /maintenance.schedule
> 
> Registry operation = maintenance::UpdateSchedule
>   Replaces the schedule with the given one.  Also sets all scheduled machines 
> into Draining mode.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44 
>   src/master/maintenance.hpp PRE-CREATION 
>   src/master/maintenance.cpp PRE-CREATION 
>   src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc 
>   src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
>   src/tests/master_maintenance_tests.cpp PRE-CREATION 
>   src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166 
> 
> Diff: https://reviews.apache.org/r/37325/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> New Tests:
>   RegistrarTest.UpdateMaintenanceSchedule
> Schedules 3 machines, 1 at a time.  Rearranges schedules.
> Checks that machines are put into Draining mode.  Removes machines.
>   MasterMaintenanceTest.UpdateSchedule
> Hits the new endpoint with some valid and invalid schedules.
> Only tests a subset of invalid schedules (requires other endpoints to 
> fully test).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.

[Joseph Wu]

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

(Updated Aug. 28, 2015, 2:37 p.m.)


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


Changes
---

Significant reworking to reflect earlier commits:
 * Some code was split into an earlier review.
 * Plenty of renaming and tweaking to match protobuf changes.
 * Commenting changes.
 * Testing helpers moved into main test file.


Bugs: MESOS-2067 and MESOS-3069
https://issues.apache.org/jira/browse/MESOS-2067
https://issues.apache.org/jira/browse/MESOS-3069


Repository: mesos


Description (updated)
---

Endpoint: /maintenance.schedule

Registry operation = maintenance::UpdateSchedule
  Replaces the schedule with the given one.  Also sets all scheduled machines 
into Draining mode.


Diffs (updated)
-

  src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
  src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44 
  src/master/maintenance.hpp PRE-CREATION 
  src/master/maintenance.cpp PRE-CREATION 
  src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc 
  src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
  src/tests/master_maintenance_tests.cpp PRE-CREATION 
  src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166 

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


Testing
---

`make check`

New Tests:
  RegistrarTest.UpdateMaintenanceSchedule
Schedules 3 machines, 1 at a time.  Rearranges schedules.
Checks that machines are put into Draining mode.  Removes machines.
  MasterMaintenanceTest.UpdateSchedule
Hits the new endpoint with some valid and invalid schedules.
Only tests a subset of invalid schedules (requires other endpoints to fully 
test).


Thanks,

Joseph Wu

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

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37903]

All tests passed.

- Mesos ReviewBot


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

Re: Review Request 37900: Maintenance Primitives: Add helper functions for the maintenance protobufs.

[Joseph Wu]

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

(Updated Aug. 28, 2015, 2:07 p.m.)


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


Changes
---

Addressed issued raised by comments.


Repository: mesos


Description
---

This includes helpers for constructing (used in tests) and for validation (used 
in HTTP endpoints).


Diffs (updated)
-

  src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
  src/common/protobuf_utils.hpp 63eeb77da5e6020085b9927ce15338aa6ed00488 
  src/common/protobuf_utils.cpp 6b283558b5ef5c16f1d9f3dc2639df4f75c92f7d 
  src/master/maintenance.hpp PRE-CREATION 
  src/master/maintenance.cpp PRE-CREATION 
  src/master/registrar.hpp c6a0655c212646618d93c9c85918af482a9ffd50 

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


Testing
---

`make check`


Thanks,

Joseph Wu

Re: Review Request 37621: Maintenance Primitives: Gracefully handle inverse offers in pre-V1 scheduler.

[Benjamin Hindman]

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

Ship it!



src/sched/sched.cpp (line 525)


Let's just simplify this and only call 'resourceOffers' if 'offers.size() 
!= 0'.


- Benjamin Hindman


On Aug. 26, 2015, 2:13 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37621/
> ---
> 
> (Updated Aug. 26, 2015, 2:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp 012af0508eeceeccd168b29f36fa258d20b28c21 
>   src/tests/master_maintenance_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37621/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 37284: Maintenance Primitives: Added support for Accept / Decline of InverseOffers in master.

[Benjamin Hindman]

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

Ship it!


Ship It!

- Benjamin Hindman


On Aug. 26, 2015, 2:13 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37284/
> ---
> 
> (Updated Aug. 26, 2015, 2:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
> 
> Diff: https://reviews.apache.org/r/37284/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 37283: Maintenance Primitives: Refactored Master maintenance test to use V1 API.

[Benjamin Hindman]

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

Ship it!



src/tests/master_maintenance_tests.cpp (line 82)


{ on newline please.



src/tests/master_maintenance_tests.cpp (line 97)


Can we do this ASAP? Either now or already as an independent review on this 
chain so that we make sure we follow up?


- Benjamin Hindman


On Aug. 26, 2015, 2:13 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37283/
> ---
> 
> (Updated Aug. 26, 2015, 2:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37283/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 37234: Maintenance Primitives: Added URL field to InverseOffer proto.

[Benjamin Hindman]

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

Ship it!


Ship It!

- Benjamin Hindman


On Aug. 26, 2015, 2:13 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37234/
> ---
> 
> (Updated Aug. 26, 2015, 2:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
> 
> Diff: https://reviews.apache.org/r/37234/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

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

[Benjamin Hindman]

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

Ship it!


Looks like this stuff is actually needed for 
https://reviews.apache.org/r/37180. But can we add a comment in 
ResourcesOffersMessage that explains that we've added the InverseOffer field 
because that's what we use in master.cpp but we HAVE NOT fully implemented this 
yet so don't expect it blah blah blah so that people don't go down a rat hole 
and then get mad at us for stuff not working. Thanks!

- Benjamin Hindman


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

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

[Benjamin Hindman]

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

Ship it!



src/master/master.cpp (line 4144)


NOTE:



src/master/master.cpp (line 4828)


Indentation looks off here.


- Benjamin Hindman


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

Re: Review Request 37280: Maintenance Primitives: Added updateInverseOffer to Allocator.

[Benjamin Hindman]

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

Ship it!



include/mesos/master/allocator.hpp (line 147)


Comment please!



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


Let's invert this and do an if (offerResponse.isSome()) { and pull the code 
up and in?


- Benjamin Hindman


On Aug. 26, 2015, 2:13 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37280/
> ---
> 
> (Updated Aug. 26, 2015, 2:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
>   src/master/allocator/mesos/allocator.hpp 
> aa55755a9c3250579e9366bdbc17a2449e95d659 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
>   src/tests/mesos.hpp 637636ac69dde02da6b7200d7c666cac89b051cb 
> 
> Diff: https://reviews.apache.org/r/37280/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

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

[Benjamin Hindman]

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


Maybe you can kill this entire review however given my comments below?


src/internal/evolve.cpp (lines 162 - 163)


See comment below.



src/messages/messages.proto (line 171)


Let's not add this yet.


- Benjamin Hindman


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

Re: Review Request 37178: Maintenance Primitives: Added InverseOffers to Scheduler Event Offers.

[Benjamin Hindman]

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

Ship it!


Ship It!

- Benjamin Hindman


On Aug. 26, 2015, 2:12 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37178/
> ---
> 
> (Updated Aug. 26, 2015, 2:12 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/scheduler/scheduler.proto 
> 89daf8a6b74057ee156b3ad691397e76fcb835b8 
>   include/mesos/v1/scheduler/scheduler.proto 
> bd5e82a614b1163b29f9b20e562208efa1ba4b55 
> 
> Diff: https://reviews.apache.org/r/37178/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 37582: Maintenance Primitives: Add test for the hierarchical DRF allocator sending inverse offers.

[Benjamin Hindman]

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

Ship it!



src/tests/hierarchical_allocator_tests.cpp (line 474)


s/uRes/unavailableResources/


- Benjamin Hindman


On Aug. 25, 2015, 5:39 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37582/
> ---
> 
> (Updated Aug. 25, 2015, 5:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-3042
> https://issues.apache.org/jira/browse/MESOS-3042
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds a test for the previous review in this chain.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 9748ca0b3fee25dcec51c64d8ba84dbd4aaf 
> 
> Diff: https://reviews.apache.org/r/37582/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 37900: Maintenance Primitives: Add helper functions for the maintenance protobufs.

[Benjamin Hindman]

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

Ship it!



src/master/maintenance.hpp (line 48)


s/agenda/schedule/

Here AND below.



src/master/maintenance.cpp (line 78)


s/DEACTIVATED/DOWN/



src/master/maintenance.cpp (line 90)


s/interval/unavailability/



src/master/maintenance.cpp (line 94)


Duration::zero() exists, so you can kill this.



src/master/maintenance.cpp (line 95)


It seems weird to not let start be something before the epoch ... but we 
should still validate that the duration is greater than zero (because IIUC 
there isn't code that assumes backwards in time maintenance).



src/master/maintenance.cpp (line 99)


if (start < Duration::zero() || duration < Duration::zero()) {



src/master/maintenance.cpp (line 100)


'start' and 'duration'



src/master/maintenance.cpp (line 117)


Let's do a sweep and clean up all of these comments that no longer apply 
please!



src/master/maintenance.cpp (line 140)


Single quotes not double here.


- Benjamin Hindman


On Aug. 28, 2015, 7:15 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37900/
> ---
> 
> (Updated Aug. 28, 2015, 7:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes helpers for constructing (used in tests) and for validation 
> (used in HTTP endpoints).
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/common/protobuf_utils.hpp 63eeb77da5e6020085b9927ce15338aa6ed00488 
>   src/common/protobuf_utils.cpp 6b283558b5ef5c16f1d9f3dc2639df4f75c92f7d 
>   src/master/maintenance.hpp PRE-CREATION 
>   src/master/maintenance.cpp PRE-CREATION 
>   src/master/registrar.hpp c6a0655c212646618d93c9c85918af482a9ffd50 
> 
> Diff: https://reviews.apache.org/r/37900/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 37900: Maintenance Primitives: Add helper functions for the maintenance protobufs.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37655, 36321, 36571, 37314, 37900]

All tests passed.

- Mesos ReviewBot


On Aug. 28, 2015, 7:15 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37900/
> ---
> 
> (Updated Aug. 28, 2015, 7:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes helpers for constructing (used in tests) and for validation 
> (used in HTTP endpoints).
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/common/protobuf_utils.hpp 63eeb77da5e6020085b9927ce15338aa6ed00488 
>   src/common/protobuf_utils.cpp 6b283558b5ef5c16f1d9f3dc2639df4f75c92f7d 
>   src/master/maintenance.hpp PRE-CREATION 
>   src/master/maintenance.cpp PRE-CREATION 
>   src/master/registrar.hpp c6a0655c212646618d93c9c85918af482a9ffd50 
> 
> Diff: https://reviews.apache.org/r/37900/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

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

[Neil Conway]

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

Review request for mesos.


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


Repository: mesos


Description
---

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

Spotted via ubsan: see MESOS-3328.


Diffs
-

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

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 37881: Implemented AppcProvisioner.

[Jie Yu]

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



src/slave/containerizer/provisioner.cpp (line 27)


See my comments below. This is no longer needed.



src/slave/containerizer/provisioner.cpp (lines 43 - 46)


I would love to get this TODO solved in this patch. It should be pretty 
straightfoward, right? Just hard code them. And right now, only Appc is 
supported.



src/slave/containerizer/provisioners/appc.cpp (lines 79 - 97)


Some high level comments.

I think it will be beneficial if we can move the fetch and discovery logic 
to the Store. For the MVP, we don't have to impl. that in the Store.

The Store only has one interface, which is the 'get' method. 'get' takes an 
Image::Appc and returns a vector of paths to layers. THis semantics is more 
natural and easy to understand: get layers for my image, if some layers are 
already cached, return them directly, otherwise, discover it and fetch it.

The job of the provisioner is to get layers from the Store and stack them 
into a rootfs using the backend. It also manages the rootfs directories.

To me, this is a more natural split of functionalities among components. In 
the future, we can also potentially unify the impl. of the provisioner (i.e., 
one single provisioner for both Docker and Appc images). All the image specific 
details are hiden in the Store interface.

Thoughts?



src/slave/flags.hpp (line 53)


See my comments below. Why you still need this flag?



src/slave/flags.cpp (lines 72 - 76)


Why do you still need this flag?


- Jie Yu


On Aug. 28, 2015, 9:04 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37881/
> ---
> 
> (Updated Aug. 28, 2015, 9:04 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-2796
> https://issues.apache.org/jira/browse/MESOS-2796
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented AppcProvisioner.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioner.cpp 
> efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/paths.hpp 
> 41e3bf79da0854406c488855f953111e67353829 
>   src/slave/containerizer/provisioners/appc/paths.cpp 
> 3113c84b9526dd9e9e89fb9aa4ec75ed66a996c7 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/appc_provisioner_tests.cpp 
> 47b66b9c30cefe8f9a8e2c1c1341776c2d235020 
> 
> Diff: https://reviews.apache.org/r/37881/diff/
> 
> 
> Testing
> ---
> 
> sudo make check.
> 
> More test cases coming.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 37669: Ignore overflow components in version parsing.

[Bernd Mathiske]

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



3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp (line 59)


"allowed" and "would" are not quite right.

Suggested phrasing:

<< "only " << maxComponents << " components will "
<< "be recognized, the rest will be ignored."


- Bernd Mathiske


On Aug. 22, 2015, 10:04 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37669/
> ---
> 
> (Updated Aug. 22, 2015, 10:04 a.m.)
> 
> 
> Review request for mesos, Isabel Jimenez and Timothy Chen.
> 
> 
> Bugs: MESOS-2986
> https://issues.apache.org/jira/browse/MESOS-2986
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignore overflow components in version parsing.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 
> 51010845e8885ac52ce88ec0deb6b65a81122cba 
>   3rdparty/libprocess/3rdparty/stout/tests/version_tests.cpp 
> e8f8358f3c113b4e21e30cb5e9d6a3d229191484 
> 
> Diff: https://reviews.apache.org/r/37669/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37900: Maintenance Primitives: Add helper functions for the maintenance protobufs.

[Joseph Wu]

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

(Updated Aug. 28, 2015, 12:15 p.m.)


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


Changes
---

Add missing default argument for creating unavailability.


Repository: mesos


Description
---

This includes helpers for constructing (used in tests) and for validation (used 
in HTTP endpoints).


Diffs (updated)
-

  src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
  src/common/protobuf_utils.hpp 63eeb77da5e6020085b9927ce15338aa6ed00488 
  src/common/protobuf_utils.cpp 6b283558b5ef5c16f1d9f3dc2639df4f75c92f7d 
  src/master/maintenance.hpp PRE-CREATION 
  src/master/maintenance.cpp PRE-CREATION 
  src/master/registrar.hpp c6a0655c212646618d93c9c85918af482a9ffd50 

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


Testing
---

`make check`


Thanks,

Joseph Wu

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

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37871, 37427, 37773]

All tests passed.

- Mesos ReviewBot


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

Review Request 37900: Maintenance Primitives: Add helper functions for the maintenance protobufs.

[Joseph Wu]

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

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


Repository: mesos


Description
---

This includes helpers for constructing (used in tests) and for validation (used 
in HTTP endpoints).


Diffs
-

  src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
  src/common/protobuf_utils.hpp 63eeb77da5e6020085b9927ce15338aa6ed00488 
  src/common/protobuf_utils.cpp 6b283558b5ef5c16f1d9f3dc2639df4f75c92f7d 
  src/master/maintenance.hpp PRE-CREATION 
  src/master/maintenance.cpp PRE-CREATION 
  src/master/registrar.hpp c6a0655c212646618d93c9c85918af482a9ffd50 

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


Testing
---

`make check`


Thanks,

Joseph Wu

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

[Timothy Chen]

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


I have a feeling we can remove all the promises usage in this review. Let's 
chat offline.

- Timothy Chen


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

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

[Timothy Chen]

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



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


Why not just use option.getOrElse()?



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


userId



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


We usually spell timeout as one word everywhere else, so all lower case.



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


Do we need to define static here if it's not being used anywhere else but 
the cpp files?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 41)


Kill extra line



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 87)


What's the reasoning behind prefixing underscores for these? You've already 
added trailing underscores for your class variables right?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 91)


getOrElse



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 114)


I'm wondering why you need to use promise here.

If you put all this logic in getManifest then essentially you just need to 
return the dispatch right? And in getManifest you just chian your return with 
onFailed and onAny too.

Also even if you don't move it in getManifest why can't you just return 
result.onFailed(...).onAny(...)?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 153)


What residue?


- Timothy Chen


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

Re: Review Request 36908: Added QuotaInfo Protobuf.

[Alexander Rukletsov]

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



include/mesos/master/quota.proto (line 38)


s/guarantees/guarantee


- Alexander Rukletsov


On Aug. 5, 2015, 2:03 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36908/
> ---
> 
> (Updated Aug. 5, 2015, 2:03 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3164
> https://issues.apache.org/jira/browse/MESOS-3164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added QuotaInfo Protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/quota.hpp PRE-CREATION 
>   include/mesos/master/quota.proto PRE-CREATION 
>   src/Makefile.am 54eaf205eecb6bf1a9a5c4b5ddad55f46ad635ec 
> 
> Diff: https://reviews.apache.org/r/36908/diff/
> 
> 
> Testing
> ---
> 
> make distcheck
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

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

[Jojy Varghese]

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

(Updated Aug. 28, 2015, 6:38 p.m.)


Review request for mesos, Lily Chen and Timothy Chen.


Changes
---

review comments addressed


Repository: mesos


Description
---

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


Diffs (updated)
-

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

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


Testing
---

make check


Thanks,

Jojy Varghese

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

[Jojy Varghese]

> On Aug. 28, 2015, 5:21 p.m., Lily Chen wrote:
> > src/slave/containerizer/provisioners/docker/registry_client.hpp, line 34
> > 
> >
> > No need for provisioners namespace, see appc example of 
> > mesos::internal::slave::appc

Why is that?


- Jojy


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


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

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

[Jojy Varghese]

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

(Updated Aug. 28, 2015, 6:36 p.m.)


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


Changes
---

review comments.


Repository: mesos


Description
---

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


Diffs (updated)
-

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

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 37871: SSL tests refactoring

[Jojy Varghese]

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

(Updated Aug. 28, 2015, 6:35 p.m.)


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


Changes
---

Fixed build.


Repository: mesos


Description
---

Refactored libprocess SSL tests to be available for reuse by other projects.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 7ef515848508c2e84ab7607595f635f67e24b19b 
  3rdparty/libprocess/include/Makefile.am 
3b6108dd37d23bd5104162e9e8f4a3aa0518cdcd 
  3rdparty/libprocess/include/process/tests/ssl_test.hpp PRE-CREATION 
  3rdparty/libprocess/src/openssl_util.hpp  
  3rdparty/libprocess/src/openssl_util.cpp 
42b2860beccff929563ed05905da226b781918b7 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
7a316bc10575325ffe732fcc87d72d15a4fc5eaf 

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


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 37894: Renamed cgroups::kill to cgroups::signal.

[Jie Yu]

> On Aug. 28, 2015, 4:45 p.m., Cong Wang wrote:
> > Why? Everyone knows kill(2) sends a signal while signal(2) installs a 
> > signal handler...
> 
> Jie Yu wrote:
> IMO, that naming is confusing, and should be 'signal' and 'install'.
> 
> Joerg Schad wrote:
> This is actually answering a discussion here 
> https://reviews.apache.org/r/36620/#comment152376. And I agree that the 
> original names are weird, but you are right in that we should have an open 
> discussion about this. I will send a mail to dev later on...
> 
> Cong Wang wrote:
> Like it or not, it is already a part of POSIX (if not just UNIX), 
> pthread_kill() has the same "problem". It's just too late to change, as it 
> simply breaks every programmer's expectation.

This is cgroups::signal and cgroups::kill (has nothing to do with Posix kill 
and signal).

It reads more natural: signal a cgroup, kill a cgroup. I don't think it'll 
confuse the reader.

Are you suggesting that any thing suffixed with 'kill' should be linked to 
kill(2) in posix? I have to disagree with that.


- Jie


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


On Aug. 28, 2015, 4:39 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37894/
> ---
> 
> (Updated Aug. 28, 2015, 4:39 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> As this function is actually just sending the signal we renamed it. 
> Additionally we want to introduce an actual cgroups::kill() with MESOS-3086.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
>   src/tests/containerizer/cgroups_tests.cpp 
> 0b171eeb53037f26b7e952830e88e59f1278e7c6 
> 
> Diff: https://reviews.apache.org/r/37894/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Jie Yu]

> On Aug. 27, 2015, 7:02 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, lines 1776-1791
> > 
> >
> > Could you please introduce a new function under cgroups namespace and 
> > put this logic there:
> > 
> > ```
> > // Kill all processes in the given cgroup.
> > Future cgroups::kill(
> > const string& hierarchy,
> > const string& cgroup)
> > {
> >   ...
> >   if (freezerCheckError.isNone()) {
> >   } else {
> >   }
> >   
> >   return ...;
> > }
> > ```
> > 
> > You may want to rename the exsiting `cgroups::kill(hierarchy, cgroup, 
> > signal)` to `cgroups::signal`.
> 
> Timothy Chen wrote:
> Hi jie, thanks for chiming in. This should be ready after the comments 
> are addressed. I'll try to merge this as well when it's done. I'll ping Joerg 
> about this.
> 
> Joerg Schad wrote:
> Hi Jie,
> I am not sure whether I should move the entire logic further into 
> cgroups::kill. Both path (freezer vs Non-freezer) have distinct logic on how 
> to destroy the cgroups, which in my understanding makes sense to have in 
> seperate TasksKiller entities. Secondly cgroups::kill currently is little 
> more than trying to kill the cgroup, when moving the logic in here 
> cgroups::kill would have to call cgroups::freeze or call the iterative logic. 
> Secondly, in my understanding the interface here is the destroy() call. I 
> actually would like to refactor this piece of code in general, but this is 
> outside the scope of this patch. If we want to discuss this further -or 
> misunderstand something here- I would be happy to discuss it further!
> 
> Jie Yu wrote:
> Hi Joerg,
> 
> I think you misunderstood what I propose here. I am not asking you to 
> merge the logics of TasksKiller and FreezerTasksKiller. I think it's great 
> that you separate these two Processes. What I am sugguesting here is that you 
> pull the code from 1777-1790 to a separate function (like the following). We 
> may want to reuse that function in Mesos containerizer launcher in the future 
> (i.e., not rely on freezer). It also makes the logics in Destroyer cleaner. 
> Let me know your thoughts! Thank you for taking on this!
> 
> ```
> virtual void initialize()
> {
>   // Stop when no one cares.
>   promise.future().onDiscard(lambda::bind(
>   static_cast(terminate), self(), true));
> 
>   // Kill tasks in the given cgroups in parallel. Use collect mechanism to
>   // wait until all kill processes finish.
>   foreach (const string& cgroup, cgroups) {
> killers.push_back(cgroups:kill(hierarchy, cgroup));
>   }
> 
>   collect(killers)
> .onAny(defer(self(), &Destroyer::killed, lambda::_1));
> }
> 
> 
> // Kill all processes in the given cgroup.
> Future cgroups::kill(
> const string& hierarchy,
> const string& cgroup)
> {
>   Future future;
>   
>   // Use the freezer subsystem if available.
>   Option freezerCheckError =
> verify(hierarchy, cgroup, "freezer.state");
>   
>   if (freezerCheckError.isNone()) {
> internal::FreezerTasksKiller* killer =
>   new internal::FreezerTasksKiller(hierarchy, cgroup);
>  
> future = killer->future();
> 
> spawn(killer, true);
>   } else {
> internal::TasksKiller* killer =
>   new internal::TasksKiller(hierarchy, cgroup);
>   
> future = killer->future();
> 
> spawn(killer, true);
>   }
>   
>   return future;
> }
> ```
> 
> Joerg Schad wrote:
> As the renaming seems to be controversial 
> (https://reviews.apache.org/r/37894/), I would right now introduce 
> cgroups::killProcesses. Does that work for you?

yeah, that sounds good to me.


- Jie


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


On Aug. 28, 2015, 3:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Aug. 28, 2015, 3:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad

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

[Alexander Rukletsov]

> On Aug. 27, 2015, 5:51 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp, lines 179-181
> > 
> >
> > From the method name, I think it's not completely obvious what the type 
> > is.
> > 
> > Since I wouldn't want to type 
> > `Try>` either, you could consider 
> > renaming to something like `protobuf::parseRepeated`.

We can do that. If we rename it to `parseRepeated()` we can even change the 
signature to take `JSON::Object` as a parameter.


- Alexander


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


On Aug. 27, 2015, 3:38 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37827/
> ---
> 
> (Updated Aug. 27, 2015, 3:38 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
> c56d6a3098293eb3659b3066f10e875927ec3ac3 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 
> cfc2803e42284f641879fb24bce1282215c8ea52 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 
> a1d4084661345f9367c75f9db61279f032b93e69 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 
> bbd36d39e9588eb8eea6d739451ad3bab029ca08 
> 
> Diff: https://reviews.apache.org/r/37827/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> **NOTE**: Filed 
> [MESOS-3323](https://issues.apache.org/jira/browse/MESOS-3323) to clean up 
> protobuf generation.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37314: Maintenance Primitives: Populate master's maintenance-related local state upon recovery from registry.

[Joseph Wu]

> On Aug. 28, 2015, 8:19 a.m., Joris Van Remoortere wrote:
> > include/mesos/type_utils.hpp, line 158
> > 
> >
> > You have this on a new line to call it out for readability right? It 
> > would fit above otherwise.

Yup, that's right.


> On Aug. 28, 2015, 8:19 a.m., Joris Van Remoortere wrote:
> > include/mesos/type_utils.hpp, line 593
> > 
> >
> > You don't need the `std::` in `std::size_t`. I know you followed the 
> > pattern that got committed recently, it got followed up with this change :-)

Got it.  I didn't see the change until I rebased a few moments ago.


- Joseph


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


On Aug. 28, 2015, 10:38 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37314/
> ---
> 
> (Updated Aug. 28, 2015, 10:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-3045
> https://issues.apache.org/jira/browse/MESOS-3045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Note: Tests will be added with the related HTTP endpoints and registrar 
> operations, later in this review chain.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp 1c8f95b894285140a228ab4202851ee391ffdcc6 
>   src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc 
>   src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
> 
> Diff: https://reviews.apache.org/r/37314/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 37314: Maintenance Primitives: Populate master's maintenance-related local state upon recovery from registry.

[Joseph Wu]

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

(Updated Aug. 28, 2015, 10:38 a.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

Note: Tests will be added with the related HTTP endpoints and registrar 
operations, later in this review chain.


Diffs (updated)
-

  include/mesos/type_utils.hpp 1c8f95b894285140a228ab4202851ee391ffdcc6 
  src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc 
  src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 

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


Testing
---

`make check`


Thanks,

Joseph Wu

Re: Review Request 36571: Maintenance Primitives: Add maintenance-related, registry protobufs.

[Joseph Wu]

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

(Updated Aug. 28, 2015, 10:25 a.m.)


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


Changes
---

Fix a small, but important typo.


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


Repository: mesos


Description
---

New protobufs:

* MachineID - Describes a single box that holds one or more agents.
* MachineIDs - A list of boxes.
* MachineInfo - Holds a box and some extra information about its status.
* MachineInfo::Mode - An enum for the three states of a machine: UP, DRAINING, 
DOWN.
* maintenance::Window - A set of machines and a planned downtime period.
* maintenance::Schedule - A set of maintenance windows.


Diffs (updated)
-

  include/mesos/maintenance/maintenance.hpp PRE-CREATION 
  include/mesos/maintenance/maintenance.proto PRE-CREATION 
  include/mesos/mesos.proto c40a09b2bb2b9444e9b90e86eefe6fc8f98b613d 
  include/mesos/v1/mesos.proto ee15b9ae70aeb8cf803ea7e4b06f4443bd7bc9e2 
  src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
  src/master/registry.proto a1995e56886f5296bc71d1bdfebec0d8316396c6 

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


Testing
---

`make check`


Thanks,

Joseph Wu

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

[Lily Chen]

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



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


No need for provisioners namespace, see appc example of 
mesos::internal::slave::appc


- Lily Chen


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

Re: Review Request 37894: Renamed cgroups::kill to cgroups::signal.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [37894]

All tests passed.

- Mesos ReviewBot


On Aug. 28, 2015, 4:39 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37894/
> ---
> 
> (Updated Aug. 28, 2015, 4:39 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> As this function is actually just sending the signal we renamed it. 
> Additionally we want to introduce an actual cgroups::kill() with MESOS-3086.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
>   src/tests/containerizer/cgroups_tests.cpp 
> 0b171eeb53037f26b7e952830e88e59f1278e7c6 
> 
> Diff: https://reviews.apache.org/r/37894/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 36571: Maintenance Primitives: Add maintenance-related, registry protobufs.

[Joseph Wu]

> On Aug. 28, 2015, 8:12 a.m., Joris Van Remoortere wrote:
> > include/mesos/mesos.proto, line 160
> > 
> >
> > `a slave` or `an agent` ?

Since I added Machine* to the v1 API, I just used "slave" in one, and "agent" 
in the other.


> On Aug. 28, 2015, 8:12 a.m., Joris Van Remoortere wrote:
> > include/mesos/mesos.proto, line 162
> > 
> >
> > Is `Missing fields default to the empty string for the purpose of 
> > matching` still true?
> > Do you want to mention the hostname comparison is not case sensitive?

Not as true anymore.
And yes, I should.


> On Aug. 28, 2015, 8:12 a.m., Joris Van Remoortere wrote:
> > include/mesos/mesos.proto, lines 170-175
> > 
> >
> > Did you mention you were getting rid of the list version? Are we still 
> > going to use it?

I'll put a TODO here.  We'll want to remove it as soon as 
https://reviews.apache.org/r/37826/ is submitted.


> On Aug. 28, 2015, 8:12 a.m., Joris Van Remoortere wrote:
> > src/master/registry.proto, lines 55-57
> > 
> >
> > Why not follow the wrapper message pattern in the `NOTE` above:
> > ```
> > message Machines {
> >   repeated Machine machines = 1;
> > }
> > ```
> > 
> > Can you add a comment as to why not if you leave it as is?

I was conflicted if I wanted a list accessor like `.machines().machines()`, 
which is what the `slaves` accessor looks like.
I'll change it to match the pattern though, since it doesn't have too big of an 
impact.


- Joseph


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


On Aug. 28, 2015, 10:20 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36571/
> ---
> 
> (Updated Aug. 28, 2015, 10:20 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-3066
> https://issues.apache.org/jira/browse/MESOS-3066
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> New protobufs:
> 
> * MachineID - Describes a single box that holds one or more agents.
> * MachineIDs - A list of boxes.
> * MachineInfo - Holds a box and some extra information about its status.
> * MachineInfo::Mode - An enum for the three states of a machine: UP, 
> DRAINING, DOWN.
> * maintenance::Window - A set of machines and a planned downtime period.
> * maintenance::Schedule - A set of maintenance windows.
> 
> 
> Diffs
> -
> 
>   include/mesos/maintenance/maintenance.hpp PRE-CREATION 
>   include/mesos/maintenance/maintenance.proto PRE-CREATION 
>   include/mesos/mesos.proto c40a09b2bb2b9444e9b90e86eefe6fc8f98b613d 
>   include/mesos/v1/mesos.proto ee15b9ae70aeb8cf803ea7e4b06f4443bd7bc9e2 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/master/registry.proto a1995e56886f5296bc71d1bdfebec0d8316396c6 
> 
> Diff: https://reviews.apache.org/r/36571/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36571: Maintenance Primitives: Add maintenance-related, registry protobufs.

[Joseph Wu]

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

(Updated Aug. 28, 2015, 10:20 a.m.)


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


Changes
---

Comments, some tweaks for naming, and a rebase.


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


Repository: mesos


Description
---

New protobufs:

* MachineID - Describes a single box that holds one or more agents.
* MachineIDs - A list of boxes.
* MachineInfo - Holds a box and some extra information about its status.
* MachineInfo::Mode - An enum for the three states of a machine: UP, DRAINING, 
DOWN.
* maintenance::Window - A set of machines and a planned downtime period.
* maintenance::Schedule - A set of maintenance windows.


Diffs (updated)
-

  include/mesos/maintenance/maintenance.hpp PRE-CREATION 
  include/mesos/maintenance/maintenance.proto PRE-CREATION 
  include/mesos/mesos.proto c40a09b2bb2b9444e9b90e86eefe6fc8f98b613d 
  include/mesos/v1/mesos.proto ee15b9ae70aeb8cf803ea7e4b06f4443bd7bc9e2 
  src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
  src/master/registry.proto a1995e56886f5296bc71d1bdfebec0d8316396c6 

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


Testing
---

`make check`


Thanks,

Joseph Wu

Re: Review Request 36321: Maintenance primitives: Add Unavailability and InverseOffer protobufs.

[Joseph Wu]

> On Aug. 28, 2015, 7:49 a.m., Joris Van Remoortere wrote:
> > include/mesos/mesos.proto, line 877
> > 
> >
> > In your last diff, you changed a bunch of `window` to `interval`. Do 
> > you want to change this one as well? If not, are you sure you wanted all 
> > the `window` to `interval` changes in diff 13-14?

Missed that one.  Good catch :)

I'm changing all the synonyms I use to describe unavailability (period, window, 
interval, range, etc) just to "interval", for clarity.


- Joseph


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


On Aug. 28, 2015, 10:11 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36321/
> ---
> 
> (Updated Aug. 28, 2015, 10:11 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-2061 and MESOS-2066
> https://issues.apache.org/jira/browse/MESOS-2061
> https://issues.apache.org/jira/browse/MESOS-2066
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-2061: Add Unavailability and InverseOffer protobufs declarations.
> MESOS-2066: Add the Unavailability field to Offers.
> 
> Also copied to v1 API.
> 
> No integration with other components (that part is tracked in separate JIRAs, 
> see MESOS-1474).
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto c40a09b2bb2b9444e9b90e86eefe6fc8f98b613d 
>   include/mesos/v1/mesos.proto ee15b9ae70aeb8cf803ea7e4b06f4443bd7bc9e2 
> 
> Diff: https://reviews.apache.org/r/36321/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36321: Maintenance primitives: Add Unavailability and InverseOffer protobufs.

[Joseph Wu]

> On Aug. 27, 2015, 1:06 p.m., Benjamin Hindman wrote:
> > include/mesos/mesos.proto, lines 926-927
> > 
> >
> > Why do we need the URL? Can we comment for folks so they know what they 
> > might need/use this for?

Added some explanation.  We can probably update the URL field in `Offer` with 
the same comment, separately.


- Joseph


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


On Aug. 28, 2015, 10:11 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36321/
> ---
> 
> (Updated Aug. 28, 2015, 10:11 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-2061 and MESOS-2066
> https://issues.apache.org/jira/browse/MESOS-2061
> https://issues.apache.org/jira/browse/MESOS-2066
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-2061: Add Unavailability and InverseOffer protobufs declarations.
> MESOS-2066: Add the Unavailability field to Offers.
> 
> Also copied to v1 API.
> 
> No integration with other components (that part is tracked in separate JIRAs, 
> see MESOS-1474).
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto c40a09b2bb2b9444e9b90e86eefe6fc8f98b613d 
>   include/mesos/v1/mesos.proto ee15b9ae70aeb8cf803ea7e4b06f4443bd7bc9e2 
> 
> Diff: https://reviews.apache.org/r/36321/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36321: Maintenance primitives: Add Unavailability and InverseOffer protobufs.

[Joseph Wu]

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

(Updated Aug. 28, 2015, 10:11 a.m.)


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


Changes
---

Comments!  And a rebase.


Bugs: MESOS-2061 and MESOS-2066
https://issues.apache.org/jira/browse/MESOS-2061
https://issues.apache.org/jira/browse/MESOS-2066


Repository: mesos


Description
---

MESOS-2061: Add Unavailability and InverseOffer protobufs declarations.
MESOS-2066: Add the Unavailability field to Offers.

Also copied to v1 API.

No integration with other components (that part is tracked in separate JIRAs, 
see MESOS-1474).


Diffs (updated)
-

  include/mesos/mesos.proto c40a09b2bb2b9444e9b90e86eefe6fc8f98b613d 
  include/mesos/v1/mesos.proto ee15b9ae70aeb8cf803ea7e4b06f4443bd7bc9e2 

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


Testing
---

`make check`


Thanks,

Joseph Wu

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

[Jojy Varghese]

> On Aug. 28, 2015, 12:18 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.hpp, line 223
> > 
> >
> > Is there a reason you need to include this in the header? Can we just 
> > put it in cpp?
> 
> Jojy Varghese wrote:
> The only reason being that this is a property of the TokenManager.
> 
> Timothy Chen wrote:
> I see, does it need to be? Can't we define a static const in the cpp?

We could but then it wont be a class property. Here the constant reflects the 
property of the class.


- Jojy


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


On Aug. 28, 2015, 4:27 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 28, 2015, 4:27 a.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - Added Token implementation (RFC 7519).
>   - Added TokenManager implementation. This component keeps a cache of tokens
>   requested for any future requests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Joerg Schad]

> On Aug. 27, 2015, 7:02 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, lines 1776-1791
> > 
> >
> > Could you please introduce a new function under cgroups namespace and 
> > put this logic there:
> > 
> > ```
> > // Kill all processes in the given cgroup.
> > Future cgroups::kill(
> > const string& hierarchy,
> > const string& cgroup)
> > {
> >   ...
> >   if (freezerCheckError.isNone()) {
> >   } else {
> >   }
> >   
> >   return ...;
> > }
> > ```
> > 
> > You may want to rename the exsiting `cgroups::kill(hierarchy, cgroup, 
> > signal)` to `cgroups::signal`.
> 
> Timothy Chen wrote:
> Hi jie, thanks for chiming in. This should be ready after the comments 
> are addressed. I'll try to merge this as well when it's done. I'll ping Joerg 
> about this.
> 
> Joerg Schad wrote:
> Hi Jie,
> I am not sure whether I should move the entire logic further into 
> cgroups::kill. Both path (freezer vs Non-freezer) have distinct logic on how 
> to destroy the cgroups, which in my understanding makes sense to have in 
> seperate TasksKiller entities. Secondly cgroups::kill currently is little 
> more than trying to kill the cgroup, when moving the logic in here 
> cgroups::kill would have to call cgroups::freeze or call the iterative logic. 
> Secondly, in my understanding the interface here is the destroy() call. I 
> actually would like to refactor this piece of code in general, but this is 
> outside the scope of this patch. If we want to discuss this further -or 
> misunderstand something here- I would be happy to discuss it further!
> 
> Jie Yu wrote:
> Hi Joerg,
> 
> I think you misunderstood what I propose here. I am not asking you to 
> merge the logics of TasksKiller and FreezerTasksKiller. I think it's great 
> that you separate these two Processes. What I am sugguesting here is that you 
> pull the code from 1777-1790 to a separate function (like the following). We 
> may want to reuse that function in Mesos containerizer launcher in the future 
> (i.e., not rely on freezer). It also makes the logics in Destroyer cleaner. 
> Let me know your thoughts! Thank you for taking on this!
> 
> ```
> virtual void initialize()
> {
>   // Stop when no one cares.
>   promise.future().onDiscard(lambda::bind(
>   static_cast(terminate), self(), true));
> 
>   // Kill tasks in the given cgroups in parallel. Use collect mechanism to
>   // wait until all kill processes finish.
>   foreach (const string& cgroup, cgroups) {
> killers.push_back(cgroups:kill(hierarchy, cgroup));
>   }
> 
>   collect(killers)
> .onAny(defer(self(), &Destroyer::killed, lambda::_1));
> }
> 
> 
> // Kill all processes in the given cgroup.
> Future cgroups::kill(
> const string& hierarchy,
> const string& cgroup)
> {
>   Future future;
>   
>   // Use the freezer subsystem if available.
>   Option freezerCheckError =
> verify(hierarchy, cgroup, "freezer.state");
>   
>   if (freezerCheckError.isNone()) {
> internal::FreezerTasksKiller* killer =
>   new internal::FreezerTasksKiller(hierarchy, cgroup);
>  
> future = killer->future();
> 
> spawn(killer, true);
>   } else {
> internal::TasksKiller* killer =
>   new internal::TasksKiller(hierarchy, cgroup);
>   
> future = killer->future();
> 
> spawn(killer, true);
>   }
>   
>   return future;
> }
> ```

As the renaming seems to be controversial 
(https://reviews.apache.org/r/37894/), I would right now introduce 
cgroups::killProcesses. Does that work for you?


- Joerg


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


On Aug. 28, 2015, 3:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Aug. 28, 2015, 3:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 37894: Renamed cgroups::kill to cgroups::signal.

[Cong Wang]

> On Aug. 28, 2015, 4:45 p.m., Cong Wang wrote:
> > Why? Everyone knows kill(2) sends a signal while signal(2) installs a 
> > signal handler...
> 
> Jie Yu wrote:
> IMO, that naming is confusing, and should be 'signal' and 'install'.
> 
> Joerg Schad wrote:
> This is actually answering a discussion here 
> https://reviews.apache.org/r/36620/#comment152376. And I agree that the 
> original names are weird, but you are right in that we should have an open 
> discussion about this. I will send a mail to dev later on...

Like it or not, it is already a part of POSIX (if not just UNIX), 
pthread_kill() has the same "problem". It's just too late to change, as it 
simply breaks every programmer's expectation.


- Cong


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


On Aug. 28, 2015, 4:39 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37894/
> ---
> 
> (Updated Aug. 28, 2015, 4:39 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> As this function is actually just sending the signal we renamed it. 
> Additionally we want to introduce an actual cgroups::kill() with MESOS-3086.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
>   src/tests/containerizer/cgroups_tests.cpp 
> 0b171eeb53037f26b7e952830e88e59f1278e7c6 
> 
> Diff: https://reviews.apache.org/r/37894/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 37894: Renamed cgroups::kill to cgroups::signal.

[Joerg Schad]

> On Aug. 28, 2015, 4:45 p.m., Cong Wang wrote:
> > Why? Everyone knows kill(2) sends a signal while signal(2) installs a 
> > signal handler...
> 
> Jie Yu wrote:
> IMO, that naming is confusing, and should be 'signal' and 'install'.

This is actually answering a discussion here 
https://reviews.apache.org/r/36620/#comment152376. And I agree that the 
original names are weird, but you are right in that we should have an open 
discussion about this. I will send a mail to dev later on...


- Joerg


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


On Aug. 28, 2015, 4:39 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37894/
> ---
> 
> (Updated Aug. 28, 2015, 4:39 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> As this function is actually just sending the signal we renamed it. 
> Additionally we want to introduce an actual cgroups::kill() with MESOS-3086.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
>   src/tests/containerizer/cgroups_tests.cpp 
> 0b171eeb53037f26b7e952830e88e59f1278e7c6 
> 
> Diff: https://reviews.apache.org/r/37894/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 37894: Renamed cgroups::kill to cgroups::signal.

[Jie Yu]

> On Aug. 28, 2015, 4:45 p.m., Cong Wang wrote:
> > Why? Everyone knows kill(2) sends a signal while signal(2) installs a 
> > signal handler...

IMO, that naming is confusing, and should be 'signal' and 'install'.


- Jie


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


On Aug. 28, 2015, 4:39 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37894/
> ---
> 
> (Updated Aug. 28, 2015, 4:39 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> As this function is actually just sending the signal we renamed it. 
> Additionally we want to introduce an actual cgroups::kill() with MESOS-3086.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
>   src/tests/containerizer/cgroups_tests.cpp 
> 0b171eeb53037f26b7e952830e88e59f1278e7c6 
> 
> Diff: https://reviews.apache.org/r/37894/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 37894: Renamed cgroups::kill to cgroups::signal.

[Cong Wang]

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


Why? Everyone knows kill(2) sends a signal while signal(2) installs a signal 
handler...

- Cong Wang


On Aug. 28, 2015, 4:39 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37894/
> ---
> 
> (Updated Aug. 28, 2015, 4:39 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> As this function is actually just sending the signal we renamed it. 
> Additionally we want to introduce an actual cgroups::kill() with MESOS-3086.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
>   src/tests/containerizer/cgroups_tests.cpp 
> 0b171eeb53037f26b7e952830e88e59f1278e7c6 
> 
> Diff: https://reviews.apache.org/r/37894/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Review Request 37894: Renamed cgroups::kill to cgroups::signal.

[Joerg Schad]

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

Review request for mesos, Jie Yu, Till Toenshoff, and Timothy Chen.


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


Repository: mesos


Description
---

As this function is actually just sending the signal we renamed it. 
Additionally we want to introduce an actual cgroups::kill() with MESOS-3086.


Diffs
-

  src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
  src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
  src/tests/containerizer/cgroups_tests.cpp 
0b171eeb53037f26b7e952830e88e59f1278e7c6 

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


Testing
---


Thanks,

Joerg Schad

Re: Review Request 37892: Renamed cgroups::kill to cgroups::signal

[Joerg Schad]

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

(Updated Aug. 28, 2015, 4:29 p.m.)


Review request for mesos.


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


Repository: mesos


Description
---

As this function is actually just sending the signal we renamed it. 
Additionally we want to introduce an actual cgroups::kill() with MESOS-3086.


Diffs (updated)
-

  src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 
  src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
  src/tests/containerizer/cgroups_tests.cpp 
0b171eeb53037f26b7e952830e88e59f1278e7c6 

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


Testing
---


Thanks,

Joerg Schad

Re: Review Request 37892: Renamed cgroups::kill to cgroups::signal

[Joerg Schad]

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

(Updated Aug. 28, 2015, 4:12 p.m.)


Review request for mesos.


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


Repository: mesos


Description
---

As this function is actually just sending the signal we renamed it. 
Additionally we want to introduce an actual cgroups::kill() with MESOS-3086.


Diffs (updated)
-

  src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
  src/tests/containerizer/cgroups_tests.cpp 
0b171eeb53037f26b7e952830e88e59f1278e7c6 

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


Testing
---


Thanks,

Joerg Schad

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

[Jojy Varghese]

> On Aug. 28, 2015, 7:56 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker/token_manager.cpp, line 223
> > 
> >
> > If you put the cache here in TokenManager instead of 
> > TokenManagerProcess then you'll going to be concurrent access of all the 
> > fields since it's not serialized by libprocess.
> > 
> > I think tokenCache_ will have undefined behavior  when you run 
> > at/contains while it's being inserted  or deleted then.
> > 
> > I suggest you move all this into the process instead.

Here the getToken is called in the context of a "dispatch" from RegistryClient. 
Do we still need another dispatch?


- Jojy


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


On Aug. 28, 2015, 4:27 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Aug. 28, 2015, 4:27 a.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - Added Token implementation (RFC 7519).
>   - Added TokenManager implementation. This component keeps a cache of tokens
>   requested for any future requests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Review Request 37892: Renamed cgroups::kill to cgroups::signal

[Joerg Schad]

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

Review request for mesos.


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


Repository: mesos


Description
---

As this function is actually just sending the signal we renamed it. 
Additionally we want to introduce an actual cgroups::kill() with MESOS-3086.


Diffs
-

  src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
  src/tests/containerizer/cgroups_tests.cpp 
0b171eeb53037f26b7e952830e88e59f1278e7c6 

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


Testing
---


Thanks,

Joerg Schad

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Jie Yu]

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



src/linux/cgroups.cpp (line 1676)


Hum, what if 'cgroups::processes' fails the first time (i.e., 'chain' 
hasn't been set up yet)?

I would suggest terminating the process if error is encountered:

```
Try> processes = ...;
if (processes.isError()) {
  promise.fail("Failed to ...: " + processes.error());
  terminate(self());
  return;
}
```



src/linux/cgroups.cpp (line 1681)


First of all, no need to pass in 'chain' because it's a member field of 
this class.

Second, I don't think we should call 'finished' here because you can simply 
do the following and save a call to 'cgroups::procsses'

```
if (processes.get().empty()) {
  promise.set(Nothing());
  terminate(self());
  return;
}
```



src/linux/cgroups.cpp (lines 1685 - 1686)


It's not really a chain here. In fact, we don't need a 'chain' variable 
here. The 'kill' (or 'signal') is a synchronour operation. ALso, no need to 
pull 'collect(statuses)' into a separate function. Here is my suggestion:

```
statuses.clear();

// Send SIGKILL to all processes.
foreach (pid_t pid, processes.get()) {
  ::kill(...);
  
  statuses.push_back(process::reap(pid));
}

collect(statuses)
  .onAny(defer(self(), &Self::_killTasksIteration));
  
...

void _killTasksIteration(const Future<...>& future)
{
  if (!future.isReady()) {
promise.fail(...);
terminate(self());
return;
  }
  
  killTasksIteration();
}

virtual void finalize()
{
  discard(statuses);
  
  promise.discard();
}
```


- Jie Yu


On Aug. 28, 2015, 3:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Aug. 28, 2015, 3:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 37891: Fetcher now does not erroneously report a non-archive when extracting from one.

[Bernd Mathiske]

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

(Updated Aug. 28, 2015, 8:51 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy 
Chen.


Changes
---

Added bug number.


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


Repository: mesos


Description
---

Prevented fetcher from erroneously reporting a non-archive when successfully 
extracting from one.


Diffs
-

  src/launcher/fetcher.cpp 79b1d98c6dbd802bf3b4d6e60019ee9155f5c7b9 

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


Testing
---

make check
Ran one affected test manually and inspected that the erroneous output is now 
missing from the sandbox.


Thanks,

Bernd Mathiske

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Joerg Schad]

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

(Updated Aug. 28, 2015, 3:48 p.m.)


Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.


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


Repository: mesos


Description (updated)
---

Added Non-Freezeer Task Killer.


Diffs
-

  src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 

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


Testing
---

sudo make check
+ manual tests


Thanks,

Joerg Schad

Review Request 37891: Fetcher now does not erroneously report a non-archive when extracting from one.

[Bernd Mathiske]

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

Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy 
Chen.


Repository: mesos


Description
---

Prevented fetcher from erroneously reporting a non-archive when successfully 
extracting from one.


Diffs
-

  src/launcher/fetcher.cpp 79b1d98c6dbd802bf3b4d6e60019ee9155f5c7b9 

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


Testing
---

make check
Ran one affected test manually and inspected that the erroneous output is now 
missing from the sandbox.


Thanks,

Bernd Mathiske

Re: Review Request 37314: Maintenance Primitives: Populate master's maintenance-related local state upon recovery from registry.

[Joris Van Remoortere]

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

Ship it!



include/mesos/type_utils.hpp (line 153)


s/Note\:/NOTE\:/



include/mesos/type_utils.hpp (line 158)


You have this on a new line to call it out for readability right? It would 
fit above otherwise.



include/mesos/type_utils.hpp (line 593)


You don't need the `std::` in `std::size_t`. I know you followed the 
pattern that got committed recently, it got followed up with this change :-)


- Joris Van Remoortere


On Aug. 28, 2015, 1:07 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37314/
> ---
> 
> (Updated Aug. 28, 2015, 1:07 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-3045
> https://issues.apache.org/jira/browse/MESOS-3045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Note: Tests will be added with the related HTTP endpoints and registrar 
> operations, later in this review chain.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp 8d27ef439d8bfb30a10b2f32fcd893f0af08ed75 
>   src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc 
>   src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
> 
> Diff: https://reviews.apache.org/r/37314/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36571: Maintenance Primitives: Add maintenance-related, registry protobufs.

[Joris Van Remoortere]

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

Ship it!



include/mesos/maintenance/maintenance.proto (line 56)


s/Period/Interval/



include/mesos/mesos.proto (line 160)


`a slave` or `an agent` ?



include/mesos/mesos.proto (line 162)


Is `Missing fields default to the empty string for the purpose of matching` 
still true?
Do you want to mention the hostname comparison is not case sensitive?



include/mesos/mesos.proto (lines 170 - 175)


Did you mention you were getting rid of the list version? Are we still 
going to use it?



include/mesos/v1/mesos.proto (line 162)


don't forget to update this if you change it in the old proto.



include/mesos/v1/mesos.proto (lines 170 - 175)


in case you decide to remove this in the old proto.



src/master/registry.proto (lines 55 - 57)


Why not follow the wrapper message pattern in the `NOTE` above:
```
message Machines {
  repeated Machine machines = 1;
}
```

Can you add a comment as to why not if you leave it as is?


- Joris Van Remoortere


On Aug. 28, 2015, 12:57 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36571/
> ---
> 
> (Updated Aug. 28, 2015, 12:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, 
> Joris Van Remoortere, and Vinod Kone.
> 
> 
> Bugs: MESOS-3066
> https://issues.apache.org/jira/browse/MESOS-3066
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> New protobufs:
> 
> * MachineID - Describes a single box that holds one or more agents.
> * MachineIDs - A list of boxes.
> * MachineInfo - Holds a box and some extra information about its status.
> * MachineInfo::Mode - An enum for the three states of a machine: UP, 
> DRAINING, DOWN.
> * maintenance::Window - A set of machines and a planned downtime period.
> * maintenance::Schedule - A set of maintenance windows.
> 
> 
> Diffs
> -
> 
>   include/mesos/maintenance/maintenance.hpp PRE-CREATION 
>   include/mesos/maintenance/maintenance.proto PRE-CREATION 
>   include/mesos/mesos.proto 715b8cf38e1e56c18a3f2ddbb82c920bd9414f05 
>   include/mesos/v1/mesos.proto 382b978dca769757171c5558b7f259870592c321 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/master/registry.proto a1995e56886f5296bc71d1bdfebec0d8316396c6 
> 
> Diff: https://reviews.apache.org/r/36571/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36620: Added Non-Freezeer Task Killer.

[Jie Yu]

> On Aug. 27, 2015, 7:02 p.m., Jie Yu wrote:
> > src/linux/cgroups.cpp, lines 1776-1791
> > 
> >
> > Could you please introduce a new function under cgroups namespace and 
> > put this logic there:
> > 
> > ```
> > // Kill all processes in the given cgroup.
> > Future cgroups::kill(
> > const string& hierarchy,
> > const string& cgroup)
> > {
> >   ...
> >   if (freezerCheckError.isNone()) {
> >   } else {
> >   }
> >   
> >   return ...;
> > }
> > ```
> > 
> > You may want to rename the exsiting `cgroups::kill(hierarchy, cgroup, 
> > signal)` to `cgroups::signal`.
> 
> Timothy Chen wrote:
> Hi jie, thanks for chiming in. This should be ready after the comments 
> are addressed. I'll try to merge this as well when it's done. I'll ping Joerg 
> about this.
> 
> Joerg Schad wrote:
> Hi Jie,
> I am not sure whether I should move the entire logic further into 
> cgroups::kill. Both path (freezer vs Non-freezer) have distinct logic on how 
> to destroy the cgroups, which in my understanding makes sense to have in 
> seperate TasksKiller entities. Secondly cgroups::kill currently is little 
> more than trying to kill the cgroup, when moving the logic in here 
> cgroups::kill would have to call cgroups::freeze or call the iterative logic. 
> Secondly, in my understanding the interface here is the destroy() call. I 
> actually would like to refactor this piece of code in general, but this is 
> outside the scope of this patch. If we want to discuss this further -or 
> misunderstand something here- I would be happy to discuss it further!

Hi Joerg,

I think you misunderstood what I propose here. I am not asking you to merge the 
logics of TasksKiller and FreezerTasksKiller. I think it's great that you 
separate these two Processes. What I am sugguesting here is that you pull the 
code from 1777-1790 to a separate function (like the following). We may want to 
reuse that function in Mesos containerizer launcher in the future (i.e., not 
rely on freezer). It also makes the logics in Destroyer cleaner. Let me know 
your thoughts! Thank you for taking on this!

```
virtual void initialize()
{
  // Stop when no one cares.
  promise.future().onDiscard(lambda::bind(
  static_cast(terminate), self(), true));

  // Kill tasks in the given cgroups in parallel. Use collect mechanism to
  // wait until all kill processes finish.
  foreach (const string& cgroup, cgroups) {
killers.push_back(cgroups:kill(hierarchy, cgroup));
  }

  collect(killers)
.onAny(defer(self(), &Destroyer::killed, lambda::_1));
}


// Kill all processes in the given cgroup.
Future cgroups::kill(
const string& hierarchy,
const string& cgroup)
{
  Future future;
  
  // Use the freezer subsystem if available.
  Option freezerCheckError =
verify(hierarchy, cgroup, "freezer.state");
  
  if (freezerCheckError.isNone()) {
internal::FreezerTasksKiller* killer =
  new internal::FreezerTasksKiller(hierarchy, cgroup);
 
future = killer->future();

spawn(killer, true);
  } else {
internal::TasksKiller* killer =
  new internal::TasksKiller(hierarchy, cgroup);
  
future = killer->future();

spawn(killer, true);
  }
  
  return future;
}
```


- Jie


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


On Aug. 24, 2015, 9:33 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36620/
> ---
> 
> (Updated Aug. 24, 2015, 9:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
> 
> 
> Bugs: MESOS-3086
> https://issues.apache.org/jira/browse/MESOS-3086
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP Added Non-Freezeer Task Killer.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f 
> 
> Diff: https://reviews.apache.org/r/36620/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> + manual tests
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>